On Mon, 11 May 2015 21:08:47 +0800 Kinglong Mee <kinglongmee@xxxxxxxxx> 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:
pgpX6IzkfYZur.pgp
Description: OpenPGP digital signature
