[PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

Some devices do not support a WRITE SAME / WRITE SAME(16) with the UNMAP
bit set up to the length specified in the MAXIMUM WRITE SAME LENGTH field
in the block limits VPD page (or, the field is zero, indicating there is
no limit).  Limit the length by the MAXIMUM UNMAP LBA COUNT value.
Otherwise the command might be rejected.

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/sd.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6549e5c..fa07ac2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -693,6 +693,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 	struct request_queue *q = sdkp->disk->queue;
 	unsigned int logical_block_size = sdkp->device->sector_size;
 	unsigned int max_blocks = 0;
+	u32 max_ws_blocks = sdkp->max_ws_blocks;
 
 	q->limits.discard_alignment =
 		sdkp->unmap_alignment * logical_block_size;
@@ -701,6 +702,13 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 		    sdkp->unmap_granularity * logical_block_size);
 	sdkp->provisioning_mode = mode;
 
+	/*
+	 * Some devices limit WRITE SAME / WRITE SAME(16) w/UNMAP
+	 * by MAXIMUM UNMAP LBA COUNT instead of MAXIMUM WRITE SAME LENGTH.
+	 * Use the smaller of the two values to avoid ILLEGAL REQUEST errors.
+	 */
+	max_ws_blocks = min_not_zero(max_ws_blocks, sdkp->max_unmap_blocks);
+
 	switch (mode) {
 
 	case SD_LBP_FULL:
@@ -715,17 +723,17 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 		break;
 
 	case SD_LBP_WS16:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
+		max_blocks = min_not_zero(max_ws_blocks,
 					  (u32)SD_MAX_WS16_BLOCKS);
 		break;
 
 	case SD_LBP_WS10:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
+		max_blocks = min_not_zero(max_ws_blocks,
 					  (u32)SD_MAX_WS10_BLOCKS);
 		break;
 
 	case SD_LBP_ZERO:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
+		max_blocks = min_not_zero(max_ws_blocks,
 					  (u32)SD_MAX_WS10_BLOCKS);
 		break;
 	}
-- 
2.1.0

RE: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Kuzeja, William]

Ewan,

I like it, more generic than my patch. I never saw the other cases, so I limited my 
patch to WS16.

Acked-by: Bill Kuzeja <William.Kuzeja@stratus.com>

On Tue-09-19 at 12:14 Ewan D. Milne wrote: 
> Some devices do not support a WRITE SAME / WRITE SAME(16) with the
> UNMAP
> bit set up to the length specified in the MAXIMUM WRITE SAME LENGTH
> field
> in the block limits VPD page (or, the field is zero, indicating there is
> no limit).  Limit the length by the MAXIMUM UNMAP LBA COUNT value.
> Otherwise the command might be rejected.
> 
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>  drivers/scsi/sd.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 6549e5c..fa07ac2 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -693,6 +693,7 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> unsigned int mode)
>  	struct request_queue *q = sdkp->disk->queue;
>  	unsigned int logical_block_size = sdkp->device->sector_size;
>  	unsigned int max_blocks = 0;
> +	u32 max_ws_blocks = sdkp->max_ws_blocks;
> 
>  	q->limits.discard_alignment =
>  		sdkp->unmap_alignment * logical_block_size;
> @@ -701,6 +702,13 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> unsigned int mode)
>  		    sdkp->unmap_granularity * logical_block_size);
>  	sdkp->provisioning_mode = mode;
> 
> +	/*
> +	 * Some devices limit WRITE SAME / WRITE SAME(16) w/UNMAP
> +	 * by MAXIMUM UNMAP LBA COUNT instead of MAXIMUM WRITE
> SAME LENGTH.
> +	 * Use the smaller of the two values to avoid ILLEGAL REQUEST
> errors.
> +	 */
> +	max_ws_blocks = min_not_zero(max_ws_blocks, sdkp-
> >max_unmap_blocks);
> +
>  	switch (mode) {
> 
>  	case SD_LBP_FULL:
> @@ -715,17 +723,17 @@ static void sd_config_discard(struct scsi_disk *sdkp,
> unsigned int mode)
>  		break;
> 
>  	case SD_LBP_WS16:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> +		max_blocks = min_not_zero(max_ws_blocks,
>  					  (u32)SD_MAX_WS16_BLOCKS);
>  		break;
> 
>  	case SD_LBP_WS10:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> +		max_blocks = min_not_zero(max_ws_blocks,
>  					  (u32)SD_MAX_WS10_BLOCKS);
>  		break;
> 
>  	case SD_LBP_ZERO:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> +		max_blocks = min_not_zero(max_ws_blocks,
>  					  (u32)SD_MAX_WS10_BLOCKS);
>  		break;
>  	}
> --
> 2.1.0

