[PATCH] drivers/ata: print trim features at device initialization

[PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

Print trim status once at ata device initialization in form:

ataX.YZ: trim: <supported|blacklisted>, queued: <no|yes|blacklisted>, zero_after_trim: <no|yes|maybe>

Full example:

  ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
  ata1.00: NCQ Send/Recv Log not supported
  ata1.00: ATA-9: SAMSUNG MZ7GE900HMHP-000DX, EXT03Y3Q, max UDMA/133
  ata1.00: 1758174768 sectors, multi 16: LBA48 NCQ (depth 32), AA
  ata1.00: trim: supported, queued: no, zero_after_trim: maybe
  ata1.00: NCQ Send/Recv Log not supported
  ata1.00: configured for UDMA/133
  scsi 0:0:0:0: Direct-Access     ATA      SAMSUNG MZ7GE900 3Y3Q PQ: 0 ANSI: 5
  sd 0:0:0:0: [sda] 1758174768 512-byte logical blocks: (900 GB/838 GiB)
  sd 0:0:0:0: Attached scsi generic sg0 type 0
  sd 0:0:0:0: [sda] Write Protect is off
  sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
  sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
   sda: sda1 sda2
  sd 0:0:0:0: [sda] Attached SCSI disk

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 drivers/ata/libata-core.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index aaa57e0c809d..6ff33e79cfc2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2617,6 +2617,34 @@ int ata_dev_configure(struct ata_device *dev)
 			}
 		}
 
+		if (print_info && ata_id_has_trim(id)) {
+			const char *trim_status;
+			const char *trim_queued;
+			const char *trim_zero;
+
+			if (dev->horkage & ATA_HORKAGE_NOTRIM)
+				trim_status = "backlisted";
+			else
+				trim_status = "supported";
+
+			if (!ata_fpdma_dsm_supported(dev))
+				trim_queued = "no";
+			else if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
+				trim_queued = "backlisted";
+			else
+				trim_queued = "yes";
+
+			if (!ata_id_has_zero_after_trim(id))
+				trim_zero = "no";
+			else if (dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)
+				trim_zero = "yes";
+			else
+				trim_zero = "maybe";
+
+			ata_dev_info(dev, "trim: %s, queued: %s, zero_after_trim: %s\n",
+				     trim_status, trim_queued, trim_zero);
+		}
+
 		/* Check and mark DevSlp capability. Get DevSlp timing variables
 		 * from SATA Settings page of Identify Device Data Log.
 		 */

Re: [PATCH] drivers/ata: print trim features at device initialization

[Martin K. Petersen]

Konstantin,

> +			if (dev->horkage & ATA_HORKAGE_NOTRIM)
> +				trim_status = "backlisted";

blacklisted

> +			else
> +				trim_status = "supported";
> +
> +			if (!ata_fpdma_dsm_supported(dev))
> +				trim_queued = "no";
> +			else if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
> +				trim_queued = "backlisted";

ditto

> +			else
> +				trim_queued = "yes";

Why is trim_status "supported" and trim_queued/trim_zero "yes"?

> +
> +			if (!ata_id_has_zero_after_trim(id))
> +				trim_zero = "no";
> +			else if (dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)
> +				trim_zero = "yes";
> +			else
> +				trim_zero = "maybe";
> +
> +			ata_dev_info(dev, "trim: %s, queued: %s, zero_after_trim: %s\n",
> +				     trim_status, trim_queued, trim_zero);
> +		}
> +

Otherwise no particular objections. We were trying to limit noise during
boot which is why this information originally went to sysfs instead of
being printed during probe.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] drivers/ata: print trim features at device initialization

[Christoph Hellwig]

On Fri, Jun 07, 2019 at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
> Print trim status once at ata device initialization in form:

Do we really need to spam dmesg with even more ATA crap?  What about
a sysfs file that can be read on demand instead?

