| Message ID | 20240509122026.30015-1-kinga.stefaniuk@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] md: generate CHANGE uevents for md device | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6-10-PR | success | PR summary |
| mdraidci/vmtest-md-6-10-VM_Test-0 | success | Logs for build-kernel |
Hi, 在 2024/05/09 20:20, Kinga Stefaniuk 写道: > In mdadm commit 49b69533e8 ("mdmonitor: check if udev has finished > events processing") mdmonitor has been learnt to wait for udev to finish > processing, and later in commit 9935cf0f64f3 ("Mdmonitor: Improve udev > event handling") pooling for MD events on /proc/mdstat file has been > deprecated because relying on udev events is more reliable and less bug > prone (we are not concurring with udev). > After those changes we are still observing missing mdmonitor events in > some scenarios, especially SpareEvent is likely to be missed. With this > patch MD will be able to generate more change uevents and wakeup > mdmonitor more frequently to give it possibility to notice events. > MD has md_new_events() functionality to trigger events and with this > patch this function is extended to generate udev CHANGE uevents. It > cannot be done directly because this function is called on interrupts > context, so appropriate workqueue is created. Uevents are less time > critical, it is safe to use workqueue. It is limited to CHANGE event as > there is no need to generate other uevents for now. > With this change, mdmonitor events are less likely to be missed. Out > internal tests suite confirms that, mdmonitor reliability is (again) > imrpoved. > > Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com> > Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com> > --- It'll be better to add change log from previous verion here. > drivers/md/md.c | 42 +++++++++++++++++++++++++++--------------- > drivers/md/md.h | 3 ++- > drivers/md/raid10.c | 2 +- > drivers/md/raid5.c | 2 +- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index e575e74aabf5..5864beda4836 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -313,6 +313,16 @@ static int start_readonly; > */ > static bool create_on_open = true; > > +/* > + * Send every new event to the userspace. > + */ > +static void trigger_event(struct work_struct *work) > +{ > + struct mddev *mddev = container_of(work, struct mddev, uevent_work); > + > + kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE); > +} > + > /* > * We have a system wide 'event count' that is incremented > * on any 'interesting' event, and readers of /proc/mdstat > @@ -325,10 +335,11 @@ static bool create_on_open = true; > */ > static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); > static atomic_t md_event_count; > -void md_new_event(void) > +void md_new_event(struct mddev *mddev) > { > atomic_inc(&md_event_count); > wake_up(&md_event_waiters); > + schedule_work(&mddev->uevent_work); This doesn't look correct, if uevent_work is queued already and not running yet, this will do nothing, which means uevent will be missing. Looks like you want to generate as much uevent as md_new_event() is called, or you maybe don't care about this? By the way, since we're here, will it be better to add some uevent description in detail as well? for example: After add a new disk by mdadm, use kobject_uevent_env() and pass in additional string like "add <disk> to array/conf". And many other events. Thanks, Kuai > } > EXPORT_SYMBOL_GPL(md_new_event); > > @@ -2940,7 +2951,7 @@ static int add_bound_rdev(struct md_rdev *rdev) > if (mddev->degraded) > set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > } > > @@ -3057,7 +3068,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) > md_kick_rdev_from_array(rdev); > if (mddev->pers) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > - md_new_event(); > + md_new_event(mddev); > } > } > } else if (cmd_match(buf, "writemostly")) { > @@ -4173,7 +4184,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) > if (!mddev->thread) > md_update_sb(mddev, 1); > sysfs_notify_dirent_safe(mddev->sysfs_level); > - md_new_event(); > + md_new_event(mddev); > rv = len; > out_unlock: > mddev_unlock_and_resume(mddev); > @@ -4700,7 +4711,7 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len) > export_rdev(rdev, mddev); > mddev_unlock_and_resume(mddev); > if (!err) > - md_new_event(); > + md_new_event(mddev); > return err ? err : len; > } > > @@ -5902,6 +5913,7 @@ struct mddev *md_alloc(dev_t dev, char *name) > return ERR_PTR(error); > } > > + INIT_WORK(&mddev->uevent_work, trigger_event); > kobject_uevent(&mddev->kobj, KOBJ_ADD); > mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); > mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); > @@ -6244,7 +6256,7 @@ int md_run(struct mddev *mddev) > if (mddev->sb_flags) > md_update_sb(mddev, 0); > > - md_new_event(); > + md_new_event(mddev); > return 0; > > bitmap_abort: > @@ -6603,7 +6615,7 @@ static int do_md_stop(struct mddev *mddev, int mode) > if (mddev->hold_active == UNTIL_STOP) > mddev->hold_active = 0; > } > - md_new_event(); > + md_new_event(mddev); > sysfs_notify_dirent_safe(mddev->sysfs_state); > return 0; > } > @@ -7099,7 +7111,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > if (!mddev->thread) > md_update_sb(mddev, 1); > - md_new_event(); > + md_new_event(mddev); > > return 0; > busy: > @@ -7179,7 +7191,7 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev) > * array immediately. > */ > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort_export: > @@ -8158,7 +8170,7 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev) > } > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > - md_new_event(); > + md_new_event(mddev); > } > EXPORT_SYMBOL(md_error); > > @@ -9044,7 +9056,7 @@ void md_do_sync(struct md_thread *thread) > mddev->curr_resync = MD_RESYNC_ACTIVE; /* no longer delayed */ > mddev->curr_resync_completed = j; > sysfs_notify_dirent_safe(mddev->sysfs_completed); > - md_new_event(); > + md_new_event(mddev); > update_time = jiffies; > > blk_start_plug(&plug); > @@ -9115,7 +9127,7 @@ void md_do_sync(struct md_thread *thread) > /* this is the earliest that rebuild will be > * visible in /proc/mdstat > */ > - md_new_event(); > + md_new_event(mddev); > > if (last_check + window > io_sectors || j == max_sectors) > continue; > @@ -9381,7 +9393,7 @@ static int remove_and_add_spares(struct mddev *mddev, > sysfs_link_rdev(mddev, rdev); > if (!test_bit(Journal, &rdev->flags)) > spares++; > - md_new_event(); > + md_new_event(mddev); > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > } > } > @@ -9500,7 +9512,7 @@ static void md_start_sync(struct work_struct *ws) > __mddev_resume(mddev, false); > md_wakeup_thread(mddev->sync_thread); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > return; > > not_running: > @@ -9752,7 +9764,7 @@ void md_reap_sync_thread(struct mddev *mddev) > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > sysfs_notify_dirent_safe(mddev->sysfs_completed); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > wake_up(&resync_wait); > diff --git a/drivers/md/md.h b/drivers/md/md.h > index 097d9dbd69b8..111aa3a0f60c 100644 > --- a/drivers/md/md.h > +++ b/drivers/md/md.h > @@ -528,6 +528,7 @@ struct mddev { > */ > struct work_struct flush_work; > struct work_struct event_work; /* used by dm to report failure event */ > + struct work_struct uevent_work; > mempool_t *serial_info_pool; > void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev); > struct md_cluster_info *cluster_info; > @@ -802,7 +803,7 @@ extern int md_super_wait(struct mddev *mddev); > extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size, > struct page *page, blk_opf_t opf, bool metadata_op); > extern void md_do_sync(struct md_thread *thread); > -extern void md_new_event(void); > +extern void md_new_event(struct mddev *mddev); > extern void md_allow_write(struct mddev *mddev); > extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev); > extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors); > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index a4556d2e46bf..6f459d47e2a5 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -4545,7 +4545,7 @@ static int raid10_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort: > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index d874abfc1836..f5736fa1b318 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -8512,7 +8512,7 @@ static int raid5_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > } > >
On Thu, 9 May 2024 21:09:12 +0800 Yu Kuai <yukuai1@huaweicloud.com> wrote: > > static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); > > static atomic_t md_event_count; > > -void md_new_event(void) > > +void md_new_event(struct mddev *mddev) > > { > > atomic_inc(&md_event_count); > > wake_up(&md_event_waiters); > > + schedule_work(&mddev->uevent_work); > > This doesn't look correct, if uevent_work is queued already and not > running yet, this will do nothing, which means uevent will be missing. > Looks like you want to generate as much uevent as md_new_event() is > called, or you maybe don't care about this? I think we don't need to care. I meant that we don't need perfect mechanism. If there will be two events queued then probably userspace will be busy handling first one so the second one will be unnoticed anyway. Perhaps, we should left comment? > > By the way, since we're here, will it be better to add some uevent > description in detail as well? for example: > > After add a new disk by mdadm, use kobject_uevent_env() and pass in > additional string like "add <disk> to array/conf". > > And many other events. > Could you please elaborate more? I don't understand why this comment is connected to new disk, potentially CHANGE uevent can be generated in more scenarios, like rebuild/resync/reshape end, level change, starting raid array, stopping raid array (I just briefly looked where md_new_event() is called). I think that it is correct because those changes are real CHANGE of mddevice state. Also, there is a comment below md_new_event(): * Events are: * start array, stop array, error, add device, remove device, * start build, activate spare Thanks, Mariusz
Hi, 在 2024/05/09 23:33, Mariusz Tkaczyk 写道: > On Thu, 9 May 2024 21:09:12 +0800 > Yu Kuai <yukuai1@huaweicloud.com> wrote: > >>> static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); >>> static atomic_t md_event_count; >>> -void md_new_event(void) >>> +void md_new_event(struct mddev *mddev) >>> { >>> atomic_inc(&md_event_count); >>> wake_up(&md_event_waiters); >>> + schedule_work(&mddev->uevent_work); >> >> This doesn't look correct, if uevent_work is queued already and not >> running yet, this will do nothing, which means uevent will be missing. >> Looks like you want to generate as much uevent as md_new_event() is >> called, or you maybe don't care about this? > > I think we don't need to care. I meant that we don't need perfect mechanism. > If there will be two events queued then probably userspace will be busy > handling first one so the second one will be unnoticed anyway. Perhaps, we > should left comment? I'm not sure, for example: 1) user remove a device, wait for uevent; 2) a event is triggered by other context, while the device is not removed yet; 3) user receive a event, then how do user side handle this case? And if we really don't care about this case, comment will be good. > >> >> By the way, since we're here, will it be better to add some uevent >> description in detail as well? for example: >> >> After add a new disk by mdadm, use kobject_uevent_env() and pass in >> additional string like "add <disk> to array/conf". >> >> And many other events. >> > > Could you please elaborate more? I don't understand why this comment is > connected to new disk, potentially CHANGE uevent can be generated in more > scenarios, like rebuild/resync/reshape end, level change, starting raid array, > stopping raid array (I just briefly looked where md_new_event() is called). I > think that it is correct because those changes are real CHANGE of mddevice > state. Also, there is a comment below md_new_event(): Yes, this is just an example for add device. I mean all other events should be attached with proper string as well. So that if user just remove a disk, it can wait for the CHANGE event with the additional string like "remove xxx". Thanks, Kuai > > * Events are: > * start array, stop array, error, add device, remove device, > * start build, activate spare > > Thanks, > Mariusz > > . >
On 5/9/24 14:20, Kinga Stefaniuk wrote: > In mdadm commit 49b69533e8 ("mdmonitor: check if udev has finished > events processing") mdmonitor has been learnt to wait for udev to finish > processing, and later in commit 9935cf0f64f3 ("Mdmonitor: Improve udev > event handling") pooling for MD events on /proc/mdstat file has been > deprecated because relying on udev events is more reliable and less bug > prone (we are not concurring with udev). > After those changes we are still observing missing mdmonitor events in > some scenarios, especially SpareEvent is likely to be missed. With this > patch MD will be able to generate more change uevents and wakeup > mdmonitor more frequently to give it possibility to notice events. > MD has md_new_events() functionality to trigger events and with this > patch this function is extended to generate udev CHANGE uevents. It > cannot be done directly because this function is called on interrupts > context, so appropriate workqueue is created. Uevents are less time > critical, it is safe to use workqueue. It is limited to CHANGE event as > there is no need to generate other uevents for now. > With this change, mdmonitor events are less likely to be missed. Out > internal tests suite confirms that, mdmonitor reliability is (again) > imrpoved. > > Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com> > Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com> > --- > drivers/md/md.c | 42 +++++++++++++++++++++++++++--------------- > drivers/md/md.h | 3 ++- > drivers/md/raid10.c | 2 +- > drivers/md/raid5.c | 2 +- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index e575e74aabf5..5864beda4836 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -313,6 +313,16 @@ static int start_readonly; > */ > static bool create_on_open = true; > > +/* > + * Send every new event to the userspace. > + */ > +static void trigger_event(struct work_struct *work) > +{ > + struct mddev *mddev = container_of(work, struct mddev, uevent_work); > + > + kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE); > +} > + > /* > * We have a system wide 'event count' that is incremented > * on any 'interesting' event, and readers of /proc/mdstat > @@ -325,10 +335,11 @@ static bool create_on_open = true; > */ > static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); > static atomic_t md_event_count; > -void md_new_event(void) > +void md_new_event(struct mddev *mddev) > { > atomic_inc(&md_event_count); > wake_up(&md_event_waiters); > + schedule_work(&mddev->uevent_work); > } > EXPORT_SYMBOL_GPL(md_new_event); > > @@ -2940,7 +2951,7 @@ static int add_bound_rdev(struct md_rdev *rdev) > if (mddev->degraded) > set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > } > > @@ -3057,7 +3068,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) > md_kick_rdev_from_array(rdev); > if (mddev->pers) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > - md_new_event(); > + md_new_event(mddev); > } > } > } else if (cmd_match(buf, "writemostly")) { > @@ -4173,7 +4184,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) > if (!mddev->thread) > md_update_sb(mddev, 1); > sysfs_notify_dirent_safe(mddev->sysfs_level); > - md_new_event(); > + md_new_event(mddev); > rv = len; > out_unlock: > mddev_unlock_and_resume(mddev); > @@ -4700,7 +4711,7 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len) > export_rdev(rdev, mddev); > mddev_unlock_and_resume(mddev); > if (!err) > - md_new_event(); > + md_new_event(mddev); > return err ? err : len; > } > > @@ -5902,6 +5913,7 @@ struct mddev *md_alloc(dev_t dev, char *name) > return ERR_PTR(error); > } > > + INIT_WORK(&mddev->uevent_work, trigger_event); > kobject_uevent(&mddev->kobj, KOBJ_ADD); > mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); > mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); > @@ -6244,7 +6256,7 @@ int md_run(struct mddev *mddev) > if (mddev->sb_flags) > md_update_sb(mddev, 0); > > - md_new_event(); > + md_new_event(mddev); > return 0; > > bitmap_abort: > @@ -6603,7 +6615,7 @@ static int do_md_stop(struct mddev *mddev, int mode) > if (mddev->hold_active == UNTIL_STOP) > mddev->hold_active = 0; > } > - md_new_event(); > + md_new_event(mddev); > sysfs_notify_dirent_safe(mddev->sysfs_state); > return 0; > } > @@ -7099,7 +7111,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > if (!mddev->thread) > md_update_sb(mddev, 1); > - md_new_event(); > + md_new_event(mddev); > > return 0; > busy: > @@ -7179,7 +7191,7 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev) > * array immediately. > */ > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort_export: > @@ -8158,7 +8170,7 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev) > } > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > - md_new_event(); > + md_new_event(mddev); > } > EXPORT_SYMBOL(md_error); > > @@ -9044,7 +9056,7 @@ void md_do_sync(struct md_thread *thread) > mddev->curr_resync = MD_RESYNC_ACTIVE; /* no longer delayed */ > mddev->curr_resync_completed = j; > sysfs_notify_dirent_safe(mddev->sysfs_completed); > - md_new_event(); > + md_new_event(mddev); > update_time = jiffies; > > blk_start_plug(&plug); > @@ -9115,7 +9127,7 @@ void md_do_sync(struct md_thread *thread) > /* this is the earliest that rebuild will be > * visible in /proc/mdstat > */ > - md_new_event(); > + md_new_event(mddev); > > if (last_check + window > io_sectors || j == max_sectors) > continue; > @@ -9381,7 +9393,7 @@ static int remove_and_add_spares(struct mddev *mddev, > sysfs_link_rdev(mddev, rdev); > if (!test_bit(Journal, &rdev->flags)) > spares++; > - md_new_event(); > + md_new_event(mddev); > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > } > } > @@ -9500,7 +9512,7 @@ static void md_start_sync(struct work_struct *ws) > __mddev_resume(mddev, false); > md_wakeup_thread(mddev->sync_thread); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > return; > > not_running: > @@ -9752,7 +9764,7 @@ void md_reap_sync_thread(struct mddev *mddev) > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > sysfs_notify_dirent_safe(mddev->sysfs_completed); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > wake_up(&resync_wait); > diff --git a/drivers/md/md.h b/drivers/md/md.h > index 097d9dbd69b8..111aa3a0f60c 100644 > --- a/drivers/md/md.h > +++ b/drivers/md/md.h > @@ -528,6 +528,7 @@ struct mddev { > */ > struct work_struct flush_work; > struct work_struct event_work; /* used by dm to report failure event */ > + struct work_struct uevent_work; > mempool_t *serial_info_pool; > void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev); > struct md_cluster_info *cluster_info; > @@ -802,7 +803,7 @@ extern int md_super_wait(struct mddev *mddev); > extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size, > struct page *page, blk_opf_t opf, bool metadata_op); > extern void md_do_sync(struct md_thread *thread); > -extern void md_new_event(void); > +extern void md_new_event(struct mddev *mddev); > extern void md_allow_write(struct mddev *mddev); > extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev); > extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors); > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index a4556d2e46bf..6f459d47e2a5 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -4545,7 +4545,7 @@ static int raid10_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort: > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index d874abfc1836..f5736fa1b318 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -8512,7 +8512,7 @@ static int raid5_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > } > Don't you need to flush the workqueue on exit? Seeing that it's running asynchronously... Cheers, Hannes
Dear Kinga, Thank you for the more elaborate commit message. Some minor coments. Am 09.05.24 um 14:20 schrieb Kinga Stefaniuk: > In mdadm commit 49b69533e8 ("mdmonitor: check if udev has finished > events processing") mdmonitor has been learnt to wait for udev to finish > processing, and later in commit 9935cf0f64f3 ("Mdmonitor: Improve udev > event handling") pooling for MD events on /proc/mdstat file has been > deprecated because relying on udev events is more reliable and less bug > prone (we are not concurring with udev). competing instead of concurring? I’d add a blank line between paragraphs. > After those changes we are still observing missing mdmonitor events in > some scenarios, especially SpareEvent is likely to be missed. With this > patch MD will be able to generate more change uevents and wakeup > mdmonitor more frequently to give it possibility to notice events. > MD has md_new_events() functionality to trigger events and with this > patch this function is extended to generate udev CHANGE uevents. It > cannot be done directly because this function is called on interrupts > context, so appropriate workqueue is created. Uevents are less time > critical, it is safe to use workqueue. It is limited to CHANGE event as > there is no need to generate other uevents for now. > With this change, mdmonitor events are less likely to be missed. Out Our > internal tests suite confirms that, mdmonitor reliability is (again) > imrpoved. improved Would be nice, if these tests were public. (Maybe they are?) Kind regards, Paul > Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com> > Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com> > --- > drivers/md/md.c | 42 +++++++++++++++++++++++++++--------------- > drivers/md/md.h | 3 ++- > drivers/md/raid10.c | 2 +- > drivers/md/raid5.c | 2 +- > 4 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index e575e74aabf5..5864beda4836 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -313,6 +313,16 @@ static int start_readonly; > */ > static bool create_on_open = true; > > +/* > + * Send every new event to the userspace. > + */ > +static void trigger_event(struct work_struct *work) > +{ > + struct mddev *mddev = container_of(work, struct mddev, uevent_work); > + > + kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE); > +} > + > /* > * We have a system wide 'event count' that is incremented > * on any 'interesting' event, and readers of /proc/mdstat > @@ -325,10 +335,11 @@ static bool create_on_open = true; > */ > static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); > static atomic_t md_event_count; > -void md_new_event(void) > +void md_new_event(struct mddev *mddev) > { > atomic_inc(&md_event_count); > wake_up(&md_event_waiters); > + schedule_work(&mddev->uevent_work); > } > EXPORT_SYMBOL_GPL(md_new_event); > > @@ -2940,7 +2951,7 @@ static int add_bound_rdev(struct md_rdev *rdev) > if (mddev->degraded) > set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > } > > @@ -3057,7 +3068,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) > md_kick_rdev_from_array(rdev); > if (mddev->pers) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > - md_new_event(); > + md_new_event(mddev); > } > } > } else if (cmd_match(buf, "writemostly")) { > @@ -4173,7 +4184,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) > if (!mddev->thread) > md_update_sb(mddev, 1); > sysfs_notify_dirent_safe(mddev->sysfs_level); > - md_new_event(); > + md_new_event(mddev); > rv = len; > out_unlock: > mddev_unlock_and_resume(mddev); > @@ -4700,7 +4711,7 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len) > export_rdev(rdev, mddev); > mddev_unlock_and_resume(mddev); > if (!err) > - md_new_event(); > + md_new_event(mddev); > return err ? err : len; > } > > @@ -5902,6 +5913,7 @@ struct mddev *md_alloc(dev_t dev, char *name) > return ERR_PTR(error); > } > > + INIT_WORK(&mddev->uevent_work, trigger_event); > kobject_uevent(&mddev->kobj, KOBJ_ADD); > mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); > mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); > @@ -6244,7 +6256,7 @@ int md_run(struct mddev *mddev) > if (mddev->sb_flags) > md_update_sb(mddev, 0); > > - md_new_event(); > + md_new_event(mddev); > return 0; > > bitmap_abort: > @@ -6603,7 +6615,7 @@ static int do_md_stop(struct mddev *mddev, int mode) > if (mddev->hold_active == UNTIL_STOP) > mddev->hold_active = 0; > } > - md_new_event(); > + md_new_event(mddev); > sysfs_notify_dirent_safe(mddev->sysfs_state); > return 0; > } > @@ -7099,7 +7111,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev) > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > if (!mddev->thread) > md_update_sb(mddev, 1); > - md_new_event(); > + md_new_event(mddev); > > return 0; > busy: > @@ -7179,7 +7191,7 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev) > * array immediately. > */ > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort_export: > @@ -8158,7 +8170,7 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev) > } > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > - md_new_event(); > + md_new_event(mddev); > } > EXPORT_SYMBOL(md_error); > > @@ -9044,7 +9056,7 @@ void md_do_sync(struct md_thread *thread) > mddev->curr_resync = MD_RESYNC_ACTIVE; /* no longer delayed */ > mddev->curr_resync_completed = j; > sysfs_notify_dirent_safe(mddev->sysfs_completed); > - md_new_event(); > + md_new_event(mddev); > update_time = jiffies; > > blk_start_plug(&plug); > @@ -9115,7 +9127,7 @@ void md_do_sync(struct md_thread *thread) > /* this is the earliest that rebuild will be > * visible in /proc/mdstat > */ > - md_new_event(); > + md_new_event(mddev); > > if (last_check + window > io_sectors || j == max_sectors) > continue; > @@ -9381,7 +9393,7 @@ static int remove_and_add_spares(struct mddev *mddev, > sysfs_link_rdev(mddev, rdev); > if (!test_bit(Journal, &rdev->flags)) > spares++; > - md_new_event(); > + md_new_event(mddev); > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > } > } > @@ -9500,7 +9512,7 @@ static void md_start_sync(struct work_struct *ws) > __mddev_resume(mddev, false); > md_wakeup_thread(mddev->sync_thread); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > return; > > not_running: > @@ -9752,7 +9764,7 @@ void md_reap_sync_thread(struct mddev *mddev) > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > sysfs_notify_dirent_safe(mddev->sysfs_completed); > sysfs_notify_dirent_safe(mddev->sysfs_action); > - md_new_event(); > + md_new_event(mddev); > if (mddev->event_work.func) > queue_work(md_misc_wq, &mddev->event_work); > wake_up(&resync_wait); > diff --git a/drivers/md/md.h b/drivers/md/md.h > index 097d9dbd69b8..111aa3a0f60c 100644 > --- a/drivers/md/md.h > +++ b/drivers/md/md.h > @@ -528,6 +528,7 @@ struct mddev { > */ > struct work_struct flush_work; > struct work_struct event_work; /* used by dm to report failure event */ > + struct work_struct uevent_work; > mempool_t *serial_info_pool; > void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev); > struct md_cluster_info *cluster_info; > @@ -802,7 +803,7 @@ extern int md_super_wait(struct mddev *mddev); > extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size, > struct page *page, blk_opf_t opf, bool metadata_op); > extern void md_do_sync(struct md_thread *thread); > -extern void md_new_event(void); > +extern void md_new_event(struct mddev *mddev); > extern void md_allow_write(struct mddev *mddev); > extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev); > extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors); > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index a4556d2e46bf..6f459d47e2a5 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -4545,7 +4545,7 @@ static int raid10_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > > abort: > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index d874abfc1836..f5736fa1b318 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -8512,7 +8512,7 @@ static int raid5_start_reshape(struct mddev *mddev) > set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > conf->reshape_checkpoint = jiffies; > - md_new_event(); > + md_new_event(mddev); > return 0; > } >
diff --git a/drivers/md/md.c b/drivers/md/md.c index e575e74aabf5..5864beda4836 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -313,6 +313,16 @@ static int start_readonly; */ static bool create_on_open = true; +/* + * Send every new event to the userspace. + */ +static void trigger_event(struct work_struct *work) +{ + struct mddev *mddev = container_of(work, struct mddev, uevent_work); + + kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE); +} + /* * We have a system wide 'event count' that is incremented * on any 'interesting' event, and readers of /proc/mdstat @@ -325,10 +335,11 @@ static bool create_on_open = true; */ static DECLARE_WAIT_QUEUE_HEAD(md_event_waiters); static atomic_t md_event_count; -void md_new_event(void) +void md_new_event(struct mddev *mddev) { atomic_inc(&md_event_count); wake_up(&md_event_waiters); + schedule_work(&mddev->uevent_work); } EXPORT_SYMBOL_GPL(md_new_event); @@ -2940,7 +2951,7 @@ static int add_bound_rdev(struct md_rdev *rdev) if (mddev->degraded) set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); - md_new_event(); + md_new_event(mddev); return 0; } @@ -3057,7 +3068,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) md_kick_rdev_from_array(rdev); if (mddev->pers) set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); - md_new_event(); + md_new_event(mddev); } } } else if (cmd_match(buf, "writemostly")) { @@ -4173,7 +4184,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) if (!mddev->thread) md_update_sb(mddev, 1); sysfs_notify_dirent_safe(mddev->sysfs_level); - md_new_event(); + md_new_event(mddev); rv = len; out_unlock: mddev_unlock_and_resume(mddev); @@ -4700,7 +4711,7 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len) export_rdev(rdev, mddev); mddev_unlock_and_resume(mddev); if (!err) - md_new_event(); + md_new_event(mddev); return err ? err : len; } @@ -5902,6 +5913,7 @@ struct mddev *md_alloc(dev_t dev, char *name) return ERR_PTR(error); } + INIT_WORK(&mddev->uevent_work, trigger_event); kobject_uevent(&mddev->kobj, KOBJ_ADD); mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); @@ -6244,7 +6256,7 @@ int md_run(struct mddev *mddev) if (mddev->sb_flags) md_update_sb(mddev, 0); - md_new_event(); + md_new_event(mddev); return 0; bitmap_abort: @@ -6603,7 +6615,7 @@ static int do_md_stop(struct mddev *mddev, int mode) if (mddev->hold_active == UNTIL_STOP) mddev->hold_active = 0; } - md_new_event(); + md_new_event(mddev); sysfs_notify_dirent_safe(mddev->sysfs_state); return 0; } @@ -7099,7 +7111,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev) set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); if (!mddev->thread) md_update_sb(mddev, 1); - md_new_event(); + md_new_event(mddev); return 0; busy: @@ -7179,7 +7191,7 @@ static int hot_add_disk(struct mddev *mddev, dev_t dev) * array immediately. */ set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); - md_new_event(); + md_new_event(mddev); return 0; abort_export: @@ -8158,7 +8170,7 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev) } if (mddev->event_work.func) queue_work(md_misc_wq, &mddev->event_work); - md_new_event(); + md_new_event(mddev); } EXPORT_SYMBOL(md_error); @@ -9044,7 +9056,7 @@ void md_do_sync(struct md_thread *thread) mddev->curr_resync = MD_RESYNC_ACTIVE; /* no longer delayed */ mddev->curr_resync_completed = j; sysfs_notify_dirent_safe(mddev->sysfs_completed); - md_new_event(); + md_new_event(mddev); update_time = jiffies; blk_start_plug(&plug); @@ -9115,7 +9127,7 @@ void md_do_sync(struct md_thread *thread) /* this is the earliest that rebuild will be * visible in /proc/mdstat */ - md_new_event(); + md_new_event(mddev); if (last_check + window > io_sectors || j == max_sectors) continue; @@ -9381,7 +9393,7 @@ static int remove_and_add_spares(struct mddev *mddev, sysfs_link_rdev(mddev, rdev); if (!test_bit(Journal, &rdev->flags)) spares++; - md_new_event(); + md_new_event(mddev); set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); } } @@ -9500,7 +9512,7 @@ static void md_start_sync(struct work_struct *ws) __mddev_resume(mddev, false); md_wakeup_thread(mddev->sync_thread); sysfs_notify_dirent_safe(mddev->sysfs_action); - md_new_event(); + md_new_event(mddev); return; not_running: @@ -9752,7 +9764,7 @@ void md_reap_sync_thread(struct mddev *mddev) set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); sysfs_notify_dirent_safe(mddev->sysfs_completed); sysfs_notify_dirent_safe(mddev->sysfs_action); - md_new_event(); + md_new_event(mddev); if (mddev->event_work.func) queue_work(md_misc_wq, &mddev->event_work); wake_up(&resync_wait); diff --git a/drivers/md/md.h b/drivers/md/md.h index 097d9dbd69b8..111aa3a0f60c 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -528,6 +528,7 @@ struct mddev { */ struct work_struct flush_work; struct work_struct event_work; /* used by dm to report failure event */ + struct work_struct uevent_work; mempool_t *serial_info_pool; void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev); struct md_cluster_info *cluster_info; @@ -802,7 +803,7 @@ extern int md_super_wait(struct mddev *mddev); extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size, struct page *page, blk_opf_t opf, bool metadata_op); extern void md_do_sync(struct md_thread *thread); -extern void md_new_event(void); +extern void md_new_event(struct mddev *mddev); extern void md_allow_write(struct mddev *mddev); extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev); extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors); diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index a4556d2e46bf..6f459d47e2a5 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -4545,7 +4545,7 @@ static int raid10_start_reshape(struct mddev *mddev) set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); conf->reshape_checkpoint = jiffies; - md_new_event(); + md_new_event(mddev); return 0; abort: diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index d874abfc1836..f5736fa1b318 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -8512,7 +8512,7 @@ static int raid5_start_reshape(struct mddev *mddev) set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); conf->reshape_checkpoint = jiffies; - md_new_event(); + md_new_event(mddev); return 0; }
In mdadm commit 49b69533e8 ("mdmonitor: check if udev has finished events processing") mdmonitor has been learnt to wait for udev to finish processing, and later in commit 9935cf0f64f3 ("Mdmonitor: Improve udev event handling") pooling for MD events on /proc/mdstat file has been deprecated because relying on udev events is more reliable and less bug prone (we are not concurring with udev). After those changes we are still observing missing mdmonitor events in some scenarios, especially SpareEvent is likely to be missed. With this patch MD will be able to generate more change uevents and wakeup mdmonitor more frequently to give it possibility to notice events. MD has md_new_events() functionality to trigger events and with this patch this function is extended to generate udev CHANGE uevents. It cannot be done directly because this function is called on interrupts context, so appropriate workqueue is created. Uevents are less time critical, it is safe to use workqueue. It is limited to CHANGE event as there is no need to generate other uevents for now. With this change, mdmonitor events are less likely to be missed. Out internal tests suite confirms that, mdmonitor reliability is (again) imrpoved. Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com> Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com> --- drivers/md/md.c | 42 +++++++++++++++++++++++++++--------------- drivers/md/md.h | 3 ++- drivers/md/raid10.c | 2 +- drivers/md/raid5.c | 2 +- 4 files changed, 31 insertions(+), 18 deletions(-)