* fix stacking of sd-imposed max_sectors
@ 2024-05-23 18:26 Christoph Hellwig
2024-05-23 18:26 ` [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Christoph Hellwig @ 2024-05-23 18:26 UTC (permalink / raw
To: Jens Axboe; +Cc: Martin K. Petersen, Mike Snitzer, linux-block, linux-scsi
Hi Jens,
this series fixes a regression in 6.10-rc report by Mike when using
device mapper multipath on top of SCSI disks. It is due the sd
driver not only setting the hard max_dev_sectors limit but also
pre-setting a potentially lower max_sectors value.
Diffstat:
block/blk-settings.c | 2 ++
drivers/scsi/sd.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors
2024-05-23 18:26 fix stacking of sd-imposed max_sectors Christoph Hellwig
@ 2024-05-23 18:26 ` Christoph Hellwig
2024-05-23 18:53 ` Martin K. Petersen
2024-05-23 18:26 ` [PATCH 2/2] block: stack max_user_sectors Christoph Hellwig
2024-05-28 12:57 ` fix stacking of sd-imposed max_sectors Jens Axboe
2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2024-05-23 18:26 UTC (permalink / raw
To: Jens Axboe; +Cc: Martin K. Petersen, Mike Snitzer, linux-block, linux-scsi
sd can set a max_sectors value that is lower than the max_hw_sectors
limit based on the block limits VPD page. While this is rather unusual,
it used to work until the max_user_sectors field was split out to cleanly
deal with conflicting hardware and user limits when the hardware limit
changes. Also set max_user_sectors to ensure the limit can properly be
stacked.
Fixes: 4f563a64732d ("block: add a max_user_discard_sectors queue limit")
Reported-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Mike Snitzer <snitzer@kernel.org>
---
drivers/scsi/sd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 332eb9dac22d91..f6c822c9cbd2d3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3700,8 +3700,10 @@ static int sd_revalidate_disk(struct gendisk *disk)
*/
if (sdkp->first_scan ||
q->limits.max_sectors > q->limits.max_dev_sectors ||
- q->limits.max_sectors > q->limits.max_hw_sectors)
+ q->limits.max_sectors > q->limits.max_hw_sectors) {
q->limits.max_sectors = rw_max;
+ q->limits.max_user_sectors = rw_max;
+ }
sdkp->first_scan = 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] block: stack max_user_sectors
2024-05-23 18:26 fix stacking of sd-imposed max_sectors Christoph Hellwig
2024-05-23 18:26 ` [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors Christoph Hellwig
@ 2024-05-23 18:26 ` Christoph Hellwig
2024-05-23 18:54 ` Martin K. Petersen
2024-05-28 12:57 ` fix stacking of sd-imposed max_sectors Jens Axboe
2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2024-05-23 18:26 UTC (permalink / raw
To: Jens Axboe; +Cc: Martin K. Petersen, Mike Snitzer, linux-block, linux-scsi
The max_user_sectors is one of the three factors determining the actual
max_sectors limit for READ/WRITE requests. Because of that it needs to
be stacked at least for the device mapper multi-path case where requests
are directly inserted on the lower device. For SCSI disks this is
important because the sd driver actually sets it's own advisory limit
that is lower than max_hw_sectors based on the block limits VPD page.
While this is a bit odd an unusual, the same effect can happen if a
user or udev script tweaks the value manually.
Fixes: 4f563a64732d ("block: add a max_user_discard_sectors queue limit")
Reported-by: Mike Snitzer <snitzer@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Mike Snitzer <snitzer@kernel.org>
---
block/blk-settings.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index a7fe8e90240a6e..7a672021daee6a 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -611,6 +611,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
unsigned int top, bottom, alignment, ret = 0;
t->max_sectors = min_not_zero(t->max_sectors, b->max_sectors);
+ t->max_user_sectors = min_not_zero(t->max_user_sectors,
+ b->max_user_sectors);
t->max_hw_sectors = min_not_zero(t->max_hw_sectors, b->max_hw_sectors);
t->max_dev_sectors = min_not_zero(t->max_dev_sectors, b->max_dev_sectors);
t->max_write_zeroes_sectors = min(t->max_write_zeroes_sectors,
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors
2024-05-23 18:26 ` [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors Christoph Hellwig
@ 2024-05-23 18:53 ` Martin K. Petersen
2024-05-24 7:51 ` Christoph Hellwig
0 siblings, 1 reply; 7+ messages in thread
From: Martin K. Petersen @ 2024-05-23 18:53 UTC (permalink / raw
To: Christoph Hellwig
Cc: Jens Axboe, Martin K. Petersen, Mike Snitzer, linux-block,
linux-scsi
Christoph,
> sd can set a max_sectors value that is lower than the max_hw_sectors
> limit based on the block limits VPD page. While this is rather
> unusual,
It's not particularly unusual. Virtually all arrays have a much smaller
stripe or cache line size than what the average HBA can handle in one
transfer. Using the device's preferred I/O size to configure max_sectors
made a substantial difference performance-wise.
> it used to work until the max_user_sectors field was split out to
> cleanly deal with conflicting hardware and user limits when the
> hardware limit changes. Also set max_user_sectors to ensure the limit
> can properly be stacked.
Fine for a quick fix.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] block: stack max_user_sectors
2024-05-23 18:26 ` [PATCH 2/2] block: stack max_user_sectors Christoph Hellwig
@ 2024-05-23 18:54 ` Martin K. Petersen
0 siblings, 0 replies; 7+ messages in thread
From: Martin K. Petersen @ 2024-05-23 18:54 UTC (permalink / raw
To: Christoph Hellwig
Cc: Jens Axboe, Martin K. Petersen, Mike Snitzer, linux-block,
linux-scsi
Christoph,
> The max_user_sectors is one of the three factors determining the actual
> max_sectors limit for READ/WRITE requests. Because of that it needs to
> be stacked at least for the device mapper multi-path case where requests
> are directly inserted on the lower device.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors
2024-05-23 18:53 ` Martin K. Petersen
@ 2024-05-24 7:51 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2024-05-24 7:51 UTC (permalink / raw
To: Martin K. Petersen
Cc: Christoph Hellwig, Jens Axboe, Mike Snitzer, linux-block,
linux-scsi
On Thu, May 23, 2024 at 02:53:40PM -0400, Martin K. Petersen wrote:
>
> Christoph,
>
> > sd can set a max_sectors value that is lower than the max_hw_sectors
> > limit based on the block limits VPD page. While this is rather
> > unusual,
>
> It's not particularly unusual. Virtually all arrays have a much smaller
> stripe or cache line size than what the average HBA can handle in one
> transfer. Using the device's preferred I/O size to configure max_sectors
> made a substantial difference performance-wise.
Well, in terms of Linux it is weird in that drivers weren't ever supposed
to set max_sectors directly but only provide max_hw_sectors (although
nbd also decreases it and rbd increases it in odd ways). Especially
as we already have an opt_in limit for the optimal size.
I'll find a way to sort it out and build a grand unified and somewhat
coherent theory out of it..
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: fix stacking of sd-imposed max_sectors
2024-05-23 18:26 fix stacking of sd-imposed max_sectors Christoph Hellwig
2024-05-23 18:26 ` [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors Christoph Hellwig
2024-05-23 18:26 ` [PATCH 2/2] block: stack max_user_sectors Christoph Hellwig
@ 2024-05-28 12:57 ` Jens Axboe
2 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2024-05-28 12:57 UTC (permalink / raw
To: Christoph Hellwig
Cc: Martin K. Petersen, Mike Snitzer, linux-block, linux-scsi
On Thu, 23 May 2024 20:26:12 +0200, Christoph Hellwig wrote:
> this series fixes a regression in 6.10-rc report by Mike when using
> device mapper multipath on top of SCSI disks. It is due the sd
> driver not only setting the hard max_dev_sectors limit but also
> pre-setting a potentially lower max_sectors value.
>
> Diffstat:
> block/blk-settings.c | 2 ++
> drivers/scsi/sd.c | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> [...]
Applied, thanks!
[1/2] sd: also set max_user_sectors when setting max_sectors
commit: bafea1c58b24be594d97841ced1b7ae0347bf6e3
[2/2] block: stack max_user_sectors
commit: e528bede6f4e6822afdf0fa80be46ea9199f0911
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-05-28 12:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-23 18:26 fix stacking of sd-imposed max_sectors Christoph Hellwig
2024-05-23 18:26 ` [PATCH 1/2] sd: also set max_user_sectors when setting max_sectors Christoph Hellwig
2024-05-23 18:53 ` Martin K. Petersen
2024-05-24 7:51 ` Christoph Hellwig
2024-05-23 18:26 ` [PATCH 2/2] block: stack max_user_sectors Christoph Hellwig
2024-05-23 18:54 ` Martin K. Petersen
2024-05-28 12:57 ` fix stacking of sd-imposed max_sectors Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).