Re: [PATCH 4/4] nfsd: Pin to vfsmnt instead of mntget

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]

On Mon, 11 May 2015 21:08:47 +0800 Kinglong Mee <kinglongmee@gmail.com> wrote:

> On 5/8/2015 9:47 PM, J. Bruce Fields wrote:
> > On Fri, May 08, 2015 at 02:40:31PM +1000, NeilBrown wrote:
> >> Thanks for this patch.  It looks good!
> >>
> >> My only comment on the code is that I would really like to see a
> >> "path_get_pin()" and "path_put_unpin()" rather than open coding:
> >>
> >>> +	dget(item->ek_path.dentry);
> >>> +	pin_insert_group(&new->ek_pin, item->ek_path.mnt, NULL);
> >>
> >> and 
> >>
> >>> +		dput(key->ek_path.dentry);
> >>> +		pin_remove(&key->ek_pin);
> >>
> >>
> >> But the question you raise is an important one:  Exactly which filesystems
> >> should be allowed to be unmounted?
> >> This is a change in behaviour - is it one that people uniformly would want?
> >>
> >> The kernel doesn't currently know which file systems were explicitly listed
> >> in /etc/exports, and which were found by following a 'crossmnt'.
> >> It could guess and allow the unmounting of anything below a 'crossmnt', but I
> >> wouldn't be comfortable with that - it is error prone.
> >>
> >> mountd does know what is in /etc/exports, and could tell the kernel.
> >> For the expkey cache, we could always use path_get_pin.
> >> For the export cache (where flags are available) we could use path_get
> >> or path_get_pin depending on some new flag.
> >>
> >> I'm not really sure it is worth it.  I would rather the filesystems could
> >> always be unmounted.  But doing that could possibly break someone's work
> >> flow.  Maybe.
> >>
> >> Or maybe I'm seeing problems where there aren't any.
> >>
> >> Anyone else have an opinion?
> > 
> > The undisputed bug here was negative cache entries preventing unmount.
> > So most conservative might be just to purge negative entries.
> 
> I'd like this,
> if the cache is valid, user should not be allowed to umount the filesystem.
> 
> > 
> > Otherwise, the only guarantees I think we've really had is that we won't
> > allow unmount if you hold any actual state on the filesystem (NLM locks,
> > NFSv4 locks, opens, or delegations).
> 
> Those resources hold the reference of vfsmnt.
> 
> > 
> > If a filesystem is exported but no clients hold state on it, then it's
> > currently mostly chance whether the unmount succeeds or not.  So we're
> > probably free to change the behavior in this case.  I'd be inclined to
> > allow the unmount, but haven't thought this through carefully.
> 
> If client mount a nfsserver succeed without holds state, 
> nfs server umounts the exported filesystem, 
> client also think the filesystem is valid, but it is umounted.

This is no different from "exportfs -au" being run on the server, thus
unexporting the filesystem and making in unavailable to the client, even
though the client has it mounted.

I think we need to give the server admin control of their filesystems, and
assume they won't do something that they don't really want to do.



> 
> > 
> > It could also be useful to have the ability to force an unmount even in
> > the presence of locks.  That's not a safe default, but an
> > "allow_force_unmount" export option might be useful.

We already have a mechanism to forcibly drop any locks by writing some magic
to /proc/fs/nfsd/unlock_{ip,filesystem}.  I don't think we need any more.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]