[2/4] block: force an unlimited segment size on queues with a virt boundary
diff mbox series

Message ID 20190521070143.22631-3-hch@lst.de
State New
Headers show
Series
  • [1/4] block: don't decrement nr_phys_segments for physically contigous segments
Related show

Commit Message

Christoph Hellwig May 21, 2019, 7:01 a.m. UTC
We currently fail to update the front/back segment size in the bio when
deciding to allow an otherwise gappy segement to a device with a
virt boundary.  The reason why this did not cause problems is that
devices with a virt boundary fundamentally don't use segments as we
know it and thus don't care.  Make that assumption formal by forcing
an unlimited segement size in this case.

Fixes: f6970f83ef79 ("block: don't check if adjacent bvecs in one bio can be mergeable")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 block/blk-settings.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

James Bottomley July 23, 2019, 3:48 p.m. UTC | #1
On Tue, 2019-05-21 at 09:01 +0200, Christoph Hellwig wrote:
> We currently fail to update the front/back segment size in the bio
> when
> deciding to allow an otherwise gappy segement to a device with a
> virt boundary.  The reason why this did not cause problems is that
> devices with a virt boundary fundamentally don't use segments as we
> know it and thus don't care.  Make that assumption formal by forcing
> an unlimited segement size in this case.
> 
> Fixes: f6970f83ef79 ("block: don't check if adjacent bvecs in one bio
> can be mergeable")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ming Lei <ming.lei@redhat.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> ---
>  block/blk-settings.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 3facc41476be..2ae348c101a0 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -310,6 +310,9 @@ void blk_queue_max_segment_size(struct
> request_queue *q, unsigned int max_size)
>  		       __func__, max_size);
>  	}
>  
> +	/* see blk_queue_virt_boundary() for the explanation */
> +	WARN_ON_ONCE(q->limits.virt_boundary_mask);
> +
>  	q->limits.max_segment_size = max_size;
>  }
>  EXPORT_SYMBOL(blk_queue_max_segment_size);
> @@ -742,6 +745,14 @@ EXPORT_SYMBOL(blk_queue_segment_boundary);
>  void blk_queue_virt_boundary(struct request_queue *q, unsigned long
> mask)
>  {
>  	q->limits.virt_boundary_mask = mask;
> +
> +	/*
> +	 * Devices that require a virtual boundary do not support
> scatter/gather
> +	 * I/O natively, but instead require a descriptor list entry
> for each
> +	 * page (which might not be idential to the Linux
> PAGE_SIZE).  Because
> +	 * of that they are not limited by our notion of "segment
> size".
> +	 */
> +	q->limits.max_segment_size = UINT_MAX;

This needs to be conditional.  If you did set the max segment size, the
block layer should still respect it and not do an override here.  This
seems to be what's causing a regression in SCSI because we set the
max_segment_size first then call the blk_queue_virt_boundary. 
Additionally, if you're not setting the virt boundary (mask == 0) this
should not be set.  I suggest the below.

James

---
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 2ae348c101a0..46a95536f3bd 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -752,7 +752,8 @@ void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
 	 * page (which might not be idential to the Linux PAGE_SIZE).  Because
 	 * of that they are not limited by our notion of "segment size".
 	 */
-	q->limits.max_segment_size = UINT_MAX;
+	if (mask != 0 && q->limits.max_segment_size == BLK_MAX_SEGMENT_SIZE)
+		q->limits.max_segment_size = UINT_MAX;
 }
 EXPORT_SYMBOL(blk_queue_virt_boundary);
Christoph Hellwig July 23, 2019, 4:11 p.m. UTC | #2
On Tue, Jul 23, 2019 at 08:48:52AM -0700, James Bottomley wrote:
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 2ae348c101a0..46a95536f3bd 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -752,7 +752,8 @@ void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
>  	 * page (which might not be idential to the Linux PAGE_SIZE).  Because
>  	 * of that they are not limited by our notion of "segment size".
>  	 */
> -	q->limits.max_segment_size = UINT_MAX;
> +	if (mask != 0 && q->limits.max_segment_size == BLK_MAX_SEGMENT_SIZE)
> +		q->limits.max_segment_size = UINT_MAX;

The first check makes sense, defintively safer than leaving it to the
caller.  The second one is wrong - we need to force an unlimited segment
size because we can't account for it for the virt_boundary merges.  And
the comment just above explains why that is safe.

Patch
diff mbox series

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 3facc41476be..2ae348c101a0 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -310,6 +310,9 @@  void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
 		       __func__, max_size);
 	}
 
+	/* see blk_queue_virt_boundary() for the explanation */
+	WARN_ON_ONCE(q->limits.virt_boundary_mask);
+
 	q->limits.max_segment_size = max_size;
 }
 EXPORT_SYMBOL(blk_queue_max_segment_size);
@@ -742,6 +745,14 @@  EXPORT_SYMBOL(blk_queue_segment_boundary);
 void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
 {
 	q->limits.virt_boundary_mask = mask;
+
+	/*
+	 * Devices that require a virtual boundary do not support scatter/gather
+	 * I/O natively, but instead require a descriptor list entry for each
+	 * page (which might not be idential to the Linux PAGE_SIZE).  Because
+	 * of that they are not limited by our notion of "segment size".
+	 */
+	q->limits.max_segment_size = UINT_MAX;
 }
 EXPORT_SYMBOL(blk_queue_virt_boundary);