Re: [PATCH 3/4] autofs - make mountpoint checks namespace aware

[Ian Kent]

On Tue, 2016-09-27 at 08:14 -0500, Eric W. Biederman wrote:
> Ian Kent <raven@xxxxxxxxxx> writes:
> 
> > On Mon, 2016-09-26 at 11:05 -0500, Eric W. Biederman wrote:
> > > Ian Kent <raven@xxxxxxxxxx> writes:
> > > 
> > > > On Fri, 2016-09-23 at 14:15 -0500, Eric W. Biederman wrote:
> > > > > Ian Kent <raven@xxxxxxxxxx> writes:
> > > > > 
> > > > > 2> On Thu, 2016-09-22 at 20:37 -0500, Eric W. Biederman wrote:
> > > > > > > Ian Kent <raven@xxxxxxxxxx> writes:
> > > > > > > 
> > > > > > > > On Thu, 2016-09-22 at 10:43 -0500, Eric W. Biederman wrote:
> > > > > > > > > Ian Kent <raven@xxxxxxxxxx> writes:
> > > > > > > > > 
> > > > > > > > > > Eric, Mateusz, I appreciate your spending time on this and
> > > > > > > > > > particularly
> > > > > > > > > > pointing
> > > > > > > > > > out my embarrassingly stupid is_local_mountpoint() usage
> > > > > > > > > > mistake.
> > > > > > > > > > 
> > > > > > > > > > Please accept my apology for the inconvenience.
> > > > > > > > > > 
> > > > > > > > > > If all goes well (in testing) I'll have follow up patches to
> > > > > > > > > > correct
> > > > > > > > > > this
> > > > > > > > > > fairly
> > > > > > > > > > soon.
> > > > > > > > > 
> > > > > > > > > Related question.  Do you happen to know how many mounts per
> > > > > > > > > mount
> > > > > > > > > namespace tend to be used?  It looks like it is going to be
> > > > > > > > > wise
> > > > > > > > > to
> > > > > > > > > put
> > > > > > > > > a configurable limit on that number.  And I would like the
> > > > > > > > > default
> > > > > > > > > to
> > > > > > > > > be
> > > > > > > > > something high enough most people don't care.  I believe
> > > > > > > > > autofs is
> > > > > > > > > likely where people tend to use the most mounts.
> > > > > > 
> > > > > > Yes, I agree, I did want to try and avoid changing the parameters to
> > > > > > ->d_mamange() but passing a struct path pointer might be better in
> > > > > > the
> > > > > > long
> > > > > > run
> > > > > > anyway.
> > > > > 
> > > > > Given that there is exactly one implementation of d_manage in the tree
> > > > > I
> > > > > don't imagine it will be disruptive to change that.
> > > > 
> > > > Yes, but it could be used by external modules.
> > > > 
> > > > And there's also have_submounts().
> > > 
> > > Good point about have_submounts.
> > > 
> > > > I can update that using the existing d_walk() infrastructure or take it
> > > > (mostly)
> > > > into the autofs module and get rid of have_submounts().
> > > > 
> > > > I'll go with the former to start with and see what people think.
> > > 
> > > That will be interesting to so.  It is not clear to me that if d_walk
> > > needs to be updated, and if d_walk doesn't need to be updated I would
> > > be surprised to see it take into autofs.  But I am happy to look at the
> > > end result and see what you come up with.
> > 
> > I didn't mean d_walk() itself, just the have_submounts() function that's
> > used
> > only by autofs these days. That's all I'll be changing.
> > 
> > To take this functionality into the autofs module shouldn't be a big deal as
> > it
> > amounts to a directory traversal with a check at each node.
> > 
> > But I vaguely remember talk of wanting to get rid of have_submounts() and
> > autofs
> > being the only remaining user.
> > 
> > So I mentioned it to try and elicit a comment, ;)
> 
> From my perspective the key detail is that d_walk is private to dcache.c
> 
> That said you want to look at may_umount_tree, or may_umount that
> are already exported from fs/namespace.c, and already used by autofs.
> One of those may already do the job you are trying to do.

Right, I'm aware of them, autofs uses may_umount() when asking if an autofs
mount can be umounted, and it uses may_umount_tree() to check busyness in its
expire.

may_umount_tree() (and may_umount()) won't tell you if there are any mounts
within the directory tree, only that there is an elevated reference count, for
example an open file or working directory etc., ie. busy.

OTOH, have_submounts() will return true as soon as it encounters a
d_mountpoint() dentry in the directory tree which is what's needed where it's
used and why it can return a false positive for the problem case.

I get that a function, say, path_has_submounts() probably shouldn't be placed in
dcache.c and that's another reason to take the traversal into autofs. 

But if there's no word from Al on that I'd prefer to use d_walk() and put it
there.

Ian