| Message ID | 1505837661-2280-1-git-send-email-emilne@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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
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.
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
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
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.
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; }