| Message ID | 20210909222513.2184795-1-arequipeno@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce block device LED trigger | expand |
On Thu, 9 Sep 2021 17:25:00 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > Add files: > * ledtrig-blkdev-core.c - trigger components that are always built-in > * ledtrig-blkdev.c - trigger components that can be built as a module > * ledtrig-blkdev.h - common declarations I have never seen this done this way. Add new files once you have content for them. I think this entire proposal should be done as one patch, since it atomically adds functionality. Now I have to jump between various e-mail when I want to create a mind map of what this code is doing, and it is annoying. Please resend this as: - one patch adding sysfs docs - one patch for the rest Marek
On Thu, 9 Sep 2021 17:25:07 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > +static void blkdev_blink(const struct ledtrig_blkdev_led *const led) > +{ Why are you declaring the led variable as const? This is not needed. Sure, you do not change it, but I have never seen this being used in this way in kernel. > + unsigned long delay_on = READ_ONCE(led->blink_msec); > + unsigned long delay_off = 1; /* 0 leaves LED turned on */ > + > + led_blink_set_oneshot(led->led_dev, &delay_on, &delay_off, 0); > +} > + > +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk, > + const unsigned int generation) > +{ > + const struct block_device *const part0 = disk->gd->part0; > + const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]); > + const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE]) > + + part_stat_read(part0, ios[STAT_DISCARD]) > + + part_stat_read(part0, ios[STAT_FLUSH]); Again, yes, you do not change part0, read_ios or write_ios in this function, but this does not mean you need to declare them const. Const is good when you want to declare that a place where a pointer points to should be constant. You don't need to do it for the pointer itself, I don't see any benefit from this. > + > + if (disk->read_ios != read_ios) { > + disk->read_act = true; > + disk->read_ios = read_ios; > + } else { > + disk->read_act = false; > + } > + > + if (disk->write_ios != write_ios) { > + disk->write_act = true; > + disk->write_ios = write_ios; > + } else { > + disk->write_act = false; > + } > + > + disk->generation = generation; > +} > + > +static bool blkdev_read_mode(const enum ledtrig_blkdev_mode mode) > +{ > + return mode != LEDTRIG_BLKDEV_MODE_WO; > +} > + > +static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode) > +{ > + return mode != LEDTRIG_BLKDEV_MODE_RO; > +} It would be better to simply do the comparison where it is needed, since it is so short. These functions aren't needed, they do not shorten code in any significant way, nor do they make it more readable (in fact, they make it a little less readable). > + > +static void blkdev_process(struct work_struct *const work) You are again using const where it is not needed. In fact the work_func_t type does not have it: typedef void (*work_func_t)(struct work_struct *work); > +{ > + static unsigned int generation; > + > + struct ledtrig_blkdev_led *led; > + struct ledtrig_blkdev_link *link; > + unsigned long delay; > + > + if (!mutex_trylock(&ledtrig_blkdev_mutex)) > + goto exit_reschedule; > + > + hlist_for_each_entry(led, &ledtrig_blkdev_leds, leds_node) { > + > + hlist_for_each_entry(link, &led->disks, led_disks_node) { > + > + struct ledtrig_blkdev_disk *const disk = link->disk; > + > + if (disk->generation != generation) > + blkdev_update_disk(disk, generation); > + > + if (disk->read_act && blkdev_read_mode(led->mode)) { > + blkdev_blink(led); > + break; > + } > + > + if (disk->write_act && blkdev_write_mode(led->mode)) { > + blkdev_blink(led); > + break; > + } These two blinks should be one blink, i.e. if ((read_act && read_mode) || (write_act && write_mode)) blink(); > + } > + } > + > + ++generation; > + > + mutex_unlock(&ledtrig_blkdev_mutex); > + > +exit_reschedule: > + delay = READ_ONCE(ledtrig_blkdev_interval); > + WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay)); > +} > + > > /* > *
Dear Ian, I have tried to look into this and replied to some of your patches. There are still many things to do, and I think the reviewing would be much easier to review if you sent all the code changes as one patch (since the changes are doing an atomic change: adding support for blkdev LED trigger). Keep only the sysfs doc change in a separate patch. You are unnecessary using the const keyword in places where it is not needed and not customary for Linux kernel codebase. See in another of my replies. You are using a weird comment style, i.e. /* * * Disassociate an LED from the trigger * */ static void blkdev_deactivate(struct led_classdev *const led_dev) Please look at how functions are documented in led-class.c, for example. There are many other things I would like you to change and fix, I will comment on them once you send this proposal as two commits: one sysfs docs change, one code change. Marek
On Thu, 9 Sep 2021 17:25:07 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk, > + const unsigned int generation) > +{ > + const struct block_device *const part0 = disk->gd->part0; > + const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]); > + const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE]) > + + part_stat_read(part0, ios[STAT_DISCARD]) > + + part_stat_read(part0, ios[STAT_FLUSH]); So your code allows me to use a partition block device (like sda2) to register with the blkdev LED trigger, but when I do this, the code will disregard that I just want the LED to blink on activity on that one partition. Instead you will blink for whole sda, since you are looking at stats of only part0. Am I right? If so, this in unacceptable. The whole point of blkdev trigger is that you can reliably use it for any block device, and then it will blink the LED for that device, be it partition or whole disk. Marek
On Thu, Sep 09, 2021 at 05:25:09PM -0500, Ian Pilcher wrote: > Add /sys/class/leds/<led>/link_device sysfs attribute > > If this is the first LED associated with the device, create the > /sys/block/<disk>/blkdev_leds directory. Otherwise, increment its > reference count. > > Create symlinks in /sys/class/leds/<led>/block_devices and > /sys/block/<disk>/blkdev_leds > > If this the first device associated with *any* LED, schedule delayed work > to periodically check associated devices and blink LEDs > > Signed-off-by: Ian Pilcher <arequipeno@gmail.com> > --- > drivers/leds/trigger/ledtrig-blkdev.c | 160 ++++++++++++++++++++++++++ > 1 file changed, 160 insertions(+) > > diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c > index 6f78a9515976..26509837f037 100644 > --- a/drivers/leds/trigger/ledtrig-blkdev.c > +++ b/drivers/leds/trigger/ledtrig-blkdev.c > @@ -71,6 +71,9 @@ static unsigned int ledtrig_blkdev_interval; > static void blkdev_process(struct work_struct *const work); > static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process); > > +/* Total number of device-to-LED associations */ > +static unsigned int ledtrig_blkdev_count; > + > > /* > * > @@ -220,6 +223,162 @@ static int blkdev_activate(struct led_classdev *const led_dev) > } > > > +/* > + * > + * link_device sysfs attribute - assocate a block device with this LED > + * > + */ > + > +/* Gets or allocs & initializes the blkdev disk for a gendisk */ > +static int blkdev_get_disk(struct gendisk *const gd) > +{ > + struct ledtrig_blkdev_disk *disk; > + struct kobject *dir; > + > + if (gd->ledtrig != NULL) { > + kobject_get(gd->ledtrig->dir); When do you decrement this kobject? > + return 0; > + } > + > + disk = kmalloc(sizeof(*disk), GFP_KERNEL); > + if (disk == NULL) > + return -ENOMEM; > + > + dir = kobject_create_and_add("linked_leds", &disk_to_dev(gd)->kobj); > + if (dir == NULL) { > + kfree(disk); > + return -ENOMEM; > + } > + > + INIT_HLIST_HEAD(&disk->leds); > + disk->gd = gd; > + disk->dir = dir; > + disk->read_ios = 0; > + disk->write_ios = 0; > + > + gd->ledtrig = disk; > + > + return 0; > +} > + > +static void blkdev_put_disk(struct ledtrig_blkdev_disk *const disk) > +{ > + kobject_put(disk->dir); > + > + if (hlist_empty(&disk->leds)) { > + disk->gd->ledtrig = NULL; > + kfree(disk); This should happen in the kobject release function, not here, right? Did you try this out with removable block devices yet? thanks, greg k-h
On 9/9/21 9:09 PM, Marek Behún wrote: > I have tried to look into this and replied to some of your patches. > > There are still many things to do, and I think the reviewing would be > much easier to review if you sent all the code changes as one patch > (since the changes are doing an atomic change: adding support for blkdev > LED trigger). Keep only the sysfs doc change in a separate patch. Marek - I'll try to get a simplified version out as soon as I can. It will probably be 3 patches, because I do think that the block subsystem changes should be in a separate patch. (I agree that it will be simpler to review - not to mention easier for me to create. Past experience does tell me that there are likely some folks who will object to that format, however.) > You are unnecessary using the const keyword in places where it is not > needed and not customary for Linux kernel codebase. See in another of > my replies. I did see that. I'm a believer in declaring anything that should not change as const (to the extent that C allows). It documents the fact that the value is expected to remain unchanged throughout the function call, and it enlists the compiler to enforce it. So while it's true that they aren't necessary, they do result in code that is at least slightly less likely to be broken by future changes. > You are using a weird comment style, i.e. > /* > * > * Disassociate an LED from the trigger > * > */ > > static void blkdev_deactivate(struct led_classdev *const led_dev) > > Please look at how functions are documented in led-class.c, for example. Well ... that comment isn't documenting that function. It's intended to identify a section of the file whose contents are related. If there's a different comment style that I should be using for that purpose, please let me know. I'll respond to your other feedback separately. Thanks for taking your time on this. I really do appreciate it! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On 9/9/21 9:17 PM, Marek Behún wrote: > So your code allows me to use a partition block device (like sda2) to > register with the blkdev LED trigger, but when I do this, the code will > disregard that I just want the LED to blink on activity on that one > partition. Instead you will blink for whole sda, since you are looking > at stats of only part0. > > Am I right? You can't add partitions, only whole devices. # echo vda2 > link_device -bash: echo: write error: No such device static int blkdev_match_name(struct device *const dev, const void *const name) { return dev->type == &disk_type && sysfs_streq(dev_to_disk(dev)->disk_name, name); } Partitions fail the dev->type == &disk_type check. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Fri, 10 Sep 2021 10:09:09 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > On 9/9/21 9:17 PM, Marek Behún wrote: > > So your code allows me to use a partition block device (like sda2) to > > register with the blkdev LED trigger, but when I do this, the code will > > disregard that I just want the LED to blink on activity on that one > > partition. Instead you will blink for whole sda, since you are looking > > at stats of only part0. > > > > Am I right? > > You can't add partitions, only whole devices. But I should be able to, since partition is a block device in /dev. Any block device from /sys/class/block should be possible to add. Marek
On 9/10/21 1:48 AM, Greg KH wrote: >> +/* Gets or allocs & initializes the blkdev disk for a gendisk */ >> +static int blkdev_get_disk(struct gendisk *const gd) >> +{ >> + struct ledtrig_blkdev_disk *disk; >> + struct kobject *dir; >> + >> + if (gd->ledtrig != NULL) { >> + kobject_get(gd->ledtrig->dir); > > When do you decrement this kobject? That happens in blkdev_disk_unlink_locked() at line 399. (Also in the error path in blkdev_put_disk(), called at line 321.) Looking at this now, blkdev_disk_unlink_locked() should be calling blkdev_put_disk(), rather than calling kobject_put() directly. I'll fix that. >> +static void blkdev_put_disk(struct ledtrig_blkdev_disk *const disk) >> +{ >> + kobject_put(disk->dir); >> + >> + if (hlist_empty(&disk->leds)) { >> + disk->gd->ledtrig = NULL; >> + kfree(disk); > > This should happen in the kobject release function, not here, right? If you're referring to the kfree() call, it's freeing the ledtrig_blkdev_disk structure, not the gendisk. I use "gd" for gendisk pointers and "disk" for ledtrig_blkdev_disk pointers. > Did you try this out with removable block devices yet? Absolutely. I've tested removing both block devices and (user space) LEDs. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On 9/10/21 10:12 AM, Marek Behún wrote: > On Fri, 10 Sep 2021 10:09:09 -0500 > Ian Pilcher <arequipeno@gmail.com> wrote: >> You can't add partitions, only whole devices. > > But I should be able to, since partition is a block device in /dev. > Any block device from /sys/class/block should be possible to add. I wasn't aware that was something that you were interested in doing. This will require working with the block_device structure rather than the gendisk. One possible benefit of this change ... Assuming that block_device structures are always allocated by bdev_alloc() *and* I'm correct in thinking that this means that they are always allocated from the inode cache, then they are always zeroed out when allocated, so there won't be any need to explicitly initialize the pointer. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
Changes from v1: ================ * Use correct address for LKML. * Renamed the sysfs attributes used to manage and view the set of block devices associated ("linked") with an LED. - /sys/class/leds/<LED>/link_device to create associations - /sys/class/leds/<LED>/unlink_device to remove associations - /sys/class/leds/<LED>/linked_devices/ contains symlinks to all block devices associated with the LED - /sys/block/<DEVICE>/linked_leds (which only exists when the device is associated with at least one LED) contains symlinks to all LEDs with which the device is associated link_device and unlink_device are write-only attributes, each of which represents a single action, rather than any state. (The current state is shown by the symbolic links in the <LED>/linked_devices/ and <DEVICE>/linked_leds/ directories.) * Simplified sysfs attribute store functions. link_device and unlink_device no longer accept multiple devices at once, but this was really just an artifact of the way that sysfs repeatedly calls the store function when it doesn't "consume" all of its input, and it seemed to be confusing and unpopular anyway. * Use DEVICE_ATTR_* macros (rather than __ATTR) for the sysfs attributes. * Removed all pr_info() "system administrator error" messages. * Different minimum values for LED blink time (10 ms) and activity check interval (25 ms). Notes: ====== * Documentation for all sysfs attributes added in the first patch. * All patches build without warnings or errors when trigger is disabled, modular or built-in. V1 summary: =========== This patch series adds a new "blkdev" LED trigger for disk (or other block device) activity LEDs. It has the following functionality. * Supports all types of block devices, including virtual devices (unlike the existing disk trigger which only works with ATA devices). * LEDs can be configured to show read activity, write activity, or both. * Supports multiple devices and multiple LEDs in arbitrary many-to-many configurations. For example, it is possible to configure multiple devices with device-specific read activity LEDs and a shared write activity LED. (See Documentation/leds/ledtrig-blkdev.rst in the first patch.) * Doesn't add any overhead in the I/O path. Like the netdev LED trigger, it periodically checks the configured devices for activity and blinks its LEDs as appropriate. * Blink duration (per LED) and interval between activity checks (global) are configurable. * Requires minimal changes to the block subsystem. - Adds 1 pointer to struct gendisk, - Adds (inline function) call in device_add_disk() to ensure that the pointer is initialized to NULL (as protection against any drivers that allocate a gendisk themselves and don't use kzalloc()), and - Adds call in del_gendisk() to remove a device from the trigger when that device is being removed. These changes are all in patch #4, "block: Add block device LED trigger integrations." * The trigger can be mostly built as a module. When the trigger is modular, a small portion is built in to provide a "stub" function which can be called from del_gendisk(). The stub calls into the modular code via a function pointer when needed. The trigger also needs the ability to find gendisk's by name, which requires access to the un-exported block_class and disk_type symbols. Ian Pilcher (15): docs: Add block device (blkdev) LED trigger documentation leds: trigger: blkdev: Add build infrastructure leds: trigger: blkdev: Add functions needed by block changes block: Add block device LED trigger integrations leds: trigger: blkdev: Complete functions called by block subsys leds: trigger: blkdev: Add function to get gendisk by name leds: trigger: blkdev: Add constants and types leds: trigger: blkdev: Add stub LED trigger structure leds: trigger: blkdev: Check devices for activity and blink LEDs leds: trigger: blkdev: Add LED trigger activate function leds: trigger: blkdev: Enable linking block devices to LEDs leds: trigger: blkdev: Enable unlinking block devices from LEDs leds: trigger: blkdev: Add LED trigger deactivate function leds: trigger: blkdev: Add remaining sysfs attributes leds: trigger: blkdev: Add disk cleanup and init/exit functions Documentation/ABI/testing/sysfs-block | 9 + .../testing/sysfs-class-led-trigger-blkdev | 46 ++ Documentation/leds/index.rst | 1 + Documentation/leds/ledtrig-blkdev.rst | 138 ++++ block/genhd.c | 4 + drivers/leds/trigger/Kconfig | 18 + drivers/leds/trigger/Makefile | 2 + drivers/leds/trigger/ledtrig-blkdev-core.c | 55 ++ drivers/leds/trigger/ledtrig-blkdev.c | 686 ++++++++++++++++++ drivers/leds/trigger/ledtrig-blkdev.h | 18 + include/linux/genhd.h | 3 + include/linux/leds.h | 20 + 12 files changed, 1000 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev create mode 100644 Documentation/leds/ledtrig-blkdev.rst create mode 100644 drivers/leds/trigger/ledtrig-blkdev-core.c create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c create mode 100644 drivers/leds/trigger/ledtrig-blkdev.h base-commit: a3fa7a101dcff93791d1b1bdb3affcad1410c8c1