[PATCH 0/3] fix direct io errors on dm-crypt

[PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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

[dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/3] block: make dma_alignment a stacking queue_limit

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Device mappers had always been getting the default 511 dma mask, but
the underlying device might have a larger alignment requirement. Since
this value is used to determine alloweable direct-io alignment, this
needs to be a stackable limit.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-core.c       |  1 -
 block/blk-settings.c   |  8 +++++---
 include/linux/blkdev.h | 15 ++++++++-------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3a2ed8dadf73..9d6a947024ea 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -418,7 +418,6 @@ struct request_queue *blk_alloc_queue(int node_id)
 				PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
 		goto fail_stats;
 
-	blk_queue_dma_alignment(q, 511);
 	blk_set_default_limits(&q->limits);
 	q->nr_requests = BLKDEV_DEFAULT_RQ;
 
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8bb9eef5310e..4949ed3ce7c9 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -57,6 +57,7 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->misaligned = 0;
 	lim->zoned = BLK_ZONED_NONE;
 	lim->zone_write_granularity = 0;
+	lim->dma_alignment = 511;
 }
 EXPORT_SYMBOL(blk_set_default_limits);
 
@@ -600,6 +601,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 
 	t->io_min = max(t->io_min, b->io_min);
 	t->io_opt = lcm_not_zero(t->io_opt, b->io_opt);
+	t->dma_alignment = max(t->dma_alignment, b->dma_alignment);
 
 	/* Set non-power-of-2 compatible chunk_sectors boundary */
 	if (b->chunk_sectors)
@@ -773,7 +775,7 @@ EXPORT_SYMBOL(blk_queue_virt_boundary);
  **/
 void blk_queue_dma_alignment(struct request_queue *q, int mask)
 {
-	q->dma_alignment = mask;
+	q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_dma_alignment);
 
