Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference

From: "J. Bruce Fields" <bfields@fieldses.org>
To: Jeff Layton <jeff.layton@primarydata.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 3/3] nfsd: allow find_any_file to return a fi_deleg_file reference
Date: Mon, 11 Aug 2014 13:41:32 -0400	[thread overview]
Message-ID: <20140811174132.GD9095@fieldses.org> (raw)
In-Reply-To: <20140811132053.7f058eb1@tlielax.poochiereds.net>

On Mon, Aug 11, 2014 at 01:20:53PM -0400, Jeff Layton wrote:
> On Mon, 11 Aug 2014 13:09:37 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Aug 11, 2014 at 12:40:39PM -0400, Jeff Layton wrote:
> > > On Mon, 11 Aug 2014 12:08:40 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Sat, Aug 09, 2014 at 10:22:42AM -0400, Jeff Layton wrote:
> > > > > It's possible that we'll have a nfs4_file that has nothing in its fds
> > > > > array, but that has a populated fi_deleg_file field.
> > > > 
> > > > Is it really possible?  On a quick skim it looks like this is only used
> > > > in the presence of lock stateid's, when we should have an open.
> > > > 
> > > > OK with the cleanup I'm just not seeing a reason either one way or the
> > > > other for the fi_deleg_file change.
> > > > 
> > > > --b.
> > > > 
> > > 
> > > You're correct. The existing code doesn't specifically require this
> > > patch since find_any_file is only used with lock stateids. It
> > > should be harmless but it won't hurt anything to drop it.
> > > 
> > > I did however need this when I rebased some pnfsd patches on top of the
> > > state overhaul, and it seemed like a reasonable change from a
> > > "future-proofing" standpoint.
> > 
> > So layout operations depend on this somehow?  (But layouts can outlast
> > delegations, so that must not be it.)
> > 
> > I'm not opposed to future-proofing as long as we have some evidence
> > about the future.
> > 
> 
> The code I'm working on does depend on this, but it's may not be
> correct for any arbitrary filesystem.
> 
> This filesystem sets return_on_close, and we automatically return the
> layout to the fs when all of the open and deleg stateids go away. If
> the last one to drop its reference happens to be the delegation
> stateid, then you may have nothing in the fi_fds slots, and no way to
> get to the inode in order to return the layout.
> 
> I've been toying with the idea of keeping an inode reference in the
> layout state, but I'm not sure if it's the right thing to do...

OK, makes sense, thanks for the explanation.  But I think I'll drop this
third patch for now.

--b.

> 
> > > Do you intend to the take the first two in the series? I would like to
> > > see those go in since they move the lease removal outside of spinlocks.
> > 
> > Yes, just waiting for -rc1 comes out to push out a for-3.18 tree.
> > 
> 
> Sounds good -- no rush on them.
> 
> Thanks!
> -- 
> Jeff Layton <jlayton@primarydata.com>