Re: [PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

On 07.06.2019 19:58, Martin K. Petersen wrote:
> 
> Konstantin,
> 
>> +			if (dev->horkage & ATA_HORKAGE_NOTRIM)
>> +				trim_status = "backlisted";
> 
> blacklisted

Oops. My bad.

> 
>> +			else
>> +				trim_status = "supported";
>> +
>> +			if (!ata_fpdma_dsm_supported(dev))
>> +				trim_queued = "no";
>> +			else if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
>> +				trim_queued = "backlisted";
> 
> ditto
> 
>> +			else
>> +				trim_queued = "yes";
> 
> Why is trim_status "supported" and trim_queued/trim_zero "yes"?

Hmm. This seems properties of trim, not independent features.

> 
>> +
>> +			if (!ata_id_has_zero_after_trim(id))
>> +				trim_zero = "no";
>> +			else if (dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)
>> +				trim_zero = "yes";
>> +			else
>> +				trim_zero = "maybe";
>> +
>> +			ata_dev_info(dev, "trim: %s, queued: %s, zero_after_trim: %s\n",
>> +				     trim_status, trim_queued, trim_zero);
>> +		}
>> +
> 
> Otherwise no particular objections. We were trying to limit noise during
> boot which is why this information originally went to sysfs instead of
> being printed during probe.
> 

On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07, 2019 at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
 >
 > Do we really need to spam dmesg with even more ATA crap?  What about
 > a sysfs file that can be read on demand instead?
 >

Makes sense.

Trim state is exposed for ata_device: /sys/class/ata_device/devX.Y/trim
but there is no link from scsi device to ata device so they hard to match.

I'll think about it.

Re: [PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

On 08.06.2019 12:12, Konstantin Khlebnikov wrote:
> On 07.06.2019 19:58, Martin K. Petersen wrote:
>>
>> Konstantin,
>>
>>> +            if (dev->horkage & ATA_HORKAGE_NOTRIM)
>>> +                trim_status = "backlisted";
>>
>> blacklisted
> 
> Oops. My bad.
> 
>>
>>> +            else
>>> +                trim_status = "supported";
>>> +
>>> +            if (!ata_fpdma_dsm_supported(dev))
>>> +                trim_queued = "no";
>>> +            else if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
>>> +                trim_queued = "backlisted";
>>
>> ditto
>>
>>> +            else
>>> +                trim_queued = "yes";
>>
>> Why is trim_status "supported" and trim_queued/trim_zero "yes"?
> 
> Hmm. This seems properties of trim, not independent features.
> 
>>
>>> +
>>> +            if (!ata_id_has_zero_after_trim(id))
>>> +                trim_zero = "no";
>>> +            else if (dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)
>>> +                trim_zero = "yes";
>>> +            else
>>> +                trim_zero = "maybe";
>>> +
>>> +            ata_dev_info(dev, "trim: %s, queued: %s, zero_after_trim: %s\n",
>>> +                     trim_status, trim_queued, trim_zero);
>>> +        }
>>> +
>>
>> Otherwise no particular objections. We were trying to limit noise during
>> boot which is why this information originally went to sysfs instead of
>> being printed during probe.
>>
> 
> On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07, 2019 at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
>  >
>  > Do we really need to spam dmesg with even more ATA crap?  What about
>  > a sysfs file that can be read on demand instead?
>  >
> 
> Makes sense.
> 
> Trim state is exposed for ata_device: /sys/class/ata_device/devX.Y/trim
> but there is no link from scsi device to ata device so they hard to match.
> 
> I'll think about it.

Nope. There is no obvious way to link scsi device with ata_device.
ata_device is built on top of "transport_class" and "attribute_container".
This some extremely over engineered sysfs framework used only in ata/scsi.
I don't want to touch this.

Re: [PATCH] drivers/ata: print trim features at device initialization

[James Bottomley]

On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
> > On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07, 2019
> > at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
> >  >
> >  > Do we really need to spam dmesg with even more ATA crap?  What
> > about
> >  > a sysfs file that can be read on demand instead?
> >  >
> > 
> > Makes sense.
> > 
> > Trim state is exposed for ata_device:
> > /sys/class/ata_device/devX.Y/trim
> > but there is no link from scsi device to ata device so they hard to
> > match.
> > 
> > I'll think about it.
> 
> Nope. There is no obvious way to link scsi device with ata_device.
> ata_device is built on top of "transport_class" and
> "attribute_container".
> This some extremely over engineered sysfs framework used only in
> ata/scsi. I don't want to touch this.

You don't need to know any of that.  The problem is actually when the
ata transport classes were first created, the devices weren't properly
parented.  What should have happened, like every other transport class,
is that the devices should have descended down to the scsi device as
the leaf in an integrated fashion.  Instead, what we seem to have is
three completely separate trees.

So if you look at a SAS device, you see from the pci device:

