On Sat, Sep 29 2018, Al Viro wrote:
> Rationale in e2f23b606b94 (md: avoid oops on unload if some
> process is in poll or select) is very odd. Waitqueue code _does_
> provide a way to remove all listeners from a waitqueue - it's simply
> wake_up_all(). Once the wakeup callback has been executed (and it
> runs in context of wake_up_all() caller), we don't *care* if md.o
> is still there - all waiters are gone from the queue and the callback
> (pollwake() and friends) doesn't reinsert them.
>
> Why do we need the entire md_unloading logics? Just do
> remove_proc_entry() (which will wait for any in-progress call of
> ->poll()) and then do plain wake_up_all(). Nothing new could get
> added there once remove_proc_entry() is done and we don't need to
> wake the waiters one-by-one, let alone play with exponential
> backoffs for them to be gone from the queue...
>
> And while we are at it, procfs file_operations do not need
> ->owner - it's never looked at by anybody.
>
> Looks like the following would do just as well as the variant
> currently in mainline and it's considerably simpler... What am I
> missing here?
Hi Al,
I don't think wake_up_all() does remove anything from the queue.
It simply wakes up the various processes that are waiting.
They remain on the queue until they call remove_wait_queue(), which
could be delayed arbitrarily.
If it was delayed until after the module was unloaded and
"md_event_waiters" no longer existed, the unlink attempt would cause an
invalid memory access.
I think your approach for simplify the code would only work if
md_event_waiters could be moved out of the module, or if some global
wait_queue could be used instead.
Maybe we could use bit_waitqueue(NULL,0) (rather an ugly hack).
Maybe we could export a general-purpose waitqueue for modules to use.
Maybe procfs could export something??
I agree that we can remove md_unloading, by moving remove_proc_entry()
before the wakeup. I'm not yet convinced that we can remove the wakeup
loop.
Or am I missing something else here?
Thanks,
NeilBrown
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 63ceabb4e020..42ea44a809bd 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7953,14 +7953,11 @@ static int md_seq_open(struct inode *inode, struct file *file)
> return error;
> }
>
> -static int md_unloading;
> static __poll_t mdstat_poll(struct file *filp, poll_table *wait)
> {
> struct seq_file *seq = filp->private_data;
> __poll_t mask;
>
> - if (md_unloading)
> - return EPOLLIN|EPOLLRDNORM|EPOLLERR|EPOLLPRI;
> poll_wait(filp, &md_event_waiters, wait);
>
> /* always allow read */
> @@ -7972,7 +7969,6 @@ static __poll_t mdstat_poll(struct file *filp, poll_table *wait)
> }
>
> static const struct file_operations md_seq_fops = {
> - .owner = THIS_MODULE,
> .open = md_seq_open,
> .read = seq_read,
> .llseek = seq_lseek,
> @@ -9382,7 +9378,6 @@ static __exit void md_exit(void)
> {
> struct mddev *mddev;
> struct list_head *tmp;
> - int delay = 1;
>
> blk_unregister_region(MKDEV(MD_MAJOR,0), 512);
> blk_unregister_region(MKDEV(mdp_major,0), 1U << MINORBITS);
> @@ -9392,17 +9387,8 @@ static __exit void md_exit(void)
> unregister_reboot_notifier(&md_notifier);
> unregister_sysctl_table(raid_table_header);
>
> - /* We cannot unload the modules while some process is
> - * waiting for us in select() or poll() - wake them up
> - */
> - md_unloading = 1;
> - while (waitqueue_active(&md_event_waiters)) {
> - /* not safe to leave yet */
> - wake_up(&md_event_waiters);
> - msleep(delay);
> - delay += delay;
> - }
> remove_proc_entry("mdstat", NULL);
> + wake_up_all(&md_event_waiters);
>
> for_each_mddev(mddev, tmp) {
> export_array(mddev);
Attachment:
signature.asc
Description: PGP signature
