From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:58470 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727503AbeI3Feh (ORCPT ); Sun, 30 Sep 2018 01:34:37 -0400 From: NeilBrown To: Al Viro Date: Sun, 30 Sep 2018 09:04:11 +1000 Cc: linux-fsdevel@vger.kernel.org Subject: Re: [RFC] bloody odd logics in md_exit() In-Reply-To: <20180929033334.GG32577@ZenIV.linux.org.uk> References: <20180929033334.GG32577@ZenIV.linux.org.uk> Message-ID: <87k1n3gb2s.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable 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. =20 Or am I missing something else here? Thanks, NeilBrown > > Signed-off-by: Al Viro > --- > 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, struc= t file *file) > return error; > } >=20=20 > -static int md_unloading; > static __poll_t mdstat_poll(struct file *filp, poll_table *wait) > { > struct seq_file *seq =3D filp->private_data; > __poll_t mask; >=20=20 > - if (md_unloading) > - return EPOLLIN|EPOLLRDNORM|EPOLLERR|EPOLLPRI; > poll_wait(filp, &md_event_waiters, wait); >=20=20 > /* always allow read */ > @@ -7972,7 +7969,6 @@ static __poll_t mdstat_poll(struct file *filp, poll= _table *wait) > } >=20=20 > static const struct file_operations md_seq_fops =3D { > - .owner =3D THIS_MODULE, > .open =3D md_seq_open, > .read =3D seq_read, > .llseek =3D seq_lseek, > @@ -9382,7 +9378,6 @@ static __exit void md_exit(void) > { > struct mddev *mddev; > struct list_head *tmp; > - int delay =3D 1; >=20=20 > 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); >=20=20 > - /* We cannot unload the modules while some process is > - * waiting for us in select() or poll() - wake them up > - */ > - md_unloading =3D 1; > - while (waitqueue_active(&md_event_waiters)) { > - /* not safe to leave yet */ > - wake_up(&md_event_waiters); > - msleep(delay); > - delay +=3D delay; > - } > remove_proc_entry("mdstat", NULL); > + wake_up_all(&md_event_waiters); >=20=20 > for_each_mddev(mddev, tmp) { > export_array(mddev); --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAluwBOwACgkQOeye3VZi gbmnUQ//WPnTiHadGe6R8J7gufXAMcCrhj9Z5/RuxKTuYiZExM4drQRZUB2xgnQ1 402qyRTrVe7V9cDVPDe2rGFi1WNAPvHOARqcxfWs50UqeAm2pNuaW9EZWGmJSYON VdCaIi4OR/WOKlXMzkK6k7Edg4k/tpLYfmPeecUo4nO2VN6IZiRURaMb7Jjm4xAj T2Z2M8Nhb1da2kcrT8fmqufKwRYOXfyEbF5z+ZFgK1/wpjqQ8YiIxouDlp/JErOr b+DFjlSkgASHKXChAjFpfl19xQHXZ3KVJZbPKf9Ae/weKWDBV+3D5SOVh0QXp9LY 0X6atws2epjnuXrm+Cy4B/cJsY0gxUxVb55RVVwe8sGkvmXIJBeZxyGoYh3K9j/U sfcs8w37QP9SKZAJ3Y/VNvUdp23xOZnO7VwbCfvo6th7OySKh6eGEocZNyffEh1H yB+oQ9Du16e87iqvB26aNNlinxiSpb9cKOmkNgMSr8tw+HIWUkEcG2RHa/YiI3DE 22fNFeqZub8Z1MElC0gjcw2sJwya6cIlA7OzeASSnWgjiNwxUHrI0BYpKuWXe7uX mIlx+UfjDrTkpMtD2LNG0HXRItvJYYg0y82PvETYQVDJRV6zaCU44iSTR4YZ/z7u kLhjoXgCTJawDvAp961sb03xvOvrz/yo7fGSZNC2/V9pGTeHsaQ= =0CjL -----END PGP SIGNATURE----- --=-=-=--