| Message ID | 20221103152559.1909328-1-kbusch@meta.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix direct io errors on dm-crypt | expand |
Hi The patchset seems OK - but dm-integrity also has a limitation that the bio vectors must be aligned on logical block size. dm-writecache and dm-verity seem to handle unaligned bioset, but you should check them anyway. I'm not sure about dm-log-writes. Mikulas On Thu, 3 Nov 2022, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > The 6.0 kernel made some changes to the direct io interface to allow > offsets in user addresses. This based on the hardware's capabilities > reported in the request_queue's dma_alignment attribute. > > dm-crypt requires direct io be aligned to the block size. Since it was > only ever using the default 511 dma mask, this requirement may fail if > formatted to something larger, like 4k, which will result in unexpected > behavior with direct-io. > > There are two parts to fixing this: > > First, the attribute needs to be moved to the queue_limit so that it > can properly stack with device mappers. > > Second, dm-crypt provides its minimum required limit to match the > logical block size. > > Keith Busch (3): > block: make dma_alignment a stacking queue_limit > dm-crypt: provide dma_alignment limit in io_hints > block: make blk_set_default_limits() private > > block/blk-core.c | 1 - > block/blk-settings.c | 9 +++++---- > block/blk.h | 1 + > drivers/md/dm-crypt.c | 1 + > include/linux/blkdev.h | 16 ++++++++-------- > 5 files changed, 15 insertions(+), 13 deletions(-) > > -- > 2.30.2 >
On Thu, 3 Nov 2022 08:25:56 -0700 Keith Busch <kbusch@meta.com> wrote: > From: Keith Busch <kbusch@kernel.org> > > The 6.0 kernel made some changes to the direct io interface to allow > offsets in user addresses. This based on the hardware's capabilities > reported in the request_queue's dma_alignment attribute. > > dm-crypt requires direct io be aligned to the block size. Since it was > only ever using the default 511 dma mask, this requirement may fail if > formatted to something larger, like 4k, which will result in > unexpected behavior with direct-io. > > There are two parts to fixing this: > > First, the attribute needs to be moved to the queue_limit so that it > can properly stack with device mappers. > > Second, dm-crypt provides its minimum required limit to match the > logical block size. > > Keith Busch (3): > block: make dma_alignment a stacking queue_limit > dm-crypt: provide dma_alignment limit in io_hints > block: make blk_set_default_limits() private > > block/blk-core.c | 1 - > block/blk-settings.c | 9 +++++---- > block/blk.h | 1 + > drivers/md/dm-crypt.c | 1 + > include/linux/blkdev.h | 16 ++++++++-------- > 5 files changed, 15 insertions(+), 13 deletions(-) > Applied on top 6.1-rc3, after that issue[1] doesn't reproduce. [1] https://lore.kernel.org/linux-block/20221101001558.648ee024@xps.demsh.org/
On Thu, Nov 03, 2022 at 12:33:19PM -0400, Mikulas Patocka wrote: > Hi > > The patchset seems OK - but dm-integrity also has a limitation that the > bio vectors must be aligned on logical block size. > > dm-writecache and dm-verity seem to handle unaligned bioset, but you > should check them anyway. > > I'm not sure about dm-log-writes. Yeah, dm-integrity definitly needs it too. This is easy enough to use the same io_hint that I added for dm-crypt. dm-log-writes is doing some weird things with writes that I don't think would work with some low level drivers without the same alignment constraint. The other two appear to handle this fine, but I'll run everything through some focused test cases to be sure. In the meantime, do you want to see the remaining mappers fixed in a v2, or are you okay if they follow after this series?
On Thu, 3 Nov 2022, Keith Busch wrote: > On Thu, Nov 03, 2022 at 12:33:19PM -0400, Mikulas Patocka wrote: > > Hi > > > > The patchset seems OK - but dm-integrity also has a limitation that the > > bio vectors must be aligned on logical block size. > > > > dm-writecache and dm-verity seem to handle unaligned bioset, but you > > should check them anyway. > > > > I'm not sure about dm-log-writes. > > Yeah, dm-integrity definitly needs it too. This is easy enough to use > the same io_hint that I added for dm-crypt. > > dm-log-writes is doing some weird things with writes that I don't think > would work with some low level drivers without the same alignment > constraint. > > The other two appear to handle this fine, but I'll run everything > through some focused test cases to be sure. > > In the meantime, do you want to see the remaining mappers fixed in a v2, > or are you okay if they follow after this series? I don't care if you make a separate patch or fold it into an existing patch. Mikulas
On Thu, Nov 03, 2022 at 08:25:56AM -0700, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > The 6.0 kernel made some changes to the direct io interface to allow > offsets in user addresses. This based on the hardware's capabilities > reported in the request_queue's dma_alignment attribute. > > dm-crypt requires direct io be aligned to the block size. Since it was > only ever using the default 511 dma mask, this requirement may fail if > formatted to something larger, like 4k, which will result in unexpected > behavior with direct-io. > > There are two parts to fixing this: > > First, the attribute needs to be moved to the queue_limit so that it > can properly stack with device mappers. > > Second, dm-crypt provides its minimum required limit to match the > logical block size. > > Keith Busch (3): > block: make dma_alignment a stacking queue_limit > dm-crypt: provide dma_alignment limit in io_hints > block: make blk_set_default_limits() private Hi Keith, can you send out an updated version of this patch series that addresses the feedback? I'd really like for this bug to be fixed before 6.1 is released, so that there isn't a known bug in STATX_DIOALIGN already upon release. Thanks! - Eric
On Thu, Nov 10, 2022 at 06:24:03PM +0000, Eric Biggers wrote: > On Thu, Nov 03, 2022 at 08:25:56AM -0700, Keith Busch wrote: > > From: Keith Busch <kbusch@kernel.org> > > > > The 6.0 kernel made some changes to the direct io interface to allow > > offsets in user addresses. This based on the hardware's capabilities > > reported in the request_queue's dma_alignment attribute. > > > > dm-crypt requires direct io be aligned to the block size. Since it was > > only ever using the default 511 dma mask, this requirement may fail if > > formatted to something larger, like 4k, which will result in unexpected > > behavior with direct-io. > > > > There are two parts to fixing this: > > > > First, the attribute needs to be moved to the queue_limit so that it > > can properly stack with device mappers. > > > > Second, dm-crypt provides its minimum required limit to match the > > logical block size. > > > > Keith Busch (3): > > block: make dma_alignment a stacking queue_limit > > dm-crypt: provide dma_alignment limit in io_hints > > block: make blk_set_default_limits() private > > Hi Keith, can you send out an updated version of this patch series that > addresses the feedback? > > I'd really like for this bug to be fixed before 6.1 is released, so that there > isn't a known bug in STATX_DIOALIGN already upon release. Sorry for the delay, v2 sent.
From: Keith Busch <kbusch@kernel.org> The 6.0 kernel made some changes to the direct io interface to allow offsets in user addresses. This based on the hardware's capabilities reported in the request_queue's dma_alignment attribute. dm-crypt requires direct io be aligned to the block size. Since it was only ever using the default 511 dma mask, this requirement may fail if formatted to something larger, like 4k, which will result in unexpected behavior with direct-io. There are two parts to fixing this: First, the attribute needs to be moved to the queue_limit so that it can properly stack with device mappers. Second, dm-crypt provides its minimum required limit to match the logical block size. Keith Busch (3): block: make dma_alignment a stacking queue_limit dm-crypt: provide dma_alignment limit in io_hints block: make blk_set_default_limits() private block/blk-core.c | 1 - block/blk-settings.c | 9 +++++---- block/blk.h | 1 + drivers/md/dm-crypt.c | 1 + include/linux/blkdev.h | 16 ++++++++-------- 5 files changed, 15 insertions(+), 13 deletions(-)