Preserve itx alloc size for zio_data_buf_free() #6912
Conversation
There was a problem hiding this comment.
LGTM.
Can you also comment a little more about the motivation to move back to using vmem_alloc. Since we moved from vmem_alloc to zio_data_buf_alloc, and now we're going back to vmem_alloc, I think it'd be good to explain why we're not regressing on commit 19ea3d2 by doing this.
Additionally, we should port this to openzfs right? At least, the portion adding itx_size to the itx_t structure.
include/sys/zil.h
Outdated
| @@ -395,6 +395,7 @@ typedef struct itx { | |||
| uint8_t itx_sync; /* synchronous transaction */ | |||
| zil_callback_t itx_callback; /* Called when the itx is persistent */ | |||
| void *itx_callback_data; /* User data for the callback */ | |||
| size_t itx_size; /* itx size including lr.lrc_reclen */ | |||
There was a problem hiding this comment.
the comment here seems a little sketchy to me, since if lr.lrc_reclen changes, we won't modify itx_size to match this comment. it might be better to state this field is used to ensure we properly free the itx_t structure, since that's really the reason the field exists, right? with the comment as is, I might interpret the fact we don't modify this field when changing lr.lrc_reclen as a bug, when that's not the case.
There was a problem hiding this comment.
Good point, right this simply needs to exist because kmem_free() requires the size. Passing a different value for the size will only result in an ASSERT when DEBUG_KMEM_TRACKING is defined. I'll change it to,
size_t itx_size; /* allocated itx struct size */
Will do, I'll expand further on the why this is OK in the commit message comment.
Yes, we probably should port the itx->itx_size portion to ensure that alloc/free match. I'm not sure what, if any, consequences there are in OpenZFS if these differ. |
behlendorf
force-pushed
the
zil-itx-alloc
branch
from
c0a360e to
13effb3
Compare
behlendorf
changed the title
Using zio_data_buf_alloc() to allocate the itx's may be unsafe because the itx->itx_lr.lrc_reclen field is not constant from allocation to free. Using a different itx->itx_lr.lrc_reclen size in zio_data_buf_free() can result in the allocation being returned to the wrong kmem cache. This issue can be avoided entirely by storing the allocation size in itx->itx_size and using that for zio_data_buf_free(). Signed-off-by: Brian Behlendorf <[email protected]>
|
@prakashsurya refreshed. I've reconsidered the |
Codecov Report
@@ Coverage Diff @@
## master #6912 +/- ##
=========================================
+ Coverage 75.18% 75.2% +0.02%
=========================================
Files 296 296
Lines 95197 95199 +2
=========================================
+ Hits 71577 71598 +21
+ Misses 23620 23601 -19
Continue to review full report at Codecov.
|
Using zio_data_buf_alloc() to allocate the itx's may be unsafe because the itx->itx_lr.lrc_reclen field is not constant from allocation to free. Using a different itx->itx_lr.lrc_reclen size in zio_data_buf_free() can result in the allocation being returned to the wrong kmem cache. This issue can be avoided entirely by storing the allocation size in itx->itx_size and using that for zio_data_buf_free(). Reviewed by: Prakash Surya <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes #6912
Using zio_data_buf_alloc() to allocate the itx's may be unsafe because the itx->itx_lr.lrc_reclen field is not constant from allocation to free. Using a different itx->itx_lr.lrc_reclen size in zio_data_buf_free() can result in the allocation being returned to the wrong kmem cache. This issue can be avoided entirely by storing the allocation size in itx->itx_size and using that for zio_data_buf_free(). Reviewed by: Prakash Surya <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#6912
Using zio_data_buf_alloc() to allocate the itx's may be unsafe because the itx->itx_lr.lrc_reclen field is not constant from allocation to free. Using a different itx->itx_lr.lrc_reclen size in zio_data_buf_free() can result in the allocation being returned to the wrong kmem cache. This issue can be avoided entirely by storing the allocation size in itx->itx_size and using that for zio_data_buf_free(). Reviewed by: Prakash Surya <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#6912
Using zio_data_buf_alloc() to allocate the itx's may be unsafe because the itx->itx_lr.lrc_reclen field is not constant from allocation to free. Using a different itx->itx_lr.lrc_reclen size in zio_data_buf_free() can result in the allocation being returned to the wrong kmem cache. This issue can be avoided entirely by storing the allocation size in itx->itx_size and using that for zio_data_buf_free(). Reviewed by: Prakash Surya <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#6912
Description
Using zio_data_buf_alloc() to allocate the itx's may be unsafe
because the itx->itx_lr.lrc_reclen field is not constant from
allocation to free. This size is used by zio_data_buf_free() to
determine which cache the object should be returned to, a change
in size may result in the wrong cache being selected.
Avoid this issue entirely by storing the allocation size in the
itx as itx->itx_size. Additionally, switch to using vmem_alloc()
to perform the allocation. For the vast majority of allocations
which are <64k this will internally map to kmem_alloc() and will
therefore not impact performance.
Motivation and Context
Potential issue identified by inspection while testing #6566.
How Has This Been Tested?
Submitted PR for a full run of the ZTS.
Types of changes
Checklist:
Signed-off-by.