[PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[PATCH V2] scsi: add a new flag 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
support, and only WRITE_SAME_16 is supported, not WRITE_SAME_10.
Therefore, we need to provide a new flag for these storage devices. When
initializing these devices, we will no longer query for support for
WRITE_SAME_16 in the sd_read_write_same(), but set these SCSI disks to
support WRITE_SAME_16 by default. In that way, we can add
'vendor:product:flag' to the module parameter 'dev_flags' for these
storage devices.

Signed-off-by: AlexChen <alex.chen@huawei.com>
---
 drivers/scsi/sd.c           | 4 +++-
 include/scsi/scsi_devinfo.h | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4f7e7b607..a208ba5b5 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -68,6 +68,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_devinfo.h>

 #include "sd.h"
 #include "scsi_priv.h"
@@ -3014,7 +3015,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 ||
+			sdev->sdev_bflags & BLIST_SUPPORT_WS16)
 		sdkp->ws16 = 1;

 	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1)
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3fdb322d4..da70d4795 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -67,8 +67,10 @@
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 /* Always retry ABORTED_COMMAND with ASC 0xc1 */
 #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
+/* support for write_same_16, no need to query */
+#define BLIST_SUPPORT_WS16      ((__force blist_flags_t)(1ULL << 34))

-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+#define __BLIST_LAST_USED BLIST_SUPPORT_WS16

 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
-- 
2.19.1

Re: [PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[Martin K. Petersen]

Hi Alex,

> 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.

Your proposed code change is fine and to the point. However, I'd like to
understand why you are adding a workaround to the kernel instead of
fixing the affected device?

Implementing support for either WRITE SAME(10) or REPORT SUPPORTED
OPERATION CODES is easy. And the latter in particular is beneficial for
discovering several other SCSI protocol features. It's a good command to
support in general.

Also, we generally don't add features to the kernel without any
users. So if you add a blacklist flag, I would expect to see a set of
device strings to be added to scsi_devinfo.c.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[AlexChen]

Hi Martin,

Thanks for your reply.

>> 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.
> 
> Your proposed code change is fine and to the point. However, I'd like to
> understand why you are adding a workaround to the kernel instead of
> fixing the affected device?
> 
> Implementing support for either WRITE SAME(10) or REPORT SUPPORTED
> OPERATION CODES is easy. And the latter in particular is beneficial for
> discovering several other SCSI protocol features. It's a good command to
> support in general.
> 

From a maintenance perspective, I think the old storage device which does
not support WRITE SAME query interface can be easily supported by adding
a workaround to the kernel, instead of waiting for the storage device to
implement the query interface.

> Also, we generally don't add features to the kernel without any
> users. So if you add a blacklist flag, I would expect to see a set of
> device strings to be added to scsi_devinfo.c.

Through my test, I found that HUAWEI's storage devices do not provide
queries for WRITE_SAME_16 support.
If the lists of such devices is written into the kernel code, will it
be incomplete and difficult to maintain? On the other hand, It would be
more flexible if we provided the module parameter 'dev_flags' to set.

Thanks
Alex

Re: [PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[AlexChen]

Hi Martin,

Are there any other opinions about this patch? If so, I can make
corresponding modifications.

Thanks
Alex
On 2020/2/7 16:51, AlexChen wrote:
> Hi Martin,
> 
> Thanks for your reply.
> 
>>> 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.
>>
>> Your proposed code change is fine and to the point. However, I'd like to
>> understand why you are adding a workaround to the kernel instead of
>> fixing the affected device?
>>
>> Implementing support for either WRITE SAME(10) or REPORT SUPPORTED
>> OPERATION CODES is easy. And the latter in particular is beneficial for
>> discovering several other SCSI protocol features. It's a good command to
>> support in general.
>>
> 
>>From a maintenance perspective, I think the old storage device which does
> not support WRITE SAME query interface can be easily supported by adding
> a workaround to the kernel, instead of waiting for the storage device to
> implement the query interface.
> 
>> Also, we generally don't add features to the kernel without any
>> users. So if you add a blacklist flag, I would expect to see a set of
>> device strings to be added to scsi_devinfo.c.
> 
> Through my test, I found that HUAWEI's storage devices do not provide
> queries for WRITE_SAME_16 support.
> If the lists of such devices is written into the kernel code, will it
> be incomplete and difficult to maintain? On the other hand, It would be
> more flexible if we provided the module parameter 'dev_flags' to set.
> 
> Thanks
> Alex
> 

Re: [PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[Christoph Hellwig]

On Sat, Feb 01, 2020 at 02:54:31PM +0800, 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
> support, and only WRITE_SAME_16 is supported, not WRITE_SAME_10.
> Therefore, we need to provide a new flag for these storage devices. When
> initializing these devices, we will no longer query for support for
> WRITE_SAME_16 in the sd_read_write_same(), but set these SCSI disks to
> support WRITE_SAME_16 by default. In that way, we can add
> 'vendor:product:flag' to the module parameter 'dev_flags' for these
> storage devices.

Please send this along with the patch that actually sets the flag
somewhere..

Re: [PATCH V2] scsi: add a new flag to set whether SCSI disks support WRITE_SAME_16 by default

[AlexChen]

Hi Christoph,

Thanks for your reply.

On 2020/2/26 2:31, Christoph Hellwig wrote:
> On Sat, Feb 01, 2020 at 02:54:31PM +0800, 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
>> support, and only WRITE_SAME_16 is supported, not WRITE_SAME_10.
>> Therefore, we need to provide a new flag for these storage devices. When
>> initializing these devices, we will no longer query for support for
>> WRITE_SAME_16 in the sd_read_write_same(), but set these SCSI disks to
>> support WRITE_SAME_16 by default. In that way, we can add
>> 'vendor:product:flag' to the module parameter 'dev_flags' for these
>> storage devices.
> 
> Please send this along with the patch that actually sets the flag
> somewhere..
> 

If we want Huawei storage device to support WRITE_SAME_16 by default,
we can add module parameter dev_flags='HUAWEI::‭17179869184‬' to the kernel
command line parameters.
Do you mean I need to add these to this patch description?

Thanks
Alex