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@zeniv.linux.org.uk>
> ---
> 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);