[PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Long Li]

q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There is no need to check it again in sd.

This change also allows a SCSI driver set an maximum sector size bigger than BLK_DEF_MAX_SECTORS, without returning values on optional VPD page 0xb0 "Block Limits".

Signed-off-by: Long Li <longli@microsoft.com>

---
 drivers/scsi/sd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 60bff78..d8c4047 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	    logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
 		q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
 		rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
-	} else
-		rw_max = BLK_DEF_MAX_SECTORS;
-
-	/* Combine with controller limits */
-	q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
+		q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
+	}
 
 	set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
 	sd_config_write_same(sdkp);
-- 
2.7.4

Re: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Tom Yan]

The main point there is not to check q->limits.max_sectors against
BLK_DEF_MAX_SECTORS, but sdkp->opt_xfer_blocks against
SD_DEF_XFER_BLOCKS et al.? `rw_max = BLK_DEF_MAX_SECTORS;` there is
merely the fallback when sdkp->opt_xfer_blocks does not pass the
conditions. With your patch `rw_max` can be indeterminate in those
circumstances.

On 4 June 2016 at 11:57, Long Li <longli@microsoft.com> wrote:
> q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There is no need to check it again in sd.
>
> This change also allows a SCSI driver set an maximum sector size bigger than BLK_DEF_MAX_SECTORS, without returning values on optional VPD page 0xb0 "Block Limits".
>
> Signed-off-by: Long Li <longli@microsoft.com>
>
> ---
>  drivers/scsi/sd.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 60bff78..d8c4047 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>             logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
>                 q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
>                 rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> -       } else
> -               rw_max = BLK_DEF_MAX_SECTORS;
> -
> -       /* Combine with controller limits */
> -       q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
> +               q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
> +       }
>
>         set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
>         sd_config_write_same(sdkp);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Long Li]

Sorry, "redundant check" is not the best word to describe this patch.

The result of this patch is that:
1. if opt_xfer_blocks has a valid value (returned form VPD BLOCK LIMITS), use it to set max_sectors
2. if opt_xfer_blocks doesn't have a valid value, leave max_sectors unchanged

The reason is that, max_sectors already has value at this point, the default value is SCSI_DEFAULT_MAX_SECTORS (include/scsi/scsi_host.h). The lower layer host driver can change this value in its template. I think the drivers care about this value have already set it. So it's better not to change it again. If they want max_sectors to be set by sd, they can use BLOCK LIMITS VPD to tell it to do so.


> -----Original Message-----
> From: Tom Yan [mailto:tom.ty89@gmail.com]
> Sent: Saturday, June 4, 2016 1:41 AM
> To: Long Li <longli@microsoft.com>
> Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] sd: remove redundant check for
> BLK_DEF_MAX_SECTORS
> 
> The main point there is not to check q->limits.max_sectors against
> BLK_DEF_MAX_SECTORS, but sdkp->opt_xfer_blocks against
> SD_DEF_XFER_BLOCKS et al.? `rw_max = BLK_DEF_MAX_SECTORS;` there is
> merely the fallback when sdkp->opt_xfer_blocks does not pass the
> conditions. With your patch `rw_max` can be indeterminate in those
> circumstances.
> 
> On 4 June 2016 at 11:57, Long Li <longli@microsoft.com> wrote:
> > q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS
> in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There
> is no need to check it again in sd.
> >
> > This change also allows a SCSI driver set an maximum sector size bigger
> than BLK_DEF_MAX_SECTORS, without returning values on optional VPD
> page 0xb0 "Block Limits".
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> >
> > ---
> >  drivers/scsi/sd.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> > 60bff78..d8c4047 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
> >             logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
> >                 q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
> >                 rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> > -       } else
> > -               rw_max = BLK_DEF_MAX_SECTORS;
> > -
> > -       /* Combine with controller limits */
> > -       q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
> > +               q->limits.max_sectors = min(rw_max,
> queue_max_hw_sectors(q));
> > +       }
> >
> >         set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
> >         sd_config_write_same(sdkp);
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at
> > https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.k
> > ernel.org%2fmajordomo-
> info.html&data=01%7c01%7clongli%40microsoft.com%
> >
> 7ce142128958ec47629dbe08d38c540306%7c72f988bf86f141af91ab2d7cd011d
> b47%
> > 7c1&sdata=EjjF86cvJqaxOAOWnN0%2f3Qln05qcquwe%2fKA7DgEjtcI%3d

