Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize newly allocated buffer #11687

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

behlendorf
Copy link
Contributor

Description

When allocating a buffer using zio_buf_alloc() or an ABD with
abd_alloc_for_io() ensure the returned memory is initialized
to zero.

How Has This Been Tested?

Manual verification and a test run of the ZTS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 3, 2021
module/zfs/zio.c Outdated Show resolved Hide resolved
@ahrens
Copy link
Member

ahrens commented Mar 3, 2021

When allocating a buffer using zio_buf_alloc(), or an ABD with abd_alloc_for_io(), ensure the returned memory is initialized to zero.

Reading this, it sounds like you're saying that zio_buf_alloc/abd_alloc_for_io should return zeroed buffers, which could have a substantial performance impact. However, the code changes relate specifically to handling encryption, where perhaps performance isn't a concern?

I see that this is marked as "code cleanup", not "bug fix". I presume that spa_do_crypt_abd() completely fills the target abd, so the current code (without zeroing) is correct. How does zeroing these buffers make the code more clear?

When populating a ZIL destination buffer ensure it is always
zeroed before its contents are constructed.

Reviewed-By: Tom Caputi <caputit1@tcnj.edu>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf
Copy link
Contributor Author

I've gone ahead and moved the initialization down in to zio_crypt_init_uios_zil() so it's only applied where it's needed.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 5, 2021
@behlendorf behlendorf merged commit 6bbb44e into openzfs:master Mar 5, 2021
ckane pushed a commit to ckane/zfs that referenced this pull request Mar 7, 2021
When populating a ZIL destination buffer ensure it is always
zeroed before its contents are constructed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tom Caputi <caputit1@tcnj.edu>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11687
behlendorf added a commit that referenced this pull request Mar 8, 2021
When populating a ZIL destination buffer ensure it is always
zeroed before its contents are constructed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tom Caputi <caputit1@tcnj.edu>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #11687
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
When populating a ZIL destination buffer ensure it is always
zeroed before its contents are constructed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tom Caputi <caputit1@tcnj.edu>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11687
@behlendorf behlendorf deleted the initialize-buffers branch April 19, 2021 19:52
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
When populating a ZIL destination buffer ensure it is always
zeroed before its contents are constructed.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tom Caputi <caputit1@tcnj.edu>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#11687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants