mbox series

[00/18] Introduce block device LED trigger

Message ID 20210903204548.2745354-1-arequipeno@gmail.com
Headers show
Series Introduce block device LED trigger | expand

Message

Ian Pilcher Sept. 3, 2021, 8:45 p.m. UTC
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.

NOTES:

* This patch series applies cleanly to the linux-block and linux-next
  (20210903) trees.  All patches other than the block subsystem patch
  (patch #4) apply cleanly to the linux-leds tree.

* All patches compile (modulo warnings) with the trigger disabled,
  modular, or built-in.

Ian Pilcher (18):
  docs: Add block device (blkdev) LED trigger documentation
  ledtrig-blkdev: Add build infra for block device LED trigger
  ledtrig-blkdev: Add function placeholders needed by block changes
  block: Add block device LED trigger integrations
  ledtrig-blkdev: Implement functions called from block subsystem
  ledtrig-blkdev: Add function to get gendisk by name
  ledtrig-blkdev: Add constants, data types, and global variables
  ledtrig-blkdev: Add miscellaneous helper functions
  ledtrig-blkdev: Periodically check devices for activity & blink LEDs
  ledtrig-blkdev: Add function to associate the trigger with an LED
  ledtrig-blkdev: Add function to associate a device with an LED
  ledtrig-blkdev: Add function to remove LED/device association
  ledtrig-blkdev: Add function to disassociate a device from all LEDs
  ledtrig-blkdev: Add function to disassociate an LED from the trigger
  ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices
  ledtrig-blkdev: Add blink_time & interval sysfs attributes
  ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue
  ledtrig-blkdev: Add initialization & exit functions

 Documentation/ABI/testing/sysfs-block         |   9 +
 .../testing/sysfs-class-led-trigger-blkdev    |  48 ++
 Documentation/leds/index.rst                  |   1 +
 Documentation/leds/ledtrig-blkdev.rst         | 144 ++++
 block/genhd.c                                 |   4 +
 drivers/leds/trigger/Kconfig                  |  18 +
 drivers/leds/trigger/Makefile                 |   2 +
 drivers/leds/trigger/ledtrig-blkdev-core.c    |  78 ++
 drivers/leds/trigger/ledtrig-blkdev.c         | 767 ++++++++++++++++++
 drivers/leds/trigger/ledtrig-blkdev.h         |  27 +
 include/linux/genhd.h                         |   3 +
 include/linux/leds.h                          |  20 +
 12 files changed, 1121 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

Comments

Greg Kroah-Hartman Sept. 4, 2021, 5:57 a.m. UTC | #1
On Fri, Sep 03, 2021 at 03:45:47PM -0500, Ian Pilcher wrote:
> +static ssize_t blkdev_mode_store(struct device *const dev,

> +				 struct device_attribute *const attr,

> +				 const char *const buf, const size_t count)

> +{

> +	struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);

> +	const char *const mode_name = blkdev_skip_space(buf);

> +	const char *const endp = blkdev_find_space(mode_name);

> +	const ptrdiff_t name_len = endp - mode_name;	/* always >= 0 */

> +	enum ledtrig_blkdev_mode mode;

> +

> +	if (name_len == 0) {

> +		pr_info("blkdev LED: empty mode\n");

> +		return -EINVAL;

> +	}

> +

> +	for (mode = LEDTRIG_BLKDEV_MODE_RO;

> +				mode <= LEDTRIG_BLKDEV_MODE_RW; ++mode) {

> +

> +		if (ledtrig_blkdev_streq(blkdev_modes[mode].name,

> +						mode_name, name_len)) {

> +			WRITE_ONCE(led->mode, mode);

> +			return count;

> +		}

> +	}

> +

> +	pr_info("blkdev LED: invalid mode (%.*s)\n", (int)name_len, mode_name);


Please do not use pr_* calls in a driver when you have a struct device.

Also, you are now allowing any user to spam the kernel log, this shold
be a dev_dbg() call at the most, if it is even needed at all.  Same for
the other pr_info() call in this function, please remove them all.

thanks,

greg k-h
Greg Kroah-Hartman Sept. 4, 2021, 6:01 a.m. UTC | #2
On Fri, Sep 03, 2021 at 03:45:39PM -0500, Ian Pilcher wrote:
> Use a delayed workqueue to periodically check configured block devices for

> activity since the last check.  Blink LEDs associated with devices on which

> the configured type of activity (read/write) has occurred.

> 

> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>

> ---

>  drivers/leds/trigger/ledtrig-blkdev.c | 88 +++++++++++++++++++++++++++

>  1 file changed, 88 insertions(+)

> 

> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c

> index 1f319529c3be..37ba9bb3542e 100644

> --- a/drivers/leds/trigger/ledtrig-blkdev.c

> +++ b/drivers/leds/trigger/ledtrig-blkdev.c

> @@ -7,7 +7,9 @@

>   */

>  

>  #include <linux/ctype.h>

> +#include <linux/leds.h>

>  #include <linux/module.h>

> +#include <linux/part_stat.h>

>  

>  #include "ledtrig-blkdev.h"

>  

> @@ -68,6 +70,10 @@ static unsigned int ledtrig_blkdev_count;

>  /* How often to check for drive activity - in jiffies */

>  static unsigned int ledtrig_blkdev_interval;

>  

> +/* Delayed work used to periodically check for activity & blink LEDs */

> +static void blkdev_process(struct work_struct *const work);

> +static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process);

> +

>  

>  /*

>   *

> @@ -110,3 +116,85 @@ static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode)

>  {

>  	return mode != LEDTRIG_BLKDEV_MODE_RO;

>  }

> +

> +

> +/*

> + *

> + *	Periodically check for device acitivity and blink LEDs

> + *

> + */

> +

> +static void blkdev_blink(const struct ledtrig_blkdev_led *const led)

> +{

> +	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]);

> +

> +	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 void blkdev_process(struct work_struct *const 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;

> +			}

> +		}

> +	}

> +

> +	++generation;

> +

> +	mutex_unlock(&ledtrig_blkdev_mutex);

> +

> +exit_reschedule:

> +	delay = READ_ONCE(ledtrig_blkdev_interval);

> +	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));


You just rebooted a machine if it hit this :(

Please never use WARN_ON() in new code unless the machine is really
broken and you can not do anything else here.

thanks,

greg k-h
Pavel Machek Sept. 4, 2021, 6:35 a.m. UTC | #3
Hi!

> This patch series adds a new "blkdev" LED trigger for disk (or other block

> device) activity LEDs.

> 

> It has the following functionality.


Correct address for lkml is linux-kernel@vger, not linux@vger.

Best regards,
							Pavel
-- 
http://www.livejournal.com/~pavelmachek
Ian Pilcher Sept. 4, 2021, 9:01 p.m. UTC | #4
On 9/4/21 12:57 AM, Greg KH wrote:
> Also, you are now allowing any user to spam the kernel log, this shold

> be a dev_dbg() call at the most, if it is even needed at all.  Same for

> the other pr_info() call in this function, please remove them all.


Greg -

A bit of a "philosophical" question, if I may ...

Is allowing the root user to "spam" the kernel log really a concern?
(The permissions on the attribute don't allow non-root users to write
to it.)

As a system administrator working with a sysfs API (or writing udev
rules to do so), I know that I appreciate having a meaningful message in
the log when something doesn't work.

Given that only the root user can trigger these messages, would you be
OK with dev_info()?

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================
Ian Pilcher Sept. 5, 2021, 2:39 p.m. UTC | #5
On 9/4/21 1:01 AM, Greg KH wrote:
> Please never use WARN_ON() in new code unless the machine is really

> broken and you can not do anything else here.


Wait what?  I thought that was BUG_ON.

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================
Greg Kroah-Hartman Sept. 5, 2021, 2:50 p.m. UTC | #6
On Sat, Sep 04, 2021 at 04:01:56PM -0500, Ian Pilcher wrote:
> On 9/4/21 12:57 AM, Greg KH wrote:

> > Also, you are now allowing any user to spam the kernel log, this shold

> > be a dev_dbg() call at the most, if it is even needed at all.  Same for

> > the other pr_info() call in this function, please remove them all.

> 

> Greg -

> 

> A bit of a "philosophical" question, if I may ...

> 

> Is allowing the root user to "spam" the kernel log really a concern?


Yes.

> (The permissions on the attribute don't allow non-root users to write

> to it.)


Ah, but that was not obvious :)

> As a system administrator working with a sysfs API (or writing udev

> rules to do so), I know that I appreciate having a meaningful message in

> the log when something doesn't work.


That's fine, but do not allow a "normal" user to do so please.

> Given that only the root user can trigger these messages, would you be

> OK with dev_info()?


For a sysfs file failure, use dev_err().  If things are working
properly, your kernel code should be totally silent.

thanks,

greg k-h
Greg Kroah-Hartman Sept. 5, 2021, 2:51 p.m. UTC | #7
On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:
> On 9/4/21 1:01 AM, Greg KH wrote:

> > Please never use WARN_ON() in new code unless the machine is really

> > broken and you can not do anything else here.

> 

> Wait what?  I thought that was BUG_ON.


Not whan panic-on-warn is set, which is getting more and more common
these days.

thanks,

greg k-h
Ian Pilcher Sept. 5, 2021, 2:56 p.m. UTC | #8
On 9/5/21 9:51 AM, Greg KH wrote:
> On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:

>> On 9/4/21 1:01 AM, Greg KH wrote:

>>> Please never use WARN_ON() in new code unless the machine is really

>>> broken and you can not do anything else here.

>>

>> Wait what?  I thought that was BUG_ON.

> 

> Not whan panic-on-warn is set, which is getting more and more common

> these days.


Fair enough.  What is the recommend approach to reporting a "this should
never" happen situation these days?

Thanks!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================
Greg Kroah-Hartman Sept. 5, 2021, 3:12 p.m. UTC | #9
On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote:
> On 9/5/21 9:51 AM, Greg KH wrote:

> > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:

> > > On 9/4/21 1:01 AM, Greg KH wrote:

> > > > Please never use WARN_ON() in new code unless the machine is really

> > > > broken and you can not do anything else here.

> > > 

> > > Wait what?  I thought that was BUG_ON.

> > 

> > Not whan panic-on-warn is set, which is getting more and more common

> > these days.

> 

> Fair enough.  What is the recommend approach to reporting a "this should

> never" happen situation these days?


dev_err() and handle the error properly.
Eric Biggers Sept. 5, 2021, 4:55 p.m. UTC | #10
On Sun, Sep 05, 2021 at 05:12:39PM +0200, Greg KH wrote:
> On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote:

> > On 9/5/21 9:51 AM, Greg KH wrote:

> > > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote:

> > > > On 9/4/21 1:01 AM, Greg KH wrote:

> > > > > Please never use WARN_ON() in new code unless the machine is really

> > > > > broken and you can not do anything else here.

> > > > 

> > > > Wait what?  I thought that was BUG_ON.

> > > 

> > > Not whan panic-on-warn is set, which is getting more and more common

> > > these days.

> > 

> > Fair enough.  What is the recommend approach to reporting a "this should

> > never" happen situation these days?

> 

> dev_err() and handle the error properly.

> 

> 


WARN_ON is the right choice for reporting recoverable kernel bugs, and BUG_ON
for unrecoverable ones; see the two comments in include/asm-generic/bug.h which
explain this.  Please don't use dev_err() if it's a kernel bug (and not just
unexpected input from userspace or hardware behaving weirdly), as that prevents
the bug from being reported if it occurs.

Greg, you've been corrected on this before, e.g.
https://lore.kernel.org/linux-fsdevel/20210707023548.15872-1-desmondcheongzx@gmail.com/T/#u.
Please stop spreading false information as it is destroying your credibility :-(

- Eric