[PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Ming Lei]

Set max_segment_size as UINT_MAX explicitly:

- storvrc uses virt_boundary to define `segment`

- strovrc does not define max_segment_size

So define max_segment_size as UINT_MAX, otherwise __blk_rq_map_sg() takes
default 64K max segment size and splits one virtual segment into two parts,
then breaks virt_boundary limit.

Before commit ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers"),
max segment size is set as UINT_MAX in case that virt_boundary is
defined.

Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ewan D. Milne <emilne@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Fixes: ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/storvsc_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 2e6b2412d2c9..1e7ad85f4ba3 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1897,6 +1897,7 @@ static struct scsi_host_template scsi_driver = {
 	.no_write_same =	1,
 	.track_queue_depth =	1,
 	.change_queue_depth =	storvsc_change_queue_depth,
+	.max_segment_size   = 0xffffffff,
 };
 
 enum {
-- 
2.47.0

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Bart Van Assche]

On 6/16/25 9:05 AM, Ming Lei wrote:
> - storvrc uses virt_boundary to define `segment`
     ^^^^^^^
storvsc?

> - strovrc does not define max_segment_size
     ^^^^^^^
storvsc?

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Wei Liu]

+Long for reviewing this patch.

On Tue, Jun 17, 2025 at 12:05:09AM +0800, Ming Lei wrote:
> Set max_segment_size as UINT_MAX explicitly:
> 
> - storvrc uses virt_boundary to define `segment`
> 
> - strovrc does not define max_segment_size
> 

Typo here. It should be storvsc, not storvrc.

> So define max_segment_size as UINT_MAX, otherwise __blk_rq_map_sg() takes
> default 64K max segment size and splits one virtual segment into two parts,
> then breaks virt_boundary limit.
> 
> Before commit ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers"),
> max segment size is set as UINT_MAX in case that virt_boundary is
> defined.
> 
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ewan D. Milne <emilne@redhat.com>
> Cc: Laurence Oberman <loberman@redhat.com>
> Fixes: ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/scsi/storvsc_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 2e6b2412d2c9..1e7ad85f4ba3 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1897,6 +1897,7 @@ static struct scsi_host_template scsi_driver = {
>  	.no_write_same =	1,
>  	.track_queue_depth =	1,
>  	.change_queue_depth =	storvsc_change_queue_depth,
> +	.max_segment_size   = 0xffffffff,
>  };
>  
>  enum {
> -- 
> 2.47.0
> 

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Martin K. Petersen]

On Tue, 17 Jun 2025 00:05:09 +0800, Ming Lei wrote:

> Set max_segment_size as UINT_MAX explicitly:
> 
> - storvrc uses virt_boundary to define `segment`
> 
> - strovrc does not define max_segment_size
> 
> So define max_segment_size as UINT_MAX, otherwise __blk_rq_map_sg() takes
> default 64K max segment size and splits one virtual segment into two parts,
> then breaks virt_boundary limit.
> 
> [...]

Applied to 6.16/scsi-fixes, thanks!

[1/1] scsi: storvsc: set max_segment_size as UINT_MAX explicitly
      https://git.kernel.org/mkp/scsi/c/19ec970841ca

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Christoph Hellwig]

On Tue, Jun 17, 2025 at 12:05:09AM +0800, Ming Lei wrote:
> Set max_segment_size as UINT_MAX explicitly:
> 
> - storvrc uses virt_boundary to define `segment`
> 
> - strovrc does not define max_segment_size
> 
> So define max_segment_size as UINT_MAX, otherwise __blk_rq_map_sg() takes
> default 64K max segment size and splits one virtual segment into two parts,
> then breaks virt_boundary limit.
> 
> Before commit ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers"),
> max segment size is set as UINT_MAX in case that virt_boundary is
> defined.

Drivers should not have done this.  If you need this someone (probably
me) broke the block layer code ensuring it's not needed, and that
affects all drivers using virt_boundary.

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Christoph Hellwig]

Please try this proper fix instead:

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index e021f1106bea..09f5fb5b2fb1 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -473,7 +473,9 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	else
 		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
 
-	if (sht->max_segment_size)
+	if (sht->virt_boundary_mask)
+		shost->virt_boundary_mask = sht->virt_boundary_mask;
+	else if (sht->max_segment_size)
 		shost->max_segment_size = sht->max_segment_size;
 	else
 		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
@@ -492,9 +494,6 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	else
 		shost->dma_boundary = 0xffffffff;
 
-	if (sht->virt_boundary_mask)
-		shost->virt_boundary_mask = sht->virt_boundary_mask;
-
 	device_initialize(&shost->shost_gendev);
 	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
 	shost->shost_gendev.bus = &scsi_bus_type;

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Ming Lei]

On Tue, Jun 17, 2025 at 07:02:40AM +0200, Christoph Hellwig wrote:
> Please try this proper fix instead:
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index e021f1106bea..09f5fb5b2fb1 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -473,7 +473,9 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
>  	else
>  		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
>  
> -	if (sht->max_segment_size)
> +	if (sht->virt_boundary_mask)
> +		shost->virt_boundary_mask = sht->virt_boundary_mask;
> +	else if (sht->max_segment_size)
>  		shost->max_segment_size = sht->max_segment_size;
>  	else
>  		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;

This way works, but I prefer to set it explicitly in driver, instead of
making block layer more fragile to deal with def ->max_segment_size
if ->virt_boundary_mask is defined

- for low level driver, if ->virt_boundary_mask is defined, ->max_segment_size
should be UINT_MAX obviously since it implies single `virt segment`.
Setting UINT_MAX in driver has document benefit too.

