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

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 8/27/19 4:03 PM, Akinobu Mita wrote:
> 2019年8月25日(日) 0:53 Jacek Anaszewski <jacek.anaszewski@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.
> 
> It could be possible, but I have yet another idea.  What about introducing
> /proc/led-triggers and /sys/class/leds/<led>/current-trigger?
> The /sys/class/leds/<led>/trigger will be obsoleted by these two files.
> 
> The /proc/led-triggers is read only and no PAGE_SIZE limitation by the
> seq_file interface.  So we can list all triggers in this file.
> 
> The /sys/class/leds/<led>/current-trigger is almost identical to
> /sys/class/leds/<led>/trigger.  The only difference is that
> 'current-trigger' only shows the current trigger name.

There's not need to come up with yet another trigger interface.
We just need to convert sysfs trigger attribute type to binary.

-- 
Best regards,
Jacek Anaszewski