Re: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Martin K. Petersen]

Ewan,

> Some devices do not support a WRITE SAME / WRITE SAME(16) with the
> UNMAP bit set up to the length specified in the MAXIMUM WRITE SAME
> LENGTH field in the block limits VPD page (or, the field is zero,
> indicating there is no limit).  Limit the length by the MAXIMUM UNMAP
> LBA COUNT value.  Otherwise the command might be rejected.

>From SBC4:

  "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
  maximum number of LBAs that may be unmapped by an UNMAP command"

Note that it explicitly states "UNMAP command" and not "unmap
operation".

  "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
  the maximum number of contiguous logical blocks that the device server
  allows to be unmapped or written in a single WRITE SAME command."

It says "unmapped or written" and "WRITE SAME command".

The spec is crystal clear. The device needs to be fixed. We can
blacklist older firmware revs.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Ewan D. Milne]

On Mon, 2017-09-25 at 21:46 -0400, Martin K. Petersen wrote:
> Ewan,
> 
> > Some devices do not support a WRITE SAME / WRITE SAME(16) with the
> > UNMAP bit set up to the length specified in the MAXIMUM WRITE SAME
> > LENGTH field in the block limits VPD page (or, the field is zero,
> > indicating there is no limit).  Limit the length by the MAXIMUM UNMAP
> > LBA COUNT value.  Otherwise the command might be rejected.
> 
> From SBC4:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
>   maximum number of LBAs that may be unmapped by an UNMAP command"
> 
> Note that it explicitly states "UNMAP command" and not "unmap
> operation".
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
>   the maximum number of contiguous logical blocks that the device server
>   allows to be unmapped or written in a single WRITE SAME command."
> 
> It says "unmapped or written" and "WRITE SAME command".
> 
> The spec is crystal clear. The device needs to be fixed. We can
> blacklist older firmware revs.
> 

Yes, I know that is what SBC-4 says, and I agree that the devices
are not conforming.  Unfortunately, I've come across 3 different
arrays now from 3 different manufacturers that exhibit this behavior.

cc: Fred Knight for his opinion on this (NetApp was not one of the
arrays that I've run into, though).

-Ewan

RE: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Knight, Frederick]

I agree that it is disappointing that so many vendors seem to have trouble reading the spec.  This case is pretty clear.

The best the T10 committee could do is add a bit to indicate that the device uses the length from MAXIMUM UNMAP LBA COUNT field for the length of unmaps via the WRITE SAME w/UNMAP=1 rather than the MAXIMUM WRITE SAME LENGTH field.  BUT, I'll be very clear that the setting of any such new bit will be bit=0 is backward compatible for COMPLIANT devices, and bit=1 will be the new setting for "backwards" devices - which means they would STILL require a firmware change to tell you they are backwards, and you'd STILL need a blacklist for their older revisions.  And this would just makes the hosts job all that much harder!

Once a device is broken (violates the spec), there is not very much we can do in the spec to fix it (they have to fix their broken device).

	Fred

-----Original Message-----
From: Ewan D. Milne [mailto:emilne@redhat.com] 
Sent: Wednesday, September 27, 2017 12:28 PM
To: Martin K. Petersen <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org; Knight, Frederick <Frederick.Knight@netapp.com>
Subject: Re: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

On Mon, 2017-09-25 at 21:46 -0400, Martin K. Petersen wrote:
> Ewan,
> 
> > Some devices do not support a WRITE SAME / WRITE SAME(16) with the
> > UNMAP bit set up to the length specified in the MAXIMUM WRITE SAME
> > LENGTH field in the block limits VPD page (or, the field is zero,
> > indicating there is no limit).  Limit the length by the MAXIMUM UNMAP
> > LBA COUNT value.  Otherwise the command might be rejected.
> 
> From SBC4:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
>   maximum number of LBAs that may be unmapped by an UNMAP command"
> 
> Note that it explicitly states "UNMAP command" and not "unmap
> operation".
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
>   the maximum number of contiguous logical blocks that the device server
>   allows to be unmapped or written in a single WRITE SAME command."
> 
> It says "unmapped or written" and "WRITE SAME command".
> 
> The spec is crystal clear. The device needs to be fixed. We can
> blacklist older firmware revs.
> 

Yes, I know that is what SBC-4 says, and I agree that the devices
are not conforming.  Unfortunately, I've come across 3 different
arrays now from 3 different manufacturers that exhibit this behavior.

cc: Fred Knight for his opinion on this (NetApp was not one of the
arrays that I've run into, though).

-Ewan

Re: [PATCH] sd: Limit WRITE SAME / WRITE SAME(16) w/UNMAP length for certain devices

[Martin K. Petersen]

Ewan,

>> The spec is crystal clear. The device needs to be fixed. We can
>> blacklist older firmware revs.
>
> Yes, I know that is what SBC-4 says, and I agree that the devices
> are not conforming.  Unfortunately, I've come across 3 different
> arrays now from 3 different manufacturers that exhibit this behavior.

My main beef with your approach is that it penalizes devices that
actually implement the spec correctly.

I'll send a proposed patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Martin K. Petersen]

SBC-4 states:

  "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
   maximum number of LBAs that may be unmapped by an UNMAP command"

  "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
   the maximum number of contiguous logical blocks that the device server
   allows to be unmapped or written in a single WRITE SAME command."

Despite the spec being clear on the topic, some devices incorrectly
expect WRITE SAME commands with the UNMAP bit set to be limited to the
value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.

Implement a blacklist option that can be used to accommodate devices
with this behavior.

Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
Reported-by: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_scan.c    |  3 +++
 drivers/scsi/sd.c           | 16 ++++++++++++----
 include/scsi/scsi_device.h  |  1 +
 include/scsi/scsi_devinfo.h |  1 +
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e7818afeda2b..15590a063ad9 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -956,6 +956,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (*bflags & BLIST_NO_DIF)
 		sdev->no_dif = 1;
 
+	if (*bflags & BLIST_UNMAP_LIMIT_WS)
+		sdev->unmap_limit_for_ws = 1;
+
 	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
 
 	if (*bflags & BLIST_TRY_VPD_PAGES)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b18ba3235900..347be7580181 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -715,13 +715,21 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 		break;
 
 	case SD_LBP_WS16:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
-					  (u32)SD_MAX_WS16_BLOCKS);
+		if (sdkp->device->unmap_limit_for_ws)
+			max_blocks = sdkp->max_unmap_blocks;
+		else
+			max_blocks = sdkp->max_ws_blocks;
+
+		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS16_BLOCKS);
 		break;
 
 	case SD_LBP_WS10:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
-					  (u32)SD_MAX_WS10_BLOCKS);
+		if (sdkp->device->unmap_limit_for_ws)
+			max_blocks = sdkp->max_unmap_blocks;
+		else
+			max_blocks = sdkp->max_ws_blocks;
+
+		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS10_BLOCKS);
 		break;
 
 	case SD_LBP_ZERO:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 82e93ee94708..67c5a9f223f7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -192,6 +192,7 @@ struct scsi_device {
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 	unsigned broken_fua:1;		/* Don't set FUA bit */
 	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
+	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit for WRITE SAME */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 9592570e092a..36b03013d629 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -29,5 +29,6 @@
 #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
 #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
 #define BLIST_MAX_1024		0x40000000 /* maximum 1024 sector cdb length */
+#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit for WRITE SAME */
 
 #endif
-- 
2.14.1

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Ewan D. Milne]

On Wed, 2017-09-27 at 21:35 -0400, Martin K. Petersen wrote:
> SBC-4 states:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
>    maximum number of LBAs that may be unmapped by an UNMAP command"
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
>    the maximum number of contiguous logical blocks that the device server
>    allows to be unmapped or written in a single WRITE SAME command."
> 
> Despite the spec being clear on the topic, some devices incorrectly
> expect WRITE SAME commands with the UNMAP bit set to be limited to the
> value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.
> 
> Implement a blacklist option that can be used to accommodate devices
> with this behavior.
> 
> Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
> Reported-by: Ewan D. Milne <emilne@redhat.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_scan.c    |  3 +++
>  drivers/scsi/sd.c           | 16 ++++++++++++----
>  include/scsi/scsi_device.h  |  1 +
>  include/scsi/scsi_devinfo.h |  1 +
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e7818afeda2b..15590a063ad9 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -956,6 +956,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	if (*bflags & BLIST_NO_DIF)
>  		sdev->no_dif = 1;
>  
> +	if (*bflags & BLIST_UNMAP_LIMIT_WS)
> +		sdev->unmap_limit_for_ws = 1;
> +
>  	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>  
>  	if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b18ba3235900..347be7580181 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -715,13 +715,21 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
>  		break;
>  
>  	case SD_LBP_WS16:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS16_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS16_BLOCKS);
>  		break;
>  
>  	case SD_LBP_WS10:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS10_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS10_BLOCKS);
>  		break;
>  
>  	case SD_LBP_ZERO:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 82e93ee94708..67c5a9f223f7 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -192,6 +192,7 @@ struct scsi_device {
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
>  	unsigned broken_fua:1;		/* Don't set FUA bit */
>  	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
> +	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit for WRITE SAME */
>  
>  	atomic_t disk_events_disable_depth; /* disable depth for disk events */
>  
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 9592570e092a..36b03013d629 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -29,5 +29,6 @@
>  #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
>  #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
>  #define BLIST_MAX_1024		0x40000000 /* maximum 1024 sector cdb length */
> +#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit for WRITE SAME */
>  
>  #endif

Thanks.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Laurence Oberman]

On Wed, 2017-09-27 at 21:35 -0400, Martin K. Petersen wrote:
> SBC-4 states:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates
> the
>    maximum number of LBAs that may be unmapped by an UNMAP command"
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value
> indicates
>    the maximum number of contiguous logical blocks that the device
> server
>    allows to be unmapped or written in a single WRITE SAME command."
> 
> Despite the spec being clear on the topic, some devices incorrectly
> expect WRITE SAME commands with the UNMAP bit set to be limited to
> the
> value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.
> 
> Implement a blacklist option that can be used to accommodate devices
> with this behavior.
> 
> Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
> Reported-by: Ewan D. Milne <emilne@redhat.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_scan.c    |  3 +++
>  drivers/scsi/sd.c           | 16 ++++++++++++----
>  include/scsi/scsi_device.h  |  1 +
>  include/scsi/scsi_devinfo.h |  1 +
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e7818afeda2b..15590a063ad9 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -956,6 +956,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> unsigned char *inq_result,
>  	if (*bflags & BLIST_NO_DIF)
>  		sdev->no_dif = 1;
>  
> +	if (*bflags & BLIST_UNMAP_LIMIT_WS)
> +		sdev->unmap_limit_for_ws = 1;
> +
>  	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>  
>  	if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b18ba3235900..347be7580181 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -715,13 +715,21 @@ static void sd_config_discard(struct scsi_disk
> *sdkp, unsigned int mode)
>  		break;
>  
>  	case SD_LBP_WS16:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS16_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks,
> (u32)SD_MAX_WS16_BLOCKS);
>  		break;
>  
>  	case SD_LBP_WS10:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS10_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks,
> (u32)SD_MAX_WS10_BLOCKS);
>  		break;
>  
>  	case SD_LBP_ZERO:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 82e93ee94708..67c5a9f223f7 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -192,6 +192,7 @@ struct scsi_device {
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled
> */
>  	unsigned broken_fua:1;		/* Don't set FUA bit
> */
>  	unsigned lun_in_cdb:1;		/* Store LUN bits in
> CDB[1] */
> +	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit
> for WRITE SAME */
>  
>  	atomic_t disk_events_disable_depth; /* disable depth for
> disk events */
>  
> diff --git a/include/scsi/scsi_devinfo.h
> b/include/scsi/scsi_devinfo.h
> index 9592570e092a..36b03013d629 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -29,5 +29,6 @@
>  #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD
> pages */
>  #define BLIST_NO_RSOC		0x20000000 /* don't try to
> issue RSOC */
>  #define BLIST_MAX_1024		0x40000000 /* maximum 1024
> sector cdb length */
> +#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit
> for WRITE SAME */
>  
>  #endif

Hello Martin
I am testing this but its not being picked up so I want to know if I
have the kernel command line wrong here.

scsi_dev_flags=LIO-ORG:thin2:0x80000000

Device is here
[   16.853083] scsi 4:0:0:50: Direct-Access     LIO-
ORG  thin2            4.0  PQ: 0 ANSI: 5

Have a couple of printk's in now to see if I see the flags and they
dont trigger

        case SD_LBP_WS16:
                if (sdkp->device->unmap_limit_for_ws) {
                        max_blocks = sdkp->max_unmap_blocks;
                        printk("RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS16\n");
                }
                else
                        max_blocks = sdkp->max_ws_blocks;

                max_blocks = min_not_zero(max_blocks,
(u32)SD_MAX_WS16_BLOCKS);
                break;

        case SD_LBP_WS10:
                if (sdkp->device->unmap_limit_for_ws) {
                        max_blocks = sdkp->max_unmap_blocks;
                        printk("RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS10\n");
                }
                else
                        max_blocks = sdkp->max_ws_blocks;

                max_blocks = min_not_zero(max_blocks,
(u32)SD_MAX_WS10_BLOCKS);
                break;

What am I doing wrong to pass the BLIST flags.

Thanks
Laurence

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Martin K. Petersen]

Laurence,

> I am testing this but its not being picked up so I want to know if I
> have the kernel command line wrong here.
>
> scsi_dev_flags=LIO-ORG:thin2:0x80000000
>
> What am I doing wrong to pass the BLIST flags.

This worked for me:

[root@kvm ~]# echo "Linux:scsi_debug:0x80000000" > /proc/scsi/device_info
[root@kvm ~]# grep Linux /proc/scsi/device_info 
'Linux   ' 'scsi_debug      ' 0x80000000
[root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10 unmap_max_desc=1 write_same_length=20 lbpws=1
[root@kvm ~]# lsblk -D
NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda         0      512B       5K         0

(With the caveat that I tweaked scsi_debug to report the UNMAP
parameters despite lbpu being 0).

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Laurence Oberman]

On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> Laurence,
> 
> > I am testing this but its not being picked up so I want to know if
> > I
> > have the kernel command line wrong here.
> > 
> > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > 
> > What am I doing wrong to pass the BLIST flags.
> 
> This worked for me:
> 
> [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> /proc/scsi/device_info
> [root@kvm ~]# grep Linux /proc/scsi/device_info 
> 'Linux   ' 'scsi_debug      ' 0x80000000
> [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> unmap_max_desc=1 write_same_length=20 lbpws=1
> [root@kvm ~]# lsblk -D
> NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> sda         0      512B       5K         0
> 
> (With the caveat that I tweaked scsi_debug to report the UNMAP
> parameters despite lbpu being 0).
> 

OK, Thanks, that is working now and I pick up the correct size now.
Its going to be very useful for these corner case array
inconsistencies.

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

Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-Access     LIO-
ORG  thin2            4.0  PQ: 0 ANSI: 5
Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
implicit and explicit TPGS
Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi generic
sg64 type 0
Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS16
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-byte 
logical blocks: (41.9 GB/39.1 GiB)
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect is
off
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
enabled, read cache: enabled, supports DPO and FUA
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition timeout
set to 60 seconds

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Laurence Oberman]

On Fri, 2017-09-29 at 10:01 -0400, Laurence Oberman wrote:
> On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> > Laurence,
> > 
> > > I am testing this but its not being picked up so I want to know
> > > if
> > > I
> > > have the kernel command line wrong here.
> > > 
> > > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > > 
> > > What am I doing wrong to pass the BLIST flags.
> > 
> > This worked for me:
> > 
> > [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> > /proc/scsi/device_info
> > [root@kvm ~]# grep Linux /proc/scsi/device_info 
> > 'Linux   ' 'scsi_debug      ' 0x80000000
> > [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> > unmap_max_desc=1 write_same_length=20 lbpws=1
> > [root@kvm ~]# lsblk -D
> > NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> > sda         0      512B       5K         0
> > 
> > (With the caveat that I tweaked scsi_debug to report the UNMAP
> > parameters despite lbpu being 0).
> > 
> 
> OK, Thanks, that is working now and I pick up the correct size now.
> Its going to be very useful for these corner case array
> inconsistencies.
> 
> Tested-by: Laurence Oberman <loberman@redhat.com>
> 
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-
> Access     LIO-
> ORG  thin2            4.0  PQ: 0 ANSI: 5
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
> implicit and explicit TPGS
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
> naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi generic
> sg64 type 0
> Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set by
> kernel flag for case SD_LBP_WS16
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-
> byte 
> logical blocks: (41.9 GB/39.1 GiB)
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect
> is
> off
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
> enabled, read cache: enabled, supports DPO and FUA
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition
> timeout
> set to 60 seconds


Hi Martin

We have the code accepted for the patch above and its working fine with
echo after boot as you already know.

However I am still fighting with trying to pass this on the kernel
command line. We need to be able to do this as a dynamic method for
adding devices to the list so that the list is populated prior to the
device scan.

Using scsi_dev_flags=LIO-ORG:thin2:0x80000000 on the kernel line is
ignored and not filled in.
Its apparent to me that we have no longer have code to actually copy
the string from the kernel line after boot.

I ran some tests and added a couple of printk;s to see if we have any
capture and its NULL.

So when did this stop working, is what I am now chasing

[    1.524595] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=
[    1.524705] RHDEBUG: In scsi_init_devinfo error=0

We have this in  drivers/scsi/scsi_devinfo.c

module_param_string(dev_flags, scsi_dev_flags, sizeof(scsi_dev_flags),
0);
MODULE_PARM_DESC(dev_flags,

         "Given scsi_dev_flags=vendor:model:flags[,v:m:f] add
black/white"
         " list entries for vendor and model with an integer value of
flags"
         " to the scsi device info list");

and we have:

/**
 * scsi_init_devinfo - set up the dynamic device list.
 *
 * Description:
 *      Add command line entries from scsi_dev_flags, then add
 *      scsi_static_device_list entries to the scsi device info list.
 */
int __init scsi_init_devinfo(void)
{
#ifdef CONFIG_SCSI_PROC_FS
        struct proc_dir_entry *p;
#endif
        int error, i;

        printk("RHDEBUG:In scsi_init_devinfo
scsi_dev_flags=%s\n",scsi_dev_flags);

        error = scsi_dev_info_add_list(SCSI_DEVINFO_GLOBAL, NULL);
        printk("RHDEBUG: In scsi_init_devinfo error=%d\n",error);
        if (error) {
                printk("RHDEBUG: In scsi_init_devinfo, calling
scsi_dev_info_add_list returning with error=%d\n",error);
                return error;
        }

        error = scsi_dev_info_list_add_str(scsi_dev_flags);
        if (error) {
                printk("RHDEBUG: In scsi_init_devinfo, calling
scsi_info_list_add returning with error=%d\n",error);
                goto out;
        }

        for (i = 0; scsi_static_device_list[i].vendor; i++) {
                error = scsi_dev_info_list_add(1 /* compatibile */,
                                scsi_static_device_list[i].vendor,
                                scsi_static_device_list[i].model,
                                NULL,
                                scsi_static_device_list[i].flags);
                if (error)
                        goto out;
        }

#ifdef CONFIG_SCSI_PROC_FS
        p = proc_create("scsi/device_info", 0, NULL,
&scsi_devinfo_proc_fops);
        if (!p) {
                error = -ENOMEM;
                goto out;
        }
#endif /* CONFIG_SCSI_PROC_FS */

 out:
        if (error)
                scsi_exit_devinfo();
        return error;
}


But I fail to see where we actually copy the string off the kernel
line.

I intend to add code and test and submit a patch but first wanted to
know if its me simply missing something here.

Thanks
Laurence

Re: [PATCH] scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

[Laurence Oberman]

On Tue, 2017-10-17 at 10:26 -0400, Laurence Oberman wrote:
> On Fri, 2017-09-29 at 10:01 -0400, Laurence Oberman wrote:
> > On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> > > Laurence,
> > > 
> > > > I am testing this but its not being picked up so I want to know
> > > > if
> > > > I
> > > > have the kernel command line wrong here.
> > > > 
> > > > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > > > 
> > > > What am I doing wrong to pass the BLIST flags.
> > > 
> > > This worked for me:
> > > 
> > > [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> > > /proc/scsi/device_info
> > > [root@kvm ~]# grep Linux /proc/scsi/device_info 
> > > 'Linux   ' 'scsi_debug      ' 0x80000000
> > > [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> > > unmap_max_desc=1 write_same_length=20 lbpws=1
> > > [root@kvm ~]# lsblk -D
> > > NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> > > sda         0      512B       5K         0
> > > 
> > > (With the caveat that I tweaked scsi_debug to report the UNMAP
> > > parameters despite lbpu being 0).
> > > 
> > 
> > OK, Thanks, that is working now and I pick up the correct size now.
> > Its going to be very useful for these corner case array
> > inconsistencies.
> > 
> > Tested-by: Laurence Oberman <loberman@redhat.com>
> > 
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-
> > Access     LIO-
> > ORG  thin2            4.0  PQ: 0 ANSI: 5
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
> > implicit and explicit TPGS
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
> > naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi
> > generic
> > sg64 type 0
> > Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set
> > by
> > kernel flag for case SD_LBP_WS16
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-
> > byte 
> > logical blocks: (41.9 GB/39.1 GiB)
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect
> > is
> > off
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
> > enabled, read cache: enabled, supports DPO and FUA
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition
> > timeout
> > set to 60 seconds
> 
> 
> Hi Martin
> 
> We have the code accepted for the patch above and its working fine
> with
> echo after boot as you already know.
> 
> However I am still fighting with trying to pass this on the kernel
> command line. We need to be able to do this as a dynamic method for
> adding devices to the list so that the list is populated prior to the
> device scan.
> 
> Using scsi_dev_flags=LIO-ORG:thin2:0x80000000 on the kernel line is
> ignored and not filled in.
> Its apparent to me that we have no longer have code to actually copy
> the string from the kernel line after boot.
> 
> I ran some tests and added a couple of printk;s to see if we have any
> capture and its NULL.
> 
> So when did this stop working, is what I am now chasing
> 
> [    1.524595] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=
> [    1.524705] RHDEBUG: In scsi_init_devinfo error=0
> 
> We have this in  drivers/scsi/scsi_devinfo.c
> 
> module_param_string(dev_flags, scsi_dev_flags,
> sizeof(scsi_dev_flags),
> 0);
> MODULE_PARM_DESC(dev_flags,
> 
>          "Given scsi_dev_flags=vendor:model:flags[,v:m:f] add
> black/white"
>          " list entries for vendor and model with an integer value of
> flags"
>          " to the scsi device info list");
> 
> and we have:
> 
> /**
>  * scsi_init_devinfo - set up the dynamic device list.
>  *
>  * Description:
>  *      Add command line entries from scsi_dev_flags, then add
>  *      scsi_static_device_list entries to the scsi device info list.
>  */
> int __init scsi_init_devinfo(void)
> {
> #ifdef CONFIG_SCSI_PROC_FS
>         struct proc_dir_entry *p;
> #endif
>         int error, i;
> 
>         printk("RHDEBUG:In scsi_init_devinfo
> scsi_dev_flags=%s\n",scsi_dev_flags);
> 
>         error = scsi_dev_info_add_list(SCSI_DEVINFO_GLOBAL, NULL);
>         printk("RHDEBUG: In scsi_init_devinfo error=%d\n",error);
>         if (error) {
>                 printk("RHDEBUG: In scsi_init_devinfo, calling
> scsi_dev_info_add_list returning with error=%d\n",error);
>                 return error;
>         }
> 
>         error = scsi_dev_info_list_add_str(scsi_dev_flags);
>         if (error) {
>                 printk("RHDEBUG: In scsi_init_devinfo, calling
> scsi_info_list_add returning with error=%d\n",error);
>                 goto out;
>         }
> 
>         for (i = 0; scsi_static_device_list[i].vendor; i++) {
>                 error = scsi_dev_info_list_add(1 /* compatibile */,
>                                 scsi_static_device_list[i].vendor,
>                                 scsi_static_device_list[i].model,
>                                 NULL,
>                                 scsi_static_device_list[i].flags);
>                 if (error)
>                         goto out;
>         }
> 
> #ifdef CONFIG_SCSI_PROC_FS
>         p = proc_create("scsi/device_info", 0, NULL,
> &scsi_devinfo_proc_fops);
>         if (!p) {
>                 error = -ENOMEM;
>                 goto out;
>         }
> #endif /* CONFIG_SCSI_PROC_FS */
> 
>  out:
>         if (error)
>                 scsi_exit_devinfo();
>         return error;
> }
> 
> 
> But I fail to see where we actually copy the string off the kernel
> line.
> 
> I intend to add code and test and submit a patch but first wanted to
> know if its me simply missing something here.
> 
> Thanks
> Laurence

Answering my own post here.
As soon as I sent this Ewan emailed me explaining what I was doing
wrong.

Its working now by using 
scsi_mod.use_blk_mq=y scsi_mod.dev_flags=LIO-ORG:thin2:0x80000000

[root@segstorage1 ~]# dmesg | grep RHDEBUG
[    1.498639] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=LIO-
ORG:thin2:0x80000000
[    1.499003] RHDEBUG: In scsi_init_devinfo error=0
[    9.031071] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.202887] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.246251] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.423062] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.632754] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.781824] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   15.706504] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.254131] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.373697] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.443442] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.503806] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.582369] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.484123] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.552131] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.692909] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.975010] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.153413] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.685256] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.687920] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.692079] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.697259] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.721023] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.724256] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.728566] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16

Sorry for the noise
Thanks
Laurence