Re: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Tom Yan]

Never mind. I misread. I thought q->limits.max_sectors = min(rw_max,
queue_max_hw_sectors(q)); can be run when rw_max is not set.

On 4 June 2016 at 23:18, Long Li <longli@microsoft.com> wrote:
> Sorry, "redundant check" is not the best word to describe this patch.
>
> The result of this patch is that:
> 1. if opt_xfer_blocks has a valid value (returned form VPD BLOCK LIMITS), use it to set max_sectors
> 2. if opt_xfer_blocks doesn't have a valid value, leave max_sectors unchanged
>
> The reason is that, max_sectors already has value at this point, the default value is SCSI_DEFAULT_MAX_SECTORS (include/scsi/scsi_host.h). The lower layer host driver can change this value in its template. I think the drivers care about this value have already set it. So it's better not to change it again. If they want max_sectors to be set by sd, they can use BLOCK LIMITS VPD to tell it to do so.
>
>
>> -----Original Message-----
>> From: Tom Yan [mailto:tom.ty89@gmail.com]
>> Sent: Saturday, June 4, 2016 1:41 AM
>> To: Long Li <longli@microsoft.com>
>> Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
>> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH] sd: remove redundant check for
>> BLK_DEF_MAX_SECTORS
>>
>> The main point there is not to check q->limits.max_sectors against
>> BLK_DEF_MAX_SECTORS, but sdkp->opt_xfer_blocks against
>> SD_DEF_XFER_BLOCKS et al.? `rw_max = BLK_DEF_MAX_SECTORS;` there is
>> merely the fallback when sdkp->opt_xfer_blocks does not pass the
>> conditions. With your patch `rw_max` can be indeterminate in those
>> circumstances.
>>
>> On 4 June 2016 at 11:57, Long Li <longli@microsoft.com> wrote:
>> > q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS
>> in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There
>> is no need to check it again in sd.
>> >
>> > This change also allows a SCSI driver set an maximum sector size bigger
>> than BLK_DEF_MAX_SECTORS, without returning values on optional VPD
>> page 0xb0 "Block Limits".
>> >
>> > Signed-off-by: Long Li <longli@microsoft.com>
>> >
>> > ---
>> >  drivers/scsi/sd.c | 7 ++-----
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
>> > 60bff78..d8c4047 100644
>> > --- a/drivers/scsi/sd.c
>> > +++ b/drivers/scsi/sd.c
>> > @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk)
>> >             logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
>> >                 q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
>> >                 rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
>> > -       } else
>> > -               rw_max = BLK_DEF_MAX_SECTORS;
>> > -
>> > -       /* Combine with controller limits */
>> > -       q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
>> > +               q->limits.max_sectors = min(rw_max,
>> queue_max_hw_sectors(q));
>> > +       }
>> >
>> >         set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
>> >         sd_config_write_same(sdkp);
>> > --
>> > 2.7.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>> > in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>> > info at
>> > https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.k
>> > ernel.org%2fmajordomo-
>> info.html&data=01%7c01%7clongli%40microsoft.com%
>> >
>> 7ce142128958ec47629dbe08d38c540306%7c72f988bf86f141af91ab2d7cd011d
>> b47%
>> > 7c1&sdata=EjjF86cvJqaxOAOWnN0%2f3Qln05qcquwe%2fKA7DgEjtcI%3d

Re: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Martin K. Petersen]

>>>>> "Long" == Long Li <longli@microsoft.com> writes:

Long,

Long> The reason is that, max_sectors already has value at this point,
Long> the default value is SCSI_DEFAULT_MAX_SECTORS
Long> (include/scsi/scsi_host.h). The lower layer host driver can change
Long> this value in its template.

The LLD sets max_hw_sectors which indicates the capabilities of the
controller DMA hardware. Whereas the max_sectors limit is set by sd to
either follow advise by the device or--if not provided--use the block
layer default. max_sectors governs the size of READ/WRITE requests and
do not reflect the capabilities of the DMA hardware.

