[PATCH] scsi: sd: provide a new module parameter to set whether SCSI disks support WRITE_SAME_16 by default

[PATCH] scsi: sd: provide a new module parameter to set whether SCSI disks support WRITE_SAME_16 by default

[AlexChen]

When the SCSI device is initialized, check whether it supports
WRITE_SAME_16 or WRITE_SAME_10 in the sd_read_write_same(). If
the back-end storage device does not support queries, it will not set
sdkp->ws16 as 1.

When the WRITE_SAME io is issued through the blkdev_issue_write_same(),
the WRITE_SAME type is set to WRITE_SAME_10 by default in the
sd_setup_write_same_cmnd() since of "sdkp->ws16=0". If the storage device
does not support WRITE_SAME_10, then the SCSI device is set to not support
WRITE_SAME.

Currently, some storage devices do not provide queries for
WRITE_SAME_16/WRITE_SAME_10 support, but they do support WRITE_SAME_16 and
do not support WRITE_SAME_10. So in order for these devices to use
WRITE_SAME, we need a new module parameter to set whether SCSI disks
support WRITE_SAME_16 by default.

Signed-off-by: AlexChen <alex.chen@huawei.com>
---
 drivers/scsi/sd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4f7e7b607..ff368701d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -104,6 +104,9 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);
 #define SD_MINORS	0
 #endif

+static int sd_default_support_ws16;
+module_param(sd_default_support_ws16, int, 0644);
+
 static void sd_config_discard(struct scsi_disk *, unsigned int);
 static void sd_config_write_same(struct scsi_disk *);
 static int  sd_revalidate_disk(struct gendisk *);
@@ -3014,7 +3017,8 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 			sdev->no_write_same = 1;
 	}

-	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1)
+	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1 ||
+			sd_default_support_ws16)
 		sdkp->ws16 = 1;

 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1)
-- 2.19.1

Re: [PATCH] scsi: sd: provide a new module parameter to set whether SCSI disks support WRITE_SAME_16 by default

[Bart Van Assche]

On 2020-01-19 06:13, AlexChen wrote:
> When the SCSI device is initialized, check whether it supports
> WRITE_SAME_16 or WRITE_SAME_10 in the sd_read_write_same(). If
> the back-end storage device does not support queries, it will not set
> sdkp->ws16 as 1.
> 
> When the WRITE_SAME io is issued through the blkdev_issue_write_same(),
> the WRITE_SAME type is set to WRITE_SAME_10 by default in the
> sd_setup_write_same_cmnd() since of "sdkp->ws16=0". If the storage device
> does not support WRITE_SAME_10, then the SCSI device is set to not support
> WRITE_SAME.
> 
> Currently, some storage devices do not provide queries for
> WRITE_SAME_16/WRITE_SAME_10 support, but they do support WRITE_SAME_16 and
> do not support WRITE_SAME_10. So in order for these devices to use
> WRITE_SAME, we need a new module parameter to set whether SCSI disks
> support WRITE_SAME_16 by default.
> 
> Signed-off-by: AlexChen <alex.chen@huawei.com>
> ---
>  drivers/scsi/sd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4f7e7b607..ff368701d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -104,6 +104,9 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);
>  #define SD_MINORS	0
>  #endif
> 
> +static int sd_default_support_ws16;
> +module_param(sd_default_support_ws16, int, 0644);
> +
>  static void sd_config_discard(struct scsi_disk *, unsigned int);
>  static void sd_config_write_same(struct scsi_disk *);
>  static int  sd_revalidate_disk(struct gendisk *);
> @@ -3014,7 +3017,8 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
>  			sdev->no_write_same = 1;
>  	}
> 
> -	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1)
> +	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1 ||
> +			sd_default_support_ws16)
>  		sdkp->ws16 = 1;
> 
>  	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1)

Should this be fixed using the quirk mechanism instead of introducing a
new kernel module parameter? Kernel module parameters apply to all SCSI
disk devices irrespective of their vendor and model. The quirk mechanism
can be used to introduce special behavior for certain disk models and
types. See also the output of the following grep command:

$ git grep -nH '& BLIST'

Bart.

Re: [PATCH] scsi: sd: provide a new module parameter to set whether SCSI disks support WRITE_SAME_16 by default

[AlexChen]

On 2020/1/20 3:36, Bart Van Assche wrote:
> On 2020-01-19 06:13, AlexChen wrote:
>> When the SCSI device is initialized, check whether it supports
>> WRITE_SAME_16 or WRITE_SAME_10 in the sd_read_write_same(). If
>> the back-end storage device does not support queries, it will not set
>> sdkp->ws16 as 1.
>>
>> When the WRITE_SAME io is issued through the blkdev_issue_write_same(),
>> the WRITE_SAME type is set to WRITE_SAME_10 by default in the
>> sd_setup_write_same_cmnd() since of "sdkp->ws16=0". If the storage device
>> does not support WRITE_SAME_10, then the SCSI device is set to not support
>> WRITE_SAME.
>>
>> Currently, some storage devices do not provide queries for
>> WRITE_SAME_16/WRITE_SAME_10 support, but they do support WRITE_SAME_16 and
>> do not support WRITE_SAME_10. So in order for these devices to use
>> WRITE_SAME, we need a new module parameter to set whether SCSI disks
>> support WRITE_SAME_16 by default.
>>
>> Signed-off-by: AlexChen <alex.chen@huawei.com>
>> ---
>>  drivers/scsi/sd.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 4f7e7b607..ff368701d 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -104,6 +104,9 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_ZBC);
>>  #define SD_MINORS	0
>>  #endif
>>
>> +static int sd_default_support_ws16;
>> +module_param(sd_default_support_ws16, int, 0644);
>> +
>>  static void sd_config_discard(struct scsi_disk *, unsigned int);
>>  static void sd_config_write_same(struct scsi_disk *);
>>  static int  sd_revalidate_disk(struct gendisk *);
>> @@ -3014,7 +3017,8 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
>>  			sdev->no_write_same = 1;
>>  	}
>>
>> -	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1)
>> +	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1 ||
>> +			sd_default_support_ws16)
>>  		sdkp->ws16 = 1;
>>
>>  	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1)
> 
> Should this be fixed using the quirk mechanism instead of introducing a
> new kernel module parameter? Kernel module parameters apply to all SCSI
> disk devices irrespective of their vendor and model. The quirk mechanism
> can be used to introduce special behavior for certain disk models and
> types. See also the output of the following grep command:
> 
> $ git grep -nH '& BLIST'
> 
> Bart.
> 
Thanks for your reply.
I will try to fix the problem by the way you suggested above.

Thanks
Alex
> 
> .
> 

Re: [PATCH] scsi: sd: provide a new module parameter to set whether SCSI disks support WRITE_SAME_16 by default

[Martin K. Petersen]

Bart,

>> +	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1 ||
>> +			sd_default_support_ws16)
>>  		sdkp->ws16 = 1;
>> 
>>  	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1)
>
> Should this be fixed using the quirk mechanism instead of introducing a
> new kernel module parameter? Kernel module parameters apply to all SCSI
> disk devices irrespective of their vendor and model. The quirk mechanism
> can be used to introduce special behavior for certain disk models and
> types.

Yes, this should definitely be a blacklist option. Although I'd
obviously encourage the vendor to implement RSOC instead of working
around this in the kernel.

-- 
Martin K. Petersen	Oracle Linux Engineering