| Message ID | 20210809033217.1113444-1-arequipeno@gmail.com |
|---|---|
| Headers | show |
| Series | Add configurable block device LED triggers | expand |
On Tue, 10 Aug 2021 08:35:08 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote: > > On 8/9/21 5:43 PM, Marek Behún wrote: > > > I confess that I am not very familiar with internal blkdev API. > > > > It's mainly a matter of symbol visibility. See this thread from a few > > months ago: > > > > https://www.spinics.net/lists/linux-leds/msg18244.html > > > > Now ... my code currently lives in block/, so there isn't actually > > anything technically preventing it from iterating through the block > > devices. > > > > The reactions to Enzo's patch (which you can see in that thread) make me > > think that anything that iterates through all block devices is likely to > > be rejected, but maybe I'm reading too much into it. > > > > > > Greg / Christoph - > > > > (As you were the people who expressed disapproval of Enzo's patch to > > export block_class and disk_type ...) > > > > Can you weigh in on the acceptability of iterating through the block > > devices (searching by name) from LED trigger code within the block > > subsystem (i.e. no new symbols would need to be exported)? > > > > This would allow the trigger to implement the sysfs API that Marek and > > Pavel want. > > No idea, let's see the change first, we can never promise anything :) Hi Greg, Can't we use blkdev_get_by_path() (or blk_lookup_devt() with blkdev_get_by_dev())? This would open the block device and return a struct block_device *. When the LED trigger is disabled, it would also have to release the device. Marek
On Tue, Aug 10, 2021 at 03:38:40PM +0200, Marek Behún wrote: > On Tue, 10 Aug 2021 08:35:08 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote: > > > On 8/9/21 5:43 PM, Marek Behún wrote: > > > > I confess that I am not very familiar with internal blkdev API. > > > > > > It's mainly a matter of symbol visibility. See this thread from a few > > > months ago: > > > > > > https://www.spinics.net/lists/linux-leds/msg18244.html > > > > > > Now ... my code currently lives in block/, so there isn't actually > > > anything technically preventing it from iterating through the block > > > devices. > > > > > > The reactions to Enzo's patch (which you can see in that thread) make me > > > think that anything that iterates through all block devices is likely to > > > be rejected, but maybe I'm reading too much into it. > > > > > > > > > Greg / Christoph - > > > > > > (As you were the people who expressed disapproval of Enzo's patch to > > > export block_class and disk_type ...) > > > > > > Can you weigh in on the acceptability of iterating through the block > > > devices (searching by name) from LED trigger code within the block > > > subsystem (i.e. no new symbols would need to be exported)? > > > > > > This would allow the trigger to implement the sysfs API that Marek and > > > Pavel want. > > > > No idea, let's see the change first, we can never promise anything :) > > Hi Greg, > > Can't we use blkdev_get_by_path() (or blk_lookup_devt() with > blkdev_get_by_dev())? > This would open the block device and return a struct block_device *. > When the LED trigger is disabled, it would also have to release the > device. But what about when the device is removed from the system first? Be careful about that... Anyway, sure, try those functions, I really do not know, all I originally complained about was those exports which did not need to be exported. thanks, greg k-h
On Tue, Aug 10, 2021 at 10:55:33AM -0500, Ian Pilcher wrote: > On 8/10/21 9:48 AM, Greg KH wrote: > > But what about when the device is removed from the system first? Be > > careful about that... > > > > Anyway, sure, try those functions, I really do not know, all I > > originally complained about was those exports which did not need to be > > exported. > > Sounds good. I'll work something up. (I'm actually thinking that > class_find_device() may be the best way to go, as it grabs a reference > to the device.) There should not be anything "odd" about block devices here, just do whatever all other LED drivers do when referencing a device. thanks, greg k-h
On 8/10/21 11:24 AM, Greg KH wrote: > There should not be anything "odd" about block devices here, just do > whatever all other LED drivers do when referencing a device. AFAIK, the only LED trigger that does anything similar is the netdev trigger. It uses dev_get_by_name(), which is specific to network devices. The block subsystem doesn't appear to have any similar API, which is why Enzo submitted his patch to export block_class and disk_type back in April[1], when he wanted to do something similar. I'm basically bypassing the need to export the symbols, because my trigger code is actually in the block subsystem, rather than the LEDs subsystem. [1] https://www.spinics.net/lists/linux-leds/msg18244.html
On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote: > On 8/9/21 5:43 PM, Marek Behún wrote: >> I confess that I am not very familiar with internal blkdev API. > > It's mainly a matter of symbol visibility. See this thread from a few > months ago: > > https://www.spinics.net/lists/linux-leds/msg18244.html > > Now ... my code currently lives in block/, so there isn't actually > anything technically preventing it from iterating through the block > devices. > > The reactions to Enzo's patch (which you can see in that thread) make me > think that anything that iterates through all block devices is likely to > be rejected, but maybe I'm reading too much into it. I think the main issue with this series is that it adds a shitload of code and a hook in the absolute I/O fastpath for fricking blinkenlights. I don't think it is even worth wasting time on something this ridiculous.
On Wed, 11 Aug 2021 08:26:42 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Mon, Aug 09, 2021 at 06:50:44PM -0500, Ian Pilcher wrote: > > On 8/9/21 5:43 PM, Marek Behún wrote: > >> I confess that I am not very familiar with internal blkdev API. > > > > It's mainly a matter of symbol visibility. See this thread from a few > > months ago: > > > > https://www.spinics.net/lists/linux-leds/msg18244.html > > > > Now ... my code currently lives in block/, so there isn't actually > > anything technically preventing it from iterating through the block > > devices. > > > > The reactions to Enzo's patch (which you can see in that thread) make me > > think that anything that iterates through all block devices is likely to > > be rejected, but maybe I'm reading too much into it. > > I think the main issue with this series is that it adds a shitload of > code and a hook in the absolute I/O fastpath for fricking blinkenlights. > I don't think it is even worth wasting time on something this ridiculous. That's why I think we should do this the way the netdev trigger does. Periodically reading block_device's stats, and if they are greater, blink the LED. Marek
This is a significant rewrite of the patchset that I originally posted back on 28 July. It incorporates almost all of the suggestions that I received as feedback to my original patchset (largely because I was finally able to wrap my head around how complex LED triggers work). One thing that has not changed is that associations between block devices and LEDs are still set via an attribute on the device, rather than the LED. This is much simpler, as the device attribute only has to handle a single value (the name of the associated LED), rather than potentially handling multiple device names. More importantly, it avoids the need to iterate through all of the block devices on the system, searching by name. This was proposed fairly recently, and the reaction was not positive.[1][2] I have modeled the interface for the /sys/block/<DEVICE>/led attribute on the sysfs interface used for selecting a trigger. All available LEDs (all LEDs associated with the blkdev trigger) are shown when the attribute is read, with the currently selected LED enclosed in square brackets ([]). As before, this is all very new to me (particularly the RCU stuff), so I welcome feedback. Thanks! Changes from the original patchset: * Use a single complex LED trigger ("blkdev"), rather than multiple user-defined triggers. * Configurable blink time and interval for each LED: /sys/class/leds/<LED>/blink_{on,off} * Associated block devices linked from LED subdirectory: /sys/class/leds/<LED>/block_devices (Avoids violating the "one value per entry" sysfs rule.) * Device-LED associations set via /sys/block/<DEVICE>/led * Document all sysfs attributes in Documentation/ABI/testing (but also kept the overview doc) * Removed unnecessary [un]likely macros * Reduced number of "user error" log messages and changed level to INFO * More detailed commit messages * Add Kconfig option in final commit * Use RCU-protected pointer (rather than mutex) * No in-kernel APIs for now [1] https://www.spinics.net/lists/linux-leds/msg18256.html [2] https://www.spinics.net/lists/linux-leds/msg18261.html Ian Pilcher (10): docs: Add block device LED trigger documentation block: Add file (blk-ledtrig.c) for block device LED trigger implementation block: Add block device LED trigger fields to gendisk structure block: Add functions to set & clear block device LEDs block: Add block device sysfs attribute to set/clear/show LED block: Add activate and deactivate functions for block device LED trigger block: Add sysfs attributes to LEDs associated with blkdev trigger block: Add init function for block device LED trigger block: Blink device LED (if any) when request is sent to its driver block: Add config option to enable block device LED triggers Documentation/ABI/testing/sysfs-block | 16 + .../testing/sysfs-class-led-trigger-blkdev | 28 ++ Documentation/block/blk-ledtrig.rst | 79 +++ Documentation/block/index.rst | 1 + block/Kconfig | 8 + block/Makefile | 1 + block/blk-ledtrig.c | 469 ++++++++++++++++++ block/blk-ledtrig.h | 45 ++ block/blk-mq.c | 2 + block/genhd.c | 11 + include/linux/genhd.h | 4 + 11 files changed, 664 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev create mode 100644 Documentation/block/blk-ledtrig.rst create mode 100644 block/blk-ledtrig.c create mode 100644 block/blk-ledtrig.h