Long> I think the drivers care about this value have already set it. So
Long> it's better not to change it again. If they want max_sectors to be
Long> set by sd, they can use BLOCK LIMITS VPD to tell it to do so.

Most drivers don't have the luxury of being able to generate VPDs for
their attached target devices :)

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Long Li]

Hi Martin,

Thanks for looking into this. The problem I'm trying to solve is that, I want to have lower layer driver to setup max_sectors bigger than BLK_DEF_MAX_SECTORS. In Hyper-v, we use 2MB max transfer I/O size, in future version the max transfer I/O size will increase to 8MB.
 
The implementation of sd.c limits the maximum value of max_sectors  to BLK_DEF_MAX_SECTORS.  Because sd_revalidate_disk is called late in the SCSI disk initialization process, there is no way for a lower layer driver to set this value to its "bigger" optimal size. 

The reason why I think it may not be necessary for sd.c to setup max_sectors, it's because this value may have already been setup twice before reaching the code in sd.c:
1. When this disk device is first scanned, or re-scanned (in scsi_scan.c), where it eventually calls __scsi_init_queue(), and use the max_sectors in the scsi_host_template.
2. in slave_configure of scsi_host_template, when the lower layer driver implements this function in its template and it can change this value there.

Long

> -----Original Message-----
> From: Martin K. Petersen [mailto:martin.petersen@oracle.com]
> Sent: Monday, June 6, 2016 8:42 PM
> To: Long Li <longli@microsoft.com>
> Cc: Tom Yan <tom.ty89@gmail.com>; James E.J. Bottomley
> <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] sd: remove redundant check for
> BLK_DEF_MAX_SECTORS
> 
> >>>>> "Long" == Long Li <longli@microsoft.com> writes:
> 
> Long,
> 
> Long> The reason is that, max_sectors already has value at this point,
> Long> the default value is SCSI_DEFAULT_MAX_SECTORS
> Long> (include/scsi/scsi_host.h). The lower layer host driver can change
> Long> this value in its template.
> 
> The LLD sets max_hw_sectors which indicates the capabilities of the
> controller DMA hardware. Whereas the max_sectors limit is set by sd to
> either follow advise by the device or--if not provided--use the block layer
> default. max_sectors governs the size of READ/WRITE requests and do not
> reflect the capabilities of the DMA hardware.
> 
> Long> I think the drivers care about this value have already set it. So
> Long> it's better not to change it again. If they want max_sectors to be
> Long> set by sd, they can use BLOCK LIMITS VPD to tell it to do so.
> 
> Most drivers don't have the luxury of being able to generate VPDs for their
> attached target devices :)
> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: remove redundant check for BLK_DEF_MAX_SECTORS

[Martin K. Petersen]

>>>>> "Long" == Long Li <longli@microsoft.com> writes:

Long,

Long> The problem I'm trying to solve is that, I want to have lower
Long> layer driver to setup max_sectors bigger than
Long> BLK_DEF_MAX_SECTORS.

Capping at BLK_DEF_MAX_SECTORS unless a device has explicitly reported
requirements is intentional. We have not had good experiences with
making I/O requests too big in general. So BLK_DEF_MAX_SECTORS has
deliberately been kept small. However, it was recently bumped to 1MB and
change by default.

Long> n Hyper-v, we use 2MB max transfer I/O size, in future version the
Long> max transfer I/O size will increase to 8MB.

But presumably you provide a BLOCK LIMITS VPD for your virtual targets?

Long> The reason why I think it may not be necessary for sd.c to setup
Long> max_sectors, it's because this value may have already been setup
Long> twice before reaching the code in sd.c: 1. When this disk device
Long> is first scanned, or re-scanned (in scsi_scan.c), where it
Long> eventually calls __scsi_init_queue(), and use the max_sectors in
Long> the scsi_host_template.  2. in slave_configure of
Long> scsi_host_template, when the lower layer driver implements this
Long> function in its template and it can change this value there.

Those cause limits to be set for the controller. We won't know the
device limits until we hit revalidate.

blk_queue_max_hw_sectors() will also clamp the R/W max at
BLK_DEF_MAX_SECTORS, though.

-- 
Martin K. Petersen	Oracle Linux Engineering