@@ -795,8 +797,8 @@ void blk_queue_update_dma_alignment(struct request_queue *q, int mask)
 {
 	BUG_ON(mask > PAGE_SIZE);
 
-	if (mask > q->dma_alignment)
-		q->dma_alignment = mask;
+	if (mask > q->limits.dma_alignment)
+		q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_update_dma_alignment);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 854b4745cdd1..69ee5ea29e2f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -310,6 +310,13 @@ struct queue_limits {
 	unsigned char		discard_misaligned;
 	unsigned char		raid_partial_stripes_expensive;
 	enum blk_zoned_model	zoned;
+
+	/*
+	 * Drivers that set dma_alignment to less than 511 must be prepared to
+	 * handle individual bvec's that are not a multiple of a SECTOR_SIZE
+	 * due to possible offsets.
+	 */
+	unsigned int		dma_alignment;
 };
 
 typedef int (*report_zones_cb)(struct blk_zone *zone, unsigned int idx,
@@ -455,12 +462,6 @@ struct request_queue {
 	unsigned long		nr_requests;	/* Max # of requests */
 
 	unsigned int		dma_pad_mask;
-	/*
-	 * Drivers that set dma_alignment to less than 511 must be prepared to
-	 * handle individual bvec's that are not a multiple of a SECTOR_SIZE
-	 * due to possible offsets.
-	 */
-	unsigned int		dma_alignment;
 
 #ifdef CONFIG_BLK_INLINE_ENCRYPTION
 	struct blk_crypto_profile *crypto_profile;
@@ -1318,7 +1319,7 @@ static inline sector_t bdev_zone_sectors(struct block_device *bdev)
 
 static inline int queue_dma_alignment(const struct request_queue *q)
 {
-	return q ? q->dma_alignment : 511;
+	return q ? q->limits.dma_alignment : 511;
 }
 
 static inline unsigned int bdev_dma_alignment(struct block_device *bdev)
-- 
2.30.2

[dm-devel] [PATCH 1/3] block: make dma_alignment a stacking queue_limit

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Device mappers had always been getting the default 511 dma mask, but
the underlying device might have a larger alignment requirement. Since
this value is used to determine alloweable direct-io alignment, this
needs to be a stackable limit.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-core.c       |  1 -
 block/blk-settings.c   |  8 +++++---
 include/linux/blkdev.h | 15 ++++++++-------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3a2ed8dadf73..9d6a947024ea 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -418,7 +418,6 @@ struct request_queue *blk_alloc_queue(int node_id)
 				PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
 		goto fail_stats;
 
-	blk_queue_dma_alignment(q, 511);
 	blk_set_default_limits(&q->limits);
 	q->nr_requests = BLKDEV_DEFAULT_RQ;
 
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8bb9eef5310e..4949ed3ce7c9 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -57,6 +57,7 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->misaligned = 0;
 	lim->zoned = BLK_ZONED_NONE;
 	lim->zone_write_granularity = 0;
+	lim->dma_alignment = 511;
 }
 EXPORT_SYMBOL(blk_set_default_limits);
 
@@ -600,6 +601,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 
 	t->io_min = max(t->io_min, b->io_min);
 	t->io_opt = lcm_not_zero(t->io_opt, b->io_opt);
+	t->dma_alignment = max(t->dma_alignment, b->dma_alignment);
 
 	/* Set non-power-of-2 compatible chunk_sectors boundary */
 	if (b->chunk_sectors)
@@ -773,7 +775,7 @@ EXPORT_SYMBOL(blk_queue_virt_boundary);
  **/
 void blk_queue_dma_alignment(struct request_queue *q, int mask)
 {
-	q->dma_alignment = mask;
+	q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_dma_alignment);
 
@@ -795,8 +797,8 @@ void blk_queue_update_dma_alignment(struct request_queue *q, int mask)
 {
 	BUG_ON(mask > PAGE_SIZE);
 
-	if (mask > q->dma_alignment)
-		q->dma_alignment = mask;
+	if (mask > q->limits.dma_alignment)
+		q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_update_dma_alignment);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 854b4745cdd1..69ee5ea29e2f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -310,6 +310,13 @@ struct queue_limits {
 	unsigned char		discard_misaligned;
 	unsigned char		raid_partial_stripes_expensive;
 	enum blk_zoned_model	zoned;
+
+	/*
+	 * Drivers that set dma_alignment to less than 511 must be prepared to
+	 * handle individual bvec's that are not a multiple of a SECTOR_SIZE
+	 * due to possible offsets.
+	 */
+	unsigned int		dma_alignment;
 };
 
 typedef int (*report_zones_cb)(struct blk_zone *zone, unsigned int idx,
@@ -455,12 +462,6 @@ struct request_queue {
 	unsigned long		nr_requests;	/* Max # of requests */
 
 	unsigned int		dma_pad_mask;
-	/*
-	 * Drivers that set dma_alignment to less than 511 must be prepared to
-	 * handle individual bvec's that are not a multiple of a SECTOR_SIZE
-	 * due to possible offsets.
-	 */
-	unsigned int		dma_alignment;
 
 #ifdef CONFIG_BLK_INLINE_ENCRYPTION
 	struct blk_crypto_profile *crypto_profile;
@@ -1318,7 +1319,7 @@ static inline sector_t bdev_zone_sectors(struct block_device *bdev)
 
 static inline int queue_dma_alignment(const struct request_queue *q)
 {
-	return q ? q->dma_alignment : 511;
+	return q ? q->limits.dma_alignment : 511;
 }
 
 static inline unsigned int bdev_dma_alignment(struct block_device *bdev)
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/3] block: make dma_alignment a stacking queue_limit

[Christoph Hellwig]

On Thu, Nov 03, 2022 at 08:25:57AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Device mappers had always been getting the default 511 dma mask, but
> the underlying device might have a larger alignment requirement. Since
> this value is used to determine alloweable direct-io alignment, this
> needs to be a stackable limit.

This can also remove the just added blk_queue_dma_alignment in
nvme_mpath_alloc_disk again as it is right next to the
blk_set_stacking_limits call.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 1/3] block: make dma_alignment a stacking queue_limit

[Christoph Hellwig]

On Thu, Nov 03, 2022 at 08:25:57AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Device mappers had always been getting the default 511 dma mask, but
> the underlying device might have a larger alignment requirement. Since
> this value is used to determine alloweable direct-io alignment, this
> needs to be a stackable limit.

This can also remove the just added blk_queue_dma_alignment in
nvme_mpath_alloc_disk again as it is right next to the
blk_set_stacking_limits call.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 2/3] dm-crypt: provide dma_alignment limit in io_hints

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This device mapper needs bio vectors to be sized and memory aligned to
the logical block size. Set the minimum required queue limit
accordingly.

Fixes: b1a000d3b8ec5 ("block: relax direct io memory alignment")
Reportred-by: Eric Biggers <ebiggers@kernel.org>
Reported-by: Dmitrii Tcvetkov <me@demsh.org>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/md/dm-crypt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 159c6806c19b..2653516bcdef 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -3630,6 +3630,7 @@ static void crypt_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->physical_block_size =
 		max_t(unsigned, limits->physical_block_size, cc->sector_size);
 	limits->io_min = max_t(unsigned, limits->io_min, cc->sector_size);
