Re: [PATCH v4 4/5] block: introduce LED block device activity trigger

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>,
	linux-block@vger.kernel.org, linux-leds@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org,
	Frank Steiner <fsteiner-mail1@bio.ifi.lmu.de>,
	Dan Murphy <dmurphy@ti.com>, Jens Axboe <axboe@kernel.dk>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH v4 4/5] block: introduce LED block device activity trigger
Date: Sat, 24 Aug 2019 17:53:12 +0200	[thread overview]
Message-ID: <86309c4f-bcee-182c-369f-fcc883f379c6@gmail.com> (raw)
In-Reply-To: <CAC5umyjxkeR3rhf3XZvwkxLvc-0ENEkQfOLnk8A12Qazr9Et8w@mail.gmail.com>

On 8/23/19 6:00 PM, Akinobu Mita wrote:
> 2019年8月20日(火) 3:38 Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 8/19/19 8:22 PM, Jacek Anaszewski wrote:
>>> On 8/19/19 4:38 PM, Pavel Machek wrote:
>>>> On Sat 2019-08-17 22:07:43, Jacek Anaszewski wrote:
>>>>> On 8/17/19 4:55 PM, Pavel Machek wrote:
>>>>>> On Fri 2019-08-16 01:59:58, Akinobu Mita wrote:
>>>>>>> This allows LEDs to be controlled by block device activity.
>>>>>>>
>>>>>>> We already have ledtrig-disk (LED disk activity trigger), but the lower
>>>>>>> level disk drivers need to utilize ledtrig_disk_activity() to make the
>>>>>>> LED blink.
>>>>>>>
>>>>>>> The LED block device trigger doesn't require the lower level drivers to
>>>>>>> have any instrumentation. The activity is collected by polling the disk
>>>>>>> stats.
>>>>>>>
>>>>>>> Example:
>>>>>>>
>>>>>>> echo block-nvme0n1 > /sys/class/leds/diy/trigger
>>>>>>
>>>>>> Lets use one trigger "block" and have the device as a parameter,
>>>>>> please.
>>>>>>
>>>>>> We already have 1000 cpu triggers on 1000 cpu machines, and yes, its a
>>>>>> disaster we'll need to fix. Lets not repeat the same mistake here.
>>>>>>
>>>>>> I guess it may be slightly more work. Sorry about that.
>>>>>
>>>>> We should be able to list available block devices to set,
>>>>> so the problem would be not avoided anyway.
>>>>
>>>> Should we? We need to list triggers, but we may not list all the devices...
>>>
>>> This is similar to usbport trigger that lists available
>>> ports as files in a sub-directory. We might eventually go
>>> in this direction.
>>
>> I must withdraw this statement. This is not similar to usbport
>> trigger. The difference is that with ledtrig-block we have separate
>> triggers per each device and I am not aware if there is some centralized
>> mechanism similar to blocking_notifier_chain (usb_notifier_list
>> in drivers/usb/core/notify.c) available for block devices, that
>> would allow to gather all available block devs under common trigger.
>>
>> Moreover I remember Greg once discouraged using notifier chains
>> as they are unsafe, so we would need some other solution anyway.
> 
> I start thinking that we should implement the LED block device activity
> trigger in userspace.  The userspace application firstly activates
> one-shot LED trigger and periodically reads /sys/block/<disk>/stat and
> writes /sys/class/leds/<led>/shot if there is any disk activity.

This would suboptimal solution. I have another idea - let's get back
to the implementation of ledtrig-blk in genhd.c. We would be registering
one trigger on module initialization in a function with __init modifier.
Then we would need to add/remove triggers to the ledtrig-blk in
register_blkdev()/unregister_blkdev(). And registered triggers would
be listed in block_devs directory created by the trigger.

You can compare how drivers/usb/core/ledtrig-usbport.c maintains
similar directory of usb ports.

-- 
Best regards,
Jacek Anaszewski