host2/port-2:0/end_device-2:0/target2:0:0/2:0:0:0/block/sdb/sdb1

But if you look at a SATA device, you see three separate paths:

ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
ata3/link3/dev3.0/ata_device/dev3.0
ata3/ata_port/ata3

Instead of an integrated tree

Unfortunately, this whole thing is unfixable now.  If I integrate the
tree properly, the separate port and link directories will get subsumed
and we won't be able to recover them with judicious linking so scripts
relying on them will break.  The best we can probably do is add
additional links with what we have.

To follow the way we usually do it, there should be a link from the ata
device to the scsi target, but that wouldn't help you find the "trim"
files, so it sounds like you want a link from the scsi device to the ata device, which would?

James

Re: [PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

On 10.06.2019 0:37, James Bottomley wrote:
> On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
>>> On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07, 2019
>>> at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
>>>   >
>>>   > Do we really need to spam dmesg with even more ATA crap?  What
>>> about
>>>   > a sysfs file that can be read on demand instead?
>>>   >
>>>
>>> Makes sense.
>>>
>>> Trim state is exposed for ata_device:
>>> /sys/class/ata_device/devX.Y/trim
>>> but there is no link from scsi device to ata device so they hard to
>>> match.
>>>
>>> I'll think about it.
>>
>> Nope. There is no obvious way to link scsi device with ata_device.
>> ata_device is built on top of "transport_class" and
>> "attribute_container".
>> This some extremely over engineered sysfs framework used only in
>> ata/scsi. I don't want to touch this.
> 
> You don't need to know any of that.  The problem is actually when the
> ata transport classes were first created, the devices weren't properly
> parented.  What should have happened, like every other transport class,
> is that the devices should have descended down to the scsi device as
> the leaf in an integrated fashion.  Instead, what we seem to have is
> three completely separate trees.
> 
> So if you look at a SAS device, you see from the pci device:
> 
> host2/port-2:0/end_device-2:0/target2:0:0/2:0:0:0/block/sdb/sdb1
> 
> But if you look at a SATA device, you see three separate paths:
> 
> ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
> ata3/link3/dev3.0/ata_device/dev3.0
> ata3/ata_port/ata3
> 
> Instead of an integrated tree
> 
> Unfortunately, this whole thing is unfixable now.  If I integrate the
> tree properly, the separate port and link directories will get subsumed
> and we won't be able to recover them with judicious linking so scripts
> relying on them will break.  The best we can probably do is add
> additional links with what we have.
> 
> To follow the way we usually do it, there should be a link from the ata
> device to the scsi target, but that wouldn't help you find the "trim"
> files, so it sounds like you want a link from the scsi device to the ata device, which would?

Yes, I'm talking about link from scsi device to leaf ata_device node.

In libata scsi_device has one to one relation with ata_device.
So making link like /sys/class/block/sda/device/ata_device should be possible easy.
But I haven't found implicit reference from struct ata_device to ata_device in sysfs.

In simplest ahci case whole path looks like:
/sys/devices/pci0000:00/0000:00:1f.2/ata1/link1/dev1.0/ata_device/dev1.0
|______________________|__ata_host__|port|link_|_tdev_|___ata_device___|


/sys/class/ata_device/dev1.0 points directly to leaf ata_device
While in struct ata_device tdev is different intermediate node.

It would be nice merge tdev and ata_device into one node, or at least embed leaf
struct device into struct ata_device too.

As I see ata registers only "transport" device while scsi transport template
magically matches it and creates actual ata device of ata_dev_class.
I see no reason for this complexity. Why ata host couldn't enumerate and
register all these devices explicitly?

Re: [PATCH] drivers/ata: print trim features at device initialization

[James Bottomley]

On Mon, 2019-06-10 at 10:49 +0300, Konstantin Khlebnikov wrote:
> On 10.06.2019 0:37, James Bottomley wrote:
> > On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
> > > > On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07,
> > > > 2019
> > > > at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
> > > >   >
> > > >   > Do we really need to spam dmesg with even more ATA
> > > > crap?  What
> > > > about
> > > >   > a sysfs file that can be read on demand instead?
> > > >   >
> > > > 
> > > > Makes sense.
> > > > 
> > > > Trim state is exposed for ata_device:
> > > > /sys/class/ata_device/devX.Y/trim
> > > > but there is no link from scsi device to ata device so they
> > > > hard to match.
> > > > 
> > > > I'll think about it.
> > > 
> > > Nope. There is no obvious way to link scsi device with
> > > ata_device. ata_device is built on top of "transport_class" and
> > > "attribute_container". This some extremely over engineered sysfs
> > > framework used only in ata/scsi. I don't want to touch this.
> > 
> > You don't need to know any of that.  The problem is actually when
> > the ata transport classes were first created, the devices weren't
> > properly parented.  What should have happened, like every other
> > transport class, is that the devices should have descended down to
> > the scsi device as the leaf in an integrated fashion.  Instead,
> > what we seem to have is three completely separate trees.
> > 
> > So if you look at a SAS device, you see from the pci device:
> > 
> > host2/port-2:0/end_device-2:0/target2:0:0/2:0:0:0/block/sdb/sdb1
> > 
> > But if you look at a SATA device, you see three separate paths:
> > 
> > ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
> > ata3/link3/dev3.0/ata_device/dev3.0
> > ata3/ata_port/ata3
> > 
> > Instead of an integrated tree
> > 
> > Unfortunately, this whole thing is unfixable now.  If I integrate
> > the tree properly, the separate port and link directories will get
> > subsumed and we won't be able to recover them with judicious
> > linking so scripts relying on them will break.  The best we can
> > probably do is add additional links with what we have.
> > 
> > To follow the way we usually do it, there should be a link from the
> > ata device to the scsi target, but that wouldn't help you find the
> > "trim" files, so it sounds like you want a link from the scsi
> > device to the ata device, which would?
> 
> Yes, I'm talking about link from scsi device to leaf ata_device node.
> 
> In libata scsi_device has one to one relation with ata_device.
> So making link like /sys/class/block/sda/device/ata_device should be
> possible easy.
> But I haven't found implicit reference from struct ata_device to
> ata_device in sysfs.

If that's all you want, it is pretty simple modulo the fact we can only
get at the tdev, not the lower transport device, which is what you
want, but at least it's linear from the symlink.

The attached patch should do this.

Now I see this for my non-sas device:

# ls -l /sys/class/scsi_device/3\:0\:0\:0/device/ata_device
lrwxrwxrwx 1 root root 0 Jun 10 13:39 /sys/class/scsi_device/3:0:0:0/device/ata_device -> ../../../link3/dev3.0

James

---

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 391ac0503dc0..b644336a6d65 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4576,7 +4576,20 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
 						 NULL);
 			if (!IS_ERR(sdev)) {
+				int r;
+
 				dev->sdev = sdev;
+				/* this is a sysfs stupidity: we don't
+				 * care if the link actually gets
+				 * created: there's no error handling
+				 * for failure and we continue on
+				 * regardless, but we have to discard
+				 * the return value like this to
+				 * defeat unused result checking */
+				r = sysfs_create_link(&sdev->sdev_gendev.kobj,
+						      &dev->tdev.kobj,
+						      "ata_device");
+				(void)r;
 				scsi_device_put(sdev);
 			} else {
 				dev->sdev = NULL;
@@ -4703,6 +4716,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
 		ata_dev_info(dev, "detaching (SCSI %s)\n",
 			     dev_name(&sdev->sdev_gendev));
 
+		sysfs_remove_link(&sdev->sdev_gendev.kobj, "ata_device");
 		scsi_remove_device(sdev);
 		scsi_device_put(sdev);
 	}

