Hi Bart,
Sorry to resend this, my previous reply had a stray backslash in the
headers (no idea why) that confused certain MTA, so at least Hannes did
not receive it.
I take benefit of this resend to update with the latest information, so
keep reading.
On Fri, 15 Feb 2019 07:56:11 -0800, Bart Van Assche wrote:
> On Thu, 2019-02-14 at 22:15 +0100, Jean Delvare wrote:
> > From: Hannes Reinecke <hare@xxxxxxxx>
> >
> > When evaluating the 'block limits' VPD page we need to check if
> > the 'lbpme' (logical block provisioning management enable) bit
> > is set in the READ CAPACITY (16) output.
> > If it isn't we can safely assume that we cannot use DISCARD on
> > this device.
> >
> > [JD: forward-ported to kernel v4.20]
> >
> > Signed-off-by: Hannes Reinecke <hare@xxxxxxxx>
> > Signed-off-by: Jean Delvare <jdelvare@xxxxxxxx>
> > ---
> > Hannes, please double-check that my forward-port is correct.
> >
> > drivers/scsi/sd.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -411,6 +411,13 @@ provisioning_mode_store(struct device *d
> > if (mode < 0)
> > return -EINVAL;
> >
> > + /*
> > + * If logical block provisioning isn't enabled we can only
> > + * select 'disable' here.
> > + */
> > + if (!sdkp->lbpme && mode != SD_LBP_DISABLE)
> > + return -EINVAL;
> > +
> > sd_config_discard(sdkp, mode);
> >
> > return count;
> > @@ -2942,8 +2949,10 @@ static void sd_read_block_limits(struct
> >
> > sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
> >
> > - if (!sdkp->lbpme)
> > + if (!sdkp->lbpme) {
> > + sd_config_discard(sdkp, SD_LBP_DISABLE);
> > goto out;
> > + }
> >
> > lba_count = get_unaligned_be32(&buffer[20]);
> > desc_count = get_unaligned_be32(&buffer[24]);
>
> What is the impact of this patch on SATA SSDs? Since these SSDs do not support
> logical provisioning, does this patch break trim support for these SSDs?
Thanks for your feedback. It should be noted that I am only the
messenger here, not the author of the patch. I am only trying to ensure
that upstream is not missing a fix that we have in our SUSE kernels. My
knowledge about the sd driver or storage in general is very thin, but
I'll do my best to answer questions and address issues. Any help from
the experts will be appreciated.
What makes you think that SATA SSDs do not support logical
provisioning? I added log messages to the sd driver and booted the
instrumented driver on my workstation, which has 1 SATA SSD and 2 SATA
HHD. sd_read_block_limits is called 3 times for each (not sure why).
For the SSD, sdkp->lbpme=1. For the HDD, sdkp->lbpme=0. So I think the
code should do the right thing for SATA SSD?
[ 1.351917] scsi 0:0:0:0: Direct-Access ATA Samsung SSD 850 2B6Q PQ: 0 ANSI: 5
[ 1.352975] scsi_device 0:0:0:0: sd_read_block_limits called, sdkp->lbpme=1
[ 1.366916] scsi 1:0:0:0: Direct-Access ATA ST2000VM003-1ET1 SC12 PQ: 0 ANSI: 5
[ 1.367669] scsi_device 1:0:0:0: sd_read_block_limits called, sdkp->lbpme=0
[ 1.391266] scsi 3:0:0:0: Direct-Access ATA ST1000DM003-1ER1 CC45 PQ: 0 ANSI: 5
[ 1.391854] scsi_device 3:0:0:0: sd_read_block_limits called, sdkp->lbpme=0
Of course that's only one sample. Meanwhile I noticed that sdkp->lbpme
can be read from the "thin_provisioning" sysfs attribute, without
instrumenting the kernel. So I checked on my father's system and wife's
system, and both an INTEL SSDSA2CT04 (2011) and SAMSUNG SSD 830 (2013)
have sdkp->lbpme=1. So SATA SSD seems to be safe.
One thing that worries me more is that SATA HDD have provisioning_mode
set to "full" (SD_LBP_FULL) by default, while Hannes' patch only allows
writing "disabled" (SD_LBP_DISABLE) to this sysfs attribute file when
sdkp->lbpme=0. If SD_LBP_FULL was valid at boot time, then writing that
value back should certainly be allowed. Hannes?
--
Jean Delvare
SUSE L3 Support