- for logical block device(md, dm, ...), both ->virt_boundary_mask and
->max_segment_size may be set, and it is fine since logical block device
driver needn't to deal with sg


Thanks,
Ming

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Christoph Hellwig]

On Tue, Jun 17, 2025 at 03:25:21PM +0800, Ming Lei wrote:
> > @@ -473,7 +473,9 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
> >  	else
> >  		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
> >  
> > -	if (sht->max_segment_size)
> > +	if (sht->virt_boundary_mask)
> > +		shost->virt_boundary_mask = sht->virt_boundary_mask;
> > +	else if (sht->max_segment_size)
> >  		shost->max_segment_size = sht->max_segment_size;
> >  	else
> >  		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
> 
> This way works, but I prefer to set it explicitly in driver, instead of
> making block layer more fragile to deal with def ->max_segment_size
> if ->virt_boundary_mask is defined

The block layer already enforces this as it is a requirement.  It is
just the SCSI wrapper that broke it.  Without this proper fix iser
is still broken, and srp might or might not.

> - for low level driver, if ->virt_boundary_mask is defined, ->max_segment_size
> should be UINT_MAX obviously since it implies single `virt segment`.
> Setting UINT_MAX in driver has document benefit too.

No, it doesn't.  It means you need to cargo cult copy and paste code
instead of solving the problem in the proper place.

> - for logical block device(md, dm, ...), both ->virt_boundary_mask and
> ->max_segment_size may be set, and it is fine since logical block device
> driver needn't to deal with sg

Stacked devices should not inherit the hardware limits at all because
we split below them, but that's a separate story.  Either way this is
not relevant here as we don't have stacking drivers that use the
SCSI layer.

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Laurence Oberman]

On Tue, 2025-06-17 at 07:02 +0200, Christoph Hellwig wrote:
> Please try this proper fix instead:
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index e021f1106bea..09f5fb5b2fb1 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -473,7 +473,9 @@ struct Scsi_Host *scsi_host_alloc(const struct
> scsi_host_template *sht, int priv
>         else
>                 shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
>  
> -       if (sht->max_segment_size)
> +       if (sht->virt_boundary_mask)
> +               shost->virt_boundary_mask = sht->virt_boundary_mask;
> +       else if (sht->max_segment_size)
>                 shost->max_segment_size = sht->max_segment_size;
>         else
>                 shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
> @@ -492,9 +494,6 @@ struct Scsi_Host *scsi_host_alloc(const struct
> scsi_host_template *sht, int priv
>         else
>                 shost->dma_boundary = 0xffffffff;
>  
> -       if (sht->virt_boundary_mask)
> -               shost->virt_boundary_mask = sht->virt_boundary_mask;
> -
>         device_initialize(&shost->shost_gendev);
>         dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
>         shost->shost_gendev.bus = &scsi_bus_type;
> 

Hello

Not sure what you folks will decide as the final fix but Christoph's
patch prevents the REDO corruption as well.
Ran it long enough to know its clean from corruption with this patch
above

Tested-by: Laurence Oberman <loberman@redhat.com>

Thanks vey mauch as always folks

Regards
Laurence

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Laurence Oberman]

On Tue, 2025-06-17 at 00:05 +0800, Ming Lei wrote:
> Set max_segment_size as UINT_MAX explicitly:
> 
> - storvrc uses virt_boundary to define `segment`
> 
> - strovrc does not define max_segment_size
> 
> So define max_segment_size as UINT_MAX, otherwise __blk_rq_map_sg()
> takes
> default 64K max segment size and splits one virtual segment into two
> parts,
> then breaks virt_boundary limit.
> 
> Before commit ec84ca4025c0 ("scsi: block: Remove now unused queue
> limits helpers"),
> max segment size is set as UINT_MAX in case that virt_boundary is
> defined.
> 
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ewan D. Milne <emilne@redhat.com>
> Cc: Laurence Oberman <loberman@redhat.com>
> Fixes: ec84ca4025c0 ("scsi: block: Remove now unused queue limits
> helpers")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/scsi/storvsc_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 2e6b2412d2c9..1e7ad85f4ba3 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1897,6 +1897,7 @@ static struct scsi_host_template scsi_driver =
> {
>         .no_write_same =        1,
>         .track_queue_depth =    1,
>         .change_queue_depth =   storvsc_change_queue_depth,
> +       .max_segment_size   = 0xffffffff,
>  };
>  
>  enum {

Hello 
For what it is worth, I tested Ming's patch in our lab and at our
customers and it fixed a very serious corruption in Oracle REDO logs.

Tested-by: Laurence Oberman  <loberman@redhat.com>

I will test what Christoph share dbut our initial way to deal with this
in RHEL will be the point fix in storvsc as its a critical issue
needing an urgent fix.

Thanks
Laurence

Re: [PATCH] scsi: storvsc: set max_segment_size as UINT_MAX explicitly

[Christoph Hellwig]

On Tue, Jun 17, 2025 at 12:05:09AM +0800, Ming Lei wrote:
> Fixes: ec84ca4025c0 ("scsi: block: Remove now unused queue limits helpers")

That's not the correct fixes tag.  This got broken by:

commit b561ea56a26415bf44ce8ca6a8e625c7c390f1ea
Author: Ming Lei <ming.lei@redhat.com>
Date:   Sun Apr 7 21:19:31 2024 +0800

    block: allow device to have both virt_boundary_mask and max segment size

for which I explicitly warned about this kind of breakage.

So please, let's fix this properly instead of piling crap over crap.