Guarantee PAGESIZE alignment for large zio buffers #6084
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In current implementation, only zio buffers in 16KB and bigger are guaranteed PAGESIZE alignment. This breaks Lustre since it assumes that 'arc_buf_t::b_data' must be page aligned when zio buffers are greater than or equal to PAGESIZE. This patch will make the zio buffers to be PAGESIZE aligned when the sizes are not less than PAGESIZE. This change may cause a little bit memory waste but that should be fine because after ABD is introduced, zio buffers are used to hold data temporarily and live in memory for a short while. Signed-off-by: Jinshan Xiong <jinshan.xiong@gmail.com> Signed-off-by: Jinshan Xiong <jinshan.xiong@intel.com>
|
@jxiong, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @don-brady and @dpquigl to be potential reviewers. |
|
@jxiong nice find. You may want to add some alignment assertions on the Lustre side to make sure we quickly catch any potential future changes in the area. |
behlendorf
approved these changes
May 1, 2017
|
@behlendorf yes I already did it. Thanks for reminding. |
behlendorf
reviewed
May 1, 2017
| @@ -167,10 +167,10 @@ zio_init(void) | |||
| */ | |||
| align = 8 * SPA_MINBLOCKSIZE; | |||
| #else | |||
| if (size <= 4 * SPA_MINBLOCKSIZE) { | |||
| if (size < PAGESIZE) { | |||
There was a problem hiding this comment.
This will result in 3 additional caches being created for tiny buffers. That's not a problem and likely desirable but worth mentioning.
|
Will this fix be cherry-picked into the 0.6.5 branch? |
|
@adilger it's small enough I wouldn't object to cherry-picking it if you need it. |
|
We're looking at how to handle this in Lustre, and without this fix it adds a constant (additional) memcpy() overhead for accessing small files. IMHO, it is preferable to backport this fix to 0.6.5 and just align the allocation correctly in the first place. That also avoids potential corruption for anyone using Lustre+ZFS 0.6.5.x without the Lustre-side fix. |
|
I've added to the list of patched to check pick for 0.6.5.10 |
rread
pushed a commit
to rread/lustre
that referenced
this pull request
May 16, 2017
ZFS only guarantees PAGE_SIZE alignment to arc_buf_t only when the block size is not less than (PAGE_SIZE << 2). The patch for ZFS openzfs/zfs#6084 fixes the alignment problem, buf Lustre still needs a fix to handle the problem in case it's running old ZFS release. Signed-off-by: Jinshan Xiong <jinshan.xiong@intel.com> Change-Id: I6fd17d7b20499ec0406a3e10cebf6882b92a730f Reviewed-on: https://review.whamcloud.com/26895 Tested-by: Jenkins Tested-by: Maloo <hpdd-maloo@intel.com> Reviewed-by: Alex Zhuravlev <alexey.zhuravlev@intel.com> Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
May 25, 2017
In current implementation, only zio buffers in 16KB and bigger are guaranteed PAGESIZE alignment. This breaks Lustre since it assumes that 'arc_buf_t::b_data' must be page aligned when zio buffers are greater than or equal to PAGESIZE. This patch will make the zio buffers to be PAGESIZE aligned when the sizes are not less than PAGESIZE. This change may cause a little bit memory waste but that should be fine because after ABD is introduced, zio buffers are used to hold data temporarily and live in memory for a short while. Reviewed-by: Don Brady <don.brady@intel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Jinshan Xiong <jinshan.xiong@gmail.com> Signed-off-by: Jinshan Xiong <jinshan.xiong@intel.com> Closes openzfs#6084
tonyhutter
pushed a commit
that referenced
this pull request
Jun 9, 2017
In current implementation, only zio buffers in 16KB and bigger are guaranteed PAGESIZE alignment. This breaks Lustre since it assumes that 'arc_buf_t::b_data' must be page aligned when zio buffers are greater than or equal to PAGESIZE. This patch will make the zio buffers to be PAGESIZE aligned when the sizes are not less than PAGESIZE. This change may cause a little bit memory waste but that should be fine because after ABD is introduced, zio buffers are used to hold data temporarily and live in memory for a short while. Reviewed-by: Don Brady <don.brady@intel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Jinshan Xiong <jinshan.xiong@gmail.com> Signed-off-by: Jinshan Xiong <jinshan.xiong@intel.com> Closes #6084
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In current implementation, only zio buffers in 16KB and bigger are
guaranteed PAGESIZE alignment. This breaks Lustre since it assumes
that 'arc_buf_t::b_data' must be page aligned when zio buffers are
greater than or equal to PAGESIZE.
This patch will make the zio buffers to be PAGESIZE aligned when
the sizes are not less than PAGESIZE.
This change may cause a little bit memory waste but that should be
fine because after ABD is introduced, zio buffers are used to hold
data temporarily and live in memory for a short while.
Signed-off-by: Jinshan Xiong jinshan.xiong@gmail.com
Signed-off-by: Jinshan Xiong jinshan.xiong@intel.com
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.