| Message ID | 20190521070143.22631-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] block: don't decrement nr_phys_segments for physically contigous segments | expand |
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);
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.
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);
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(+)