vdev_disk: try harder to ensure IO alignment rules #16687

robn · 2024-10-25T11:21:10Z

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

Ongoing IO alignment issues on Linux with dm-crypt.

Closes: #16631, #15646, #15533, #14533.
Maybe also: #10094

Description

The key insight from out of #16631 is that blocks data must not cross physical page boundaries. So, this updates the alignment check function, then changes the fallback allocation method to ensure page alignment.

I'm not very happy with abd_alloc_scatter(). It feels like a big call to just create it and assert that its always page-aligned. But it's probaby right, and the ABD interface as a whole is kinda of a mess these days, and there really isn't an easier allocator to reach for that will do page alignment (even using alloc_pages() needs a bunch of ceremony that abd_alloc_chunks() already does. So it doesn't seem too bad.

Update: @amotin educated me on why the existing abd_alloc_for_io() was actually what I wanted. So this has been changed for that, and abd_alloc_scatter() no longer included.

I did notice through this that we don't honour the "minimum IO" device limit at all. Most things don't care, but as usual, dm-crypt does, because the block size is part of the crypto. So creating an ashift=9 pool over a 4K dm-crypt device just throws IO errors. I think it always did though (I can't see how it couldn't have), so I'm not going to worry too much for now. It does show that we need to work to minimum limits, which will mean eg not doing <4K accesses, but that seems like it's gonna be work all over the stack (labels, gang headers, etc)... some other time!

Update: Turns out this was a long-standing bug with ZFS, in that it should never be possible to use an ashift smaller than LBS. So now we do check for this case, and #16690 will make it impossible to get into that situation.

How Has This Been Tested?

The test case in #16631 doesn't trip the bug anymore. If I set zfs_vdev_disk_classic=1 it blows up pretty quick. My test cases from #15588 (forcing ganging) continue to work correctly.

Full ZTS run in progress, but it's all looking pretty good so far.

Update: ZTS ran to completion.

Update: I've now run the #16631 test case with various combinations of --sector-size= to cryptsetup luksCreate and -o ashift= to zpool create (always with ashift larger than sector size). Nothing tripped.

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:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

tonyhutter

A lot of this is over my head, but I don't see any surface level issues. I also tested it with e5d1f68 and it passed. It may need to be re-based though.

module/os/linux/zfs/vdev_disk.c

amotin

Looks good to me except the TODO comment.

It seems out our notion of "properly" aligned IO was incomplete. In particular, dm-crypt does its own splitting, and assumes that a logical block will never cross an order-0 page boundary (ie, the physical page size, not compound size). This effectively means that it needs to be possible to split a BIO at any page or block size boundary and have it work correctly. This updates the alignment check function to enforce these rules (to the extent possible). Our response to misaligned data is to make some new allocation that is properly aligned, and copy the data into it. It turns out that linearising (via abd_borrow_buf()) is not enough, because we allocate eg 4K blocks from a general purpose slab, and so may receive (or already have) a 4K block that crosses pages. So instead, we allocate a new ABD, which is guaranteed to be aligned properly to block sizes, and then copy everything into it, and back out on the way back. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>

Now that we can handle these different alignments, we don't this workaround. This reverts commit aefc2da. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>

robn · 2024-10-27T02:41:36Z

Last push removed the TODO comment, added a check to ensure that the data len within the page is a multiple of LBS, and adjusted the page_alignment test to match.

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #16687

Now that we can handle these different alignments, we don't this workaround. This reverts commit aefc2da. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes #16687

allanjude

Reviewed-by: Allan Jude <[email protected]>

It seems out our notion of "properly" aligned IO was incomplete. In particular, dm-crypt does its own splitting, and assumes that a logical block will never cross an order-0 page boundary (ie, the physical page size, not compound size). This effectively means that it needs to be possible to split a BIO at any page or block size boundary and have it work correctly. This updates the alignment check function to enforce these rules (to the extent possible). Our response to misaligned data is to make some new allocation that is properly aligned, and copy the data into it. It turns out that linearising (via abd_borrow_buf()) is not enough, because we allocate eg 4K blocks from a general purpose slab, and so may receive (or already have) a 4K block that crosses pages. So instead, we allocate a new ABD, which is guaranteed to be aligned properly to block sizes, and then copy everything into it, and back out on the way back. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687 openzfs#16631 openzfs#15646 openzfs#15533 openzfs#14533

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687

It seems out our notion of "properly" aligned IO was incomplete. In particular, dm-crypt does its own splitting, and assumes that a logical block will never cross an order-0 page boundary (ie, the physical page size, not compound size). This effectively means that it needs to be possible to split a BIO at any page or block size boundary and have it work correctly. This updates the alignment check function to enforce these rules (to the extent possible). Our response to misaligned data is to make some new allocation that is properly aligned, and copy the data into it. It turns out that linearising (via abd_borrow_buf()) is not enough, because we allocate eg 4K blocks from a general purpose slab, and so may receive (or already have) a 4K block that crosses pages. So instead, we allocate a new ABD, which is guaranteed to be aligned properly to block sizes, and then copy everything into it, and back out on the way back. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687 openzfs#16631 openzfs#15646 openzfs#15533 openzfs#14533 (cherry picked from commit 63bafe6)

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687 (cherry picked from commit e7425ae)

