Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Feb 14, 2014 at 09:45:24PM -0500, J. Bruce Fields wrote:
> On Fri, Feb 14, 2014 at 05:40:55PM -0800, Eric W. Biederman wrote:
> > "J. Bruce Fields" <bfields@fieldses.org> writes:
> > 
> > > On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
> > >> A user was running into errors from an NFS export of a subvolume that had a
> > >> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
> > >> to find an existing dentry for the subvolume in the case that the root subvol
> > >> has already been mounted, or a dummy one is allocated in the case that the root
> > >> subvol has not already been mounted.  This allows us to connect the dentry later
> > >> on if we wander into the path.  However if we don't ever wander into the path we
> > >> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
> > >> appear to cause any problems but it is annoying nonetheless, so simply unset
> > >> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
> > >> use d_materialise_unique() instead which will make everything play nicely
> > >> together and reconnect stuff if we wander into the defaul subvol path from a
> > >> different way.  With this patch I'm no longer getting the NFS errors when
> > >> exporting a volume that has been mounted with a default subvol set.  Thanks,
> > >
> > > Looks obviously correct, but based on a quick grep, there are four
> > > d_obtain_alias callers outside export methods:
> > >
> > > 	- btrfs/super.c:get_default_root()
> > > 	- fs/ceph/super.c:open_root_dentry()
> > > 	- fs/nfs/getroot.c:nfs_get_root()
> > > 	- fs/nilfs2/super.c:nilfs_get_root_dentry()
> > >
> > > It'd be nice to give them a common d_obtain_alias variant instead of
> > > making them all clear this by hand.
> > 
> > I am in favor of one small fix at a time, so that progress is made and
> > fixing something just for btrfs seems reasonable for the short term.
> > 
> > > Of those nilfs2 also uses d_splice_alias.  I think that problem would
> > > best be solved by fixing d_splice_alias not to require a
> > > DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.
> > 
> > You mean by renaming d_splice_alias d_materialise_unique?
> > 
> > Or is there a useful distinction you see that should be preserved
> > between the two methods?
> > 
> > Right now my inclination is that everyone should just use
> > d_materialise_unique and we should kill d_splice_alias.
> 
> Probably.  One remaining distinction:
> 
> 	- In the local filesystem case if you discover a directory is
> 	  already aliased elsewhere, you have a corrupted filesystem and
> 	  want to error out the lookup.  (Didn't you propose a patch to
> 	  do something like that before?)
> 	- In the distributed filesystem this is perfectly normal and we
> 	  want to do our best to fix up our local cache to represent
> 	  remote reality.

The following keeps the d_splice_alias/d_materialise_unique distinction
and (hopefully) fixes Josef's bug, and does a little cleanup (including
your suggested DISCONNECTED->CONNECTING rename).

Any better idea for the naming of d_materialise_unique?

I also didn't try to merge the implementations--the merged
d_splice_alias/d_materialise_unique was a little uglier than I expected.
I'll keep messing around with it though.

--b.