Re: [PATCH] drivers/ata: print trim features at device initialization

[Konstantin Khlebnikov]

On 11.06.2019 1:48, James Bottomley wrote:
> On Mon, 2019-06-10 at 10:49 +0300, Konstantin Khlebnikov wrote:
>> On 10.06.2019 0:37, James Bottomley wrote:
>>> On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
>>>>> On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun 07,
>>>>> 2019
>>>>> at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
>>>>>    >
>>>>>    > Do we really need to spam dmesg with even more ATA
>>>>> crap?  What
>>>>> about
>>>>>    > a sysfs file that can be read on demand instead?
>>>>>    >
>>>>>
>>>>> Makes sense.
>>>>>
>>>>> Trim state is exposed for ata_device:
>>>>> /sys/class/ata_device/devX.Y/trim
>>>>> but there is no link from scsi device to ata device so they
>>>>> hard to match.
>>>>>
>>>>> I'll think about it.
>>>>
>>>> Nope. There is no obvious way to link scsi device with
>>>> ata_device. ata_device is built on top of "transport_class" and
>>>> "attribute_container". This some extremely over engineered sysfs
>>>> framework used only in ata/scsi. I don't want to touch this.
>>>
>>> You don't need to know any of that.  The problem is actually when
>>> the ata transport classes were first created, the devices weren't
>>> properly parented.  What should have happened, like every other
>>> transport class, is that the devices should have descended down to
>>> the scsi device as the leaf in an integrated fashion.  Instead,
>>> what we seem to have is three completely separate trees.
>>>
>>> So if you look at a SAS device, you see from the pci device:
>>>
>>> host2/port-2:0/end_device-2:0/target2:0:0/2:0:0:0/block/sdb/sdb1
>>>
>>> But if you look at a SATA device, you see three separate paths:
>>>
>>> ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
>>> ata3/link3/dev3.0/ata_device/dev3.0
>>> ata3/ata_port/ata3
>>>
>>> Instead of an integrated tree
>>>
>>> Unfortunately, this whole thing is unfixable now.  If I integrate
>>> the tree properly, the separate port and link directories will get
>>> subsumed and we won't be able to recover them with judicious
>>> linking so scripts relying on them will break.  The best we can
>>> probably do is add additional links with what we have.
>>>
>>> To follow the way we usually do it, there should be a link from the
>>> ata device to the scsi target, but that wouldn't help you find the
>>> "trim" files, so it sounds like you want a link from the scsi
>>> device to the ata device, which would?
>>
>> Yes, I'm talking about link from scsi device to leaf ata_device node.
>>
>> In libata scsi_device has one to one relation with ata_device.
>> So making link like /sys/class/block/sda/device/ata_device should be
>> possible easy.
>> But I haven't found implicit reference from struct ata_device to
>> ata_device in sysfs.
> 
> If that's all you want, it is pretty simple modulo the fact we can only
> get at the tdev, not the lower transport device, which is what you
> want, but at least it's linear from the symlink.
> 
> The attached patch should do this.
> 
> Now I see this for my non-sas device:
> 
> # ls -l /sys/class/scsi_device/3\:0\:0\:0/device/ata_device
> lrwxrwxrwx 1 root root 0 Jun 10 13:39 /sys/class/scsi_device/3:0:0:0/device/ata_device -> ../../../link3/dev3.0