It seems out our notion of "properly" aligned IO was incomplete. In particular, dm-crypt does its own splitting, and assumes that a logical block will never cross an order-0 page boundary (ie, the physical page size, not compound size). This effectively means that it needs to be possible to split a BIO at any page or block size boundary and have it work correctly. This updates the alignment check function to enforce these rules (to the extent possible). Our response to misaligned data is to make some new allocation that is properly aligned, and copy the data into it. It turns out that linearising (via abd_borrow_buf()) is not enough, because we allocate eg 4K blocks from a general purpose slab, and so may receive (or already have) a 4K block that crosses pages. So instead, we allocate a new ABD, which is guaranteed to be aligned properly to block sizes, and then copy everything into it, and back out on the way back. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687 openzfs#16631 openzfs#15646 openzfs#15533 openzfs#14533

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687

It seems out our notion of "properly" aligned IO was incomplete. In particular, dm-crypt does its own splitting, and assumes that a logical block will never cross an order-0 page boundary (ie, the physical page size, not compound size). This effectively means that it needs to be possible to split a BIO at any page or block size boundary and have it work correctly. This updates the alignment check function to enforce these rules (to the extent possible). Our response to misaligned data is to make some new allocation that is properly aligned, and copy the data into it. It turns out that linearising (via abd_borrow_buf()) is not enough, because we allocate eg 4K blocks from a general purpose slab, and so may receive (or already have) a 4K block that crosses pages. So instead, we allocate a new ABD, which is guaranteed to be aligned properly to block sizes, and then copy everything into it, and back out on the way back. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687 openzfs#16631 openzfs#15646 openzfs#15533 openzfs#14533 (cherry picked from commit 63bafe6)

Freeing an ABD can take sleeping locks to update various stats. We aren't allowed to sleep on an interrupt handler. So, move the free off to the io_done callback. We should never have been freeing things in the interrupt handler, but we got away with it because we were usually freeing a linear ABD, which at most is returning two objects to a cache and never sleeping. Scatter ABDs can be used now, and those have more complex locking. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Rob Norris <[email protected]> Closes openzfs#16687

robn changed the title ~~Vdev disk alignment~~ vdev_disk: try harder to ensure IO alignment rules Oct 25, 2024

robn requested review from amotin and tonyhutter October 25, 2024 11:28

tonyhutter approved these changes Oct 25, 2024

View reviewed changes

amotin reviewed Oct 25, 2024

View reviewed changes

module/os/linux/zfs/vdev_disk.c Outdated Show resolved Hide resolved

module/os/linux/zfs/vdev_disk.c Outdated Show resolved Hide resolved

robn force-pushed the vdev-disk-alignment branch 2 times, most recently from 625e093 to e37282f Compare October 26, 2024 09:46

amotin approved these changes Oct 26, 2024

View reviewed changes

robn added 3 commits October 27, 2024 13:31

Revert "Workaround issue of Linux vdev_disk.c, (openzfs#16678)"

5621612

Now that we can handle these different alignments, we don't this workaround. This reverts commit aefc2da. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: Rob Norris <[email protected]>

robn force-pushed the vdev-disk-alignment branch from e37282f to 5621612 Compare October 27, 2024 02:31

amotin approved these changes Oct 27, 2024

View reviewed changes

amotin added the Status: Code Review Needed Ready for review and testing label Oct 29, 2024

behlendorf approved these changes Oct 31, 2024

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 31, 2024

behlendorf closed this in 63bafe6 Nov 1, 2024

allanjude reviewed Nov 1, 2024

View reviewed changes

robn mentioned this pull request Nov 5, 2024

zfs-2.2.7 patchset #16720

Merged

13 tasks

robn mentioned this pull request Nov 25, 2024

CKSUM and WRITE errors when receiving snapshots or scrubbing (2.2.4, LUKS) #15646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vdev_disk: try harder to ensure IO alignment rules #16687

vdev_disk: try harder to ensure IO alignment rules #16687

robn commented Oct 25, 2024 •

edited

Loading

tonyhutter left a comment

amotin left a comment

robn commented Oct 27, 2024

allanjude left a comment

vdev_disk: try harder to ensure IO alignment rules #16687

vdev_disk: try harder to ensure IO alignment rules #16687

Conversation

robn commented Oct 25, 2024 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

tonyhutter left a comment

Choose a reason for hiding this comment

amotin left a comment

Choose a reason for hiding this comment

robn commented Oct 27, 2024

allanjude left a comment

Choose a reason for hiding this comment

robn commented Oct 25, 2024 •

edited

Loading