+	limits->dma_alignment = limits->logical_block_size - 1;
 }
 
 static struct target_type crypt_target = {
-- 
2.30.2

[dm-devel] [PATCH 2/3] dm-crypt: provide dma_alignment limit in io_hints

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This device mapper needs bio vectors to be sized and memory aligned to
the logical block size. Set the minimum required queue limit
accordingly.

Fixes: b1a000d3b8ec5 ("block: relax direct io memory alignment")
Reportred-by: Eric Biggers <ebiggers@kernel.org>
Reported-by: Dmitrii Tcvetkov <me@demsh.org>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/md/dm-crypt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 159c6806c19b..2653516bcdef 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -3630,6 +3630,7 @@ static void crypt_io_hints(struct dm_target *ti, struct queue_limits *limits)
 	limits->physical_block_size =
 		max_t(unsigned, limits->physical_block_size, cc->sector_size);
 	limits->io_min = max_t(unsigned, limits->io_min, cc->sector_size);
+	limits->dma_alignment = limits->logical_block_size - 1;
 }
 
 static struct target_type crypt_target = {
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/3] block: make blk_set_default_limits() private

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There are no external users of this function.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-settings.c   | 1 -
 block/blk.h            | 1 +
 include/linux/blkdev.h | 1 -
 3 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 4949ed3ce7c9..8ac1038d0c79 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -59,7 +59,6 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->zone_write_granularity = 0;
 	lim->dma_alignment = 511;
 }
-EXPORT_SYMBOL(blk_set_default_limits);
 
 /**
  * blk_set_stacking_limits - set default limits for stacking devices
diff --git a/block/blk.h b/block/blk.h
index d259d1da4cb4..4849a2efa4c5 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -330,6 +330,7 @@ void blk_rq_set_mixed_merge(struct request *rq);
 bool blk_rq_merge_ok(struct request *rq, struct bio *bio);
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
 
+void blk_set_default_limits(struct queue_limits *lim);
 int blk_dev_init(void);
 
 /*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 69ee5ea29e2f..704bc175a732 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -939,7 +939,6 @@ extern void blk_queue_io_min(struct request_queue *q, unsigned int min);
 extern void blk_limits_io_opt(struct queue_limits *limits, unsigned int opt);
 extern void blk_queue_io_opt(struct request_queue *q, unsigned int opt);
 extern void blk_set_queue_depth(struct request_queue *q, unsigned int depth);
-extern void blk_set_default_limits(struct queue_limits *lim);
 extern void blk_set_stacking_limits(struct queue_limits *lim);
 extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 			    sector_t offset);
-- 
2.30.2

[dm-devel] [PATCH 3/3] block: make blk_set_default_limits() private

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

There are no external users of this function.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-settings.c   | 1 -
 block/blk.h            | 1 +
 include/linux/blkdev.h | 1 -
 3 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 4949ed3ce7c9..8ac1038d0c79 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -59,7 +59,6 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->zone_write_granularity = 0;
 	lim->dma_alignment = 511;
 }
-EXPORT_SYMBOL(blk_set_default_limits);
 
 /**
  * blk_set_stacking_limits - set default limits for stacking devices
diff --git a/block/blk.h b/block/blk.h
index d259d1da4cb4..4849a2efa4c5 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -330,6 +330,7 @@ void blk_rq_set_mixed_merge(struct request *rq);
 bool blk_rq_merge_ok(struct request *rq, struct bio *bio);
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
 
+void blk_set_default_limits(struct queue_limits *lim);
 int blk_dev_init(void);
 
 /*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 69ee5ea29e2f..704bc175a732 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -939,7 +939,6 @@ extern void blk_queue_io_min(struct request_queue *q, unsigned int min);
 extern void blk_limits_io_opt(struct queue_limits *limits, unsigned int opt);
 extern void blk_queue_io_opt(struct request_queue *q, unsigned int opt);
 extern void blk_set_queue_depth(struct request_queue *q, unsigned int depth);
-extern void blk_set_default_limits(struct queue_limits *lim);
 extern void blk_set_stacking_limits(struct queue_limits *lim);
 extern int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 			    sector_t offset);
-- 
2.30.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/3] block: make blk_set_default_limits() private

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [dm-devel] [PATCH 3/3] block: make blk_set_default_limits() private

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Mikulas Patocka]

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
> 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Mikulas Patocka]

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
> 

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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?

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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?

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Mikulas Patocka]

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
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Mikulas Patocka]

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

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Dmitrii Tcvetkov]

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/

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Dmitrii Tcvetkov]

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/

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Eric Biggers]

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

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Eric Biggers]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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.

Re: [dm-devel] [PATCH 0/3] fix direct io errors on dm-crypt

[Keith Busch]

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.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel