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

2017-09-27 Thread 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

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

2017-09-27 Thread 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:emi...@redhat.com] 
Sent: Wednesday, September 27, 2017 12:28 PM
To: Martin K. Petersen <martin.peter...@oracle.com>
Cc: linux-scsi@vger.kernel.org; Knight, Frederick <frederick.kni...@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

2017-09-27 Thread 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

2017-09-25 Thread 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

2017-09-19 Thread 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 

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