[PATCH] sd: read unmap block limits even if lbpme=0

[PATCH] sd: read unmap block limits even if lbpme=0

[tom.ty89]

From: Tom Yan <tom.ty89@gmail.com>

Some devices may not be decent enough to report lbpme bit properly
even when they do support unmap and report relevant information
in the block limits and logical block provisioning VPDs properly
(Namely, ASMedia ASM1351, a UAS-SATA bridge). One of the reasons
is, lbpme=1 is not a requirement for "DeleteNotify" in Windows to
be activated:

[root@archlinux ~]# sg_readcap -l /dev/sda | grep lb
   Logical block provisioning: lbpme=0, lbprz=0
[root@archlinux ~]# sg_vpd -p lbpv /dev/sda | grep \(LB
  Unmap command supported (LBPU): 1
  Write same (16) with unmap bit supported (LBWS): 0
  Write same (10) with unmap bit supported (LBWS10): 0
  Logical block provisioning read zeros (LBPRZ): 0
[root@archlinux ~]# sg_vpd -p bl /dev/sda | grep -i unmap
  Maximum unmap LBA count: 4194240
  Maximum unmap block descriptor count: 1
  Optimal unmap granularity: 1
  Unmap granularity alignment valid: 0
  Unmap granularity alignment: 0

While there may be a point to retain the "strict policy" on this,
by not configuring discard for such device automatically, there
is little reason not to read the relevant data from the VPD, for
users are allowed to configure discard for a device via the
provisioning_mode sysfs file.

While discard_max_bytes can be changed manually, it is better
if the value would be limited by a discard_max_hw_bytes that is
set from the hardware limit that is reported in the VPD.

Before this commit:
[root@archlinux ~]# cat (...)/provisioning_mode
full
[root@archlinux ~]# grep . /sys/block/sda/queue/discard_*
/sys/block/sda/queue/discard_granularity:0
/sys/block/sda/queue/discard_max_bytes:0
/sys/block/sda/queue/discard_max_hw_bytes:0
/sys/block/sda/queue/discard_zeroes_data:0
[root@archlinux ~]# echo -n unmap > (...)/provisioning_mode
[root@archlinux ~]# grep . /sys/block/sda/queue/discard_*
/sys/block/sda/queue/discard_granularity:512
/sys/block/sda/queue/discard_max_bytes:4294966784
/sys/block/sda/queue/discard_max_hw_bytes:4294966784
/sys/block/sda/queue/discard_zeroes_data:0

After this commit:
[root@archlinux ~]# cat (...)/provisioning_mode
full
[root@archlinux ~]# grep . /sys/block/sda/queue/discard_*
/sys/block/sda/queue/discard_granularity:0
/sys/block/sda/queue/discard_max_bytes:0
/sys/block/sda/queue/discard_max_hw_bytes:0
/sys/block/sda/queue/discard_zeroes_data:0
[root@archlinux ~]# echo -n unmap > (...)/provisioning_mode
[root@archlinux ~]# grep . /sys/block/sda/queue/discard_*
/sys/block/sda/queue/discard_granularity:512
/sys/block/sda/queue/discard_max_bytes:2147450880
/sys/block/sda/queue/discard_max_hw_bytes:2147450880
/sys/block/sda/queue/discard_zeroes_data:0

Signed-off-by: Tom Yan <tom.ty89@gmail.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bea36adeee17..ea9e6fc76b63 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2883,9 +2883,6 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
 
-		if (!sdkp->lbpme)
-			goto out;
-
 		lba_count = get_unaligned_be32(&buffer[20]);
 		desc_count = get_unaligned_be32(&buffer[24]);
 
@@ -2898,6 +2895,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 			sdkp->unmap_alignment =
 				get_unaligned_be32(&buffer[32]) & ~(1 << 31);
 
+		if (!sdkp->lbpme)
+			goto out;
+
 		if (!sdkp->lbpvpd) { /* LBP VPD page not provided */
 
 			if (sdkp->max_unmap_blocks)
-- 
2.14.1

Re: [PATCH] sd: read unmap block limits even if lbpme=0

[Martin K. Petersen]

Tom,

> Some devices may not be decent enough to report lbpme bit properly
> even when they do support unmap and report relevant information in the
> block limits and logical block provisioning VPDs properly (Namely,
> ASMedia ASM1351, a UAS-SATA bridge). One of the reasons is, lbpme=1 is
> not a requirement for "DeleteNotify" in Windows to be activated:

I have to confess I'm wary of interpreting values reported by a device
that messes up setting the single bit flag that enables the feature.

Before I entertain taking your patch I'd like to take a closer look to
make sure everything is gated by sdkp->lbpme.

However, instead of relying on UNMAP, does this bridge support WRITE
SAME with the UNMAP bit? Because in that case you should be able to set
provisioning_mode to WS10 or WS16 and then adjust max_write_same_blocks.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sd: read unmap block limits even if lbpme=0

[Tom Yan]

On 17 August 2017 at 10:11, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
> Tom,
>
> I have to confess I'm wary of interpreting values reported by a device
> that messes up setting the single bit flag that enables the feature.
>

FWIW, this implementation in particular works pretty well as far as I
can tell. The values reported on the VPDs are sensical. You can see
that it advertises the same limit as the libata SATL does, which is
(65335 * 512 / 8) blocks.

Also I think it could not hurt anyway, because the scsi disk driver
would use SD_MAX_WS16_BLOCKS if the reported limit is larger, so it's
essentially a conservative thing to do.

Not to mention that if someone is determined to enable it on linux,
the limits reported in the VPD would most likely be the ones that the
he/she would want to start with.

>
> Before I entertain taking your patch I'd like to take a closer look to
> make sure everything is gated by sdkp->lbpme.
>

Sure take your time. Look forward to your favorable reply.

>
> However, instead of relying on UNMAP, does this bridge support WRITE
> SAME with the UNMAP bit? Because in that case you should be able to set
> provisioning_mode to WS10 or WS16 and then adjust max_write_same_blocks.
>

Nope, neither does it report a maximum write same length. Again, IIRC,
Windows does not support WRITE SAME with UNMAP bit set but only the
UNMAP command.

> --
> Martin K. Petersen      Oracle Linux Engineering