On Thu, Apr 09, 2020 at 05:54:46PM +0100, Al Viro wrote: > On Thu, Apr 09, 2020 at 05:50:48PM +0100, Al Viro wrote: > > On Thu, Apr 09, 2020 at 04:16:19PM +0200, Miklos Szeredi wrote: > > > Solve this by adding a cursor entry for each open instance. Taking the > > > global namespace_sem for write seems excessive, since we are only dealing > > > with a per-namespace list. Instead add a per-namespace spinlock and use > > > that together with namespace_sem taken for read to protect against > > > concurrent modification of the mount list. This may reduce parallelism of > > > is_local_mountpoint(), but it's hardly a big contention point. We could > > > also use RCU freeing of cursors to make traversal not need additional > > > locks, if that turns out to be neceesary. > > > > Umm... That can do more than reduction of parallelism - longer lists take > > longer to scan and moving cursors dirties cachelines in a bunch of struct > > mount instances. And I'm not convinced that your locking in m_next() is > > correct. > > > > What's to stop umount_tree() from removing the next entry from the list > > just as your m_next() tries to move the cursor? I don't see any common > > locks for those two... > > Ah, you still have namespace_sem taken (shared) by m_start(). Nevermind > that one, then... Let me get through mnt_list users and see if I can > catch anything. OK... Locking is safe, but it's not obvious. And your changes do make it scarier. There are several kinds of lists that can be threaded through ->mnt_list and your code depends upon never having those suckers appear in e.g. anon namespace ->list. It is true (AFAICS), but... Another fun question is ns->mounts rules - it used to be "the number of entries in ns->list", now it's "the number of non-cursor entries there". Incidentally, we might have a problem with that logics wrt count_mount(). Sigh... The damn thing has grown much too convoluted over years ;-/ I'm still not happy with that patch; at the very least it needs a lot more detailed analysis to go along with it.
