Re: [PATCH v2] proc/mounts: add cursor

[Al Viro <viro@zeniv.linux.org.uk>]

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.