| Message ID | 20250616160509.52491-1-ming.lei@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: storvsc: set max_segment_size as UINT_MAX explicitly | expand |
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.
+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 >
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
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.
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;
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
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
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.
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 {
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(+)