I've tried this too. Such link is not very useful,
because attribute 'trim' is deeper and suffix path isn't constant:

/sys/class/block/sda/device/ata_device/ata_device/dev1.0/trim

while I expect something like

/sys/class/block/sda/device/ata_device/trim

> 
> James
> 
> ---
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 391ac0503dc0..b644336a6d65 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4576,7 +4576,20 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>   			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>   						 NULL);
>   			if (!IS_ERR(sdev)) {
> +				int r;
> +
>   				dev->sdev = sdev;
> +				/* this is a sysfs stupidity: we don't
> +				 * care if the link actually gets
> +				 * created: there's no error handling
> +				 * for failure and we continue on
> +				 * regardless, but we have to discard
> +				 * the return value like this to
> +				 * defeat unused result checking */
> +				r = sysfs_create_link(&sdev->sdev_gendev.kobj,
> +						      &dev->tdev.kobj,
> +						      "ata_device");
> +				(void)r;
>   				scsi_device_put(sdev);
>   			} else {
>   				dev->sdev = NULL;
> @@ -4703,6 +4716,7 @@ static void ata_scsi_remove_dev(struct ata_device *dev)
>   		ata_dev_info(dev, "detaching (SCSI %s)\n",
>   			     dev_name(&sdev->sdev_gendev));
>   
> +		sysfs_remove_link(&sdev->sdev_gendev.kobj, "ata_device");
>   		scsi_remove_device(sdev);
>   		scsi_device_put(sdev);
>   	}
> 

Re: [PATCH] drivers/ata: print trim features at device initialization

[James Bottomley]

On Fri, 2019-06-14 at 16:49 +0300, Konstantin Khlebnikov wrote:
> 
> On 11.06.2019 1:48, James Bottomley wrote:
> > On Mon, 2019-06-10 at 10:49 +0300, Konstantin Khlebnikov wrote:
> > > On 10.06.2019 0:37, James Bottomley wrote:
> > > > On Sat, 2019-06-08 at 17:13 +0300, Konstantin Khlebnikov wrote:
> > > > > > On 08.06.2019 11:25, Christoph Hellwig wrote:> On Fri, Jun
> > > > > > 07,
> > > > > > 2019
> > > > > > at 10:34:39AM +0300, Konstantin Khlebnikov wrote:
> > > > > >    >
> > > > > >    > Do we really need to spam dmesg with even more ATA
> > > > > > crap?  What
> > > > > > about
> > > > > >    > a sysfs file that can be read on demand instead?
> > > > > >    >
> > > > > > 
> > > > > > Makes sense.
> > > > > > 
> > > > > > Trim state is exposed for ata_device:
> > > > > > /sys/class/ata_device/devX.Y/trim
> > > > > > but there is no link from scsi device to ata device so they
> > > > > > hard to match.
> > > > > > 
> > > > > > I'll think about it.
> > > > > 
> > > > > Nope. There is no obvious way to link scsi device with
> > > > > ata_device. ata_device is built on top of "transport_class"
> > > > > and
> > > > > "attribute_container". This some extremely over engineered
> > > > > sysfs
> > > > > framework used only in ata/scsi. I don't want to touch this.
> > > > 
> > > > You don't need to know any of that.  The problem is actually
> > > > when
> > > > the ata transport classes were first created, the devices
> > > > weren't
> > > > properly parented.  What should have happened, like every other
> > > > transport class, is that the devices should have descended down
> > > > to
> > > > the scsi device as the leaf in an integrated fashion.  Instead,
> > > > what we seem to have is three completely separate trees.
> > > > 
> > > > So if you look at a SAS device, you see from the pci device:
> > > > 
> > > > host2/port-2:0/end_device-
> > > > 2:0/target2:0:0/2:0:0:0/block/sdb/sdb1
> > > > 
> > > > But if you look at a SATA device, you see three separate paths:
> > > > 
> > > > ata3/host3/target3\:0\:0/3\:0\:0\:0/block/sda/sda1
> > > > ata3/link3/dev3.0/ata_device/dev3.0
> > > > ata3/ata_port/ata3
> > > > 
> > > > Instead of an integrated tree
> > > > 
> > > > Unfortunately, this whole thing is unfixable now.  If I
> > > > integrate
> > > > the tree properly, the separate port and link directories will
> > > > get
> > > > subsumed and we won't be able to recover them with judicious
> > > > linking so scripts relying on them will break.  The best we can
> > > > probably do is add additional links with what we have.
> > > > 
> > > > To follow the way we usually do it, there should be a link from
> > > > the
> > > > ata device to the scsi target, but that wouldn't help you find
> > > > the
> > > > "trim" files, so it sounds like you want a link from the scsi
> > > > device to the ata device, which would?
> > > 
> > > Yes, I'm talking about link from scsi device to leaf ata_device
> > > node.
> > > 
> > > In libata scsi_device has one to one relation with ata_device.
> > > So making link like /sys/class/block/sda/device/ata_device should
> > > be
> > > possible easy.
> > > But I haven't found implicit reference from struct ata_device to
> > > ata_device in sysfs.
> > 
> > If that's all you want, it is pretty simple modulo the fact we can
> > only
> > get at the tdev, not the lower transport device, which is what you
> > want, but at least it's linear from the symlink.
> > 
> > The attached patch should do this.
> > 
> > Now I see this for my non-sas device:
> > 
> > # ls -l /sys/class/scsi_device/3\:0\:0\:0/device/ata_device
> > lrwxrwxrwx 1 root root 0 Jun 10 13:39
> > /sys/class/scsi_device/3:0:0:0/device/ata_device ->
> > ../../../link3/dev3.0
> 
> I've tried this too. Such link is not very useful,
> because attribute 'trim' is deeper and suffix path isn't constant:
> 
> /sys/class/block/sda/device/ata_device/ata_device/dev1.0/trim
> 
> while I expect something like
> 
> /sys/class/block/sda/device/ata_device/trim

It provides you with an unambiguous way of finding the correct trim
file.

The problem with trying to lower the level of the link is that all the
devices below the one I linked to are transport class devices meaning
they're not visible at the point the link needs to be created.  That's
not to say some additional transport class magic couldn't be done, but
it would require pretty extensive code changes in drivers/base because
none of the current constructor functions carries additional
information, which is necessary to carry the link.

James