Re: [PATCH 5/7] NFS: Convert lookups of the open context to RCU

[Olga Kornievskaia <aglo@umich.edu>]

On Thu, Oct 4, 2018 at 12:42 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Thu, 2018-10-04 at 12:31 -0400, Olga Kornievskaia wrote:
> > On Thu, Oct 4, 2018 at 12:13 PM Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > >
> > > On Thu, 2018-10-04 at 11:49 -0400, Olga Kornievskaia wrote:
> > > > On Thu, Oct 4, 2018 at 11:22 AM Trond Myklebust <
> > > > trondmy@hammerspace.com> wrote:
> > > > >
> > > > > Hi Olga,
> > > > >
> > > > > On Wed, 2018-10-03 at 14:38 -0400, Olga Kornievskaia wrote:
> > > > > > Hi Trond,
> > > > > >
> > > > > > Here's why the ordering of the "open_files" list matters and
> > > > > > changes/fixes the existing problem.
> > > > > >
> > > > > > When we first open the file for writing and get a delegation,
> > > > > > it's
> > > > > > the
> > > > > > first one on the list. When we opened the file again for the
> > > > > > same
> > > > > > mode
> > > > > > type, then before the patch, the new entry is inserted before
> > > > > > what's
> > > > > > already on the list. Both of these files share the same
> > > > > > nfs4_state
> > > > > > that's marked delegated.
> > > > > >
> > > > > > Once we receive a delegation recall, in
> > > > > > delegation_claim_opens()
> > > > > > we
> > > > > > walk the list. First one will be the 2nd open. It's marked
> > > > > > delegated
> > > > > > but after calling nfs4_open_delegation_recall() the
> > > > > > delegation
> > > > > > flag
> > > > > > is
> > > > > > cleared. The 2nd open doesn't have the lock associated with
> > > > > > it.
> > > > > > So no
> > > > > > lock is reclaimed. We now go to the 2nd entry in the
> > > > > > open_file
> > > > > > list
> > > > > > which is the 1st open but now the delegation flag is cleared
> > > > > > so
> > > > > > we
> > > > > > never recover the lock.
> > > > > >
> > > > > > Any of the opens on the open_list can be the lock holder and
> > > > > > we
> > > > > > can't
> > > > > > clear the delegation flag on the first treatment of the
> > > > > > delegated
> > > > > > open
> > > > > > because it might not be the owner of the lock.
> > > > > >
> > > > > > I'm trying to figure out how I would fix it but I thought I'd
> > > > > > send
> > > > > > this for your comments.
> > > > >
> > > > > The expectation here is that once we find an open context with
> > > > > a
> > > > > stateid that needs to be reclaimed or recovered, we recover
> > > > > _all_
> > > > > the
> > > > > state associated with that stateid.
> > > > > IOW: the expectation is that we first look at the open state,
> > > > > and
> > > > > (depending on whether this is a write delegation or a read
> > > > > delegation)
> > > > > run through a set of calls to nfs4_open_recover_helper() that
> > > > > will
> > > > > recover all outstanding open state for this file.
> > > >
> > > > That's true. I see that it will recover all the outstanding opens
> > > > for
> > > > this file.
> > > >
> > > > > We then iterate through all the lock stateids for the file and
> > > > > recover
> > > > > those.
> > > >
> > > > However this is not true. Because we pass in a specific
> > > > nfs_open_context into the nfs_delegation_claim_locks() and while
> > > > looping thru the list of locks for the file we compare if the
> > > > open
> > > > context associated with the file lock is same as the passed in
> > > > context. The passed in context is that of the first
> > > > nfs_open_context
> > > > that was marked delegated and not necessarily the context that
> > > > hold
> > > > the locks. That's the problem.
> > > >
> > > > While we are looping thru the locks for the file, we need to be
> > > > checking against any and all the nfs_open_context that are
> > > > associated
> > > > with the file and recovering those locks. I'm still not sure how
> > > > to
> > > > do
> > > > it.
> > > >
> > >
> > > Interesting. Why are we checking that open context? I can't see any
> > > reason why we would want to do that.
> >
> > I'm thinking it's probably because we might have open context
> > (non-delegated) on this file that holds a lock (say we opened with
> > O_DIRECT and got a lock; that lock was sent to the server so doesn't
> > need to recovered). Then we opened a file again and got a delegation
> > and got a different lock. When we are looping thru the locks for the
> > file, we only need to recover the 2nd one.
> >
> > I'm thinking can't we check that the passed in state is the same that
> > is what we get nfs_file_open_context(fl->fl_file)->state ? I'm
> > testing
> > it now...
>
> There is nothing in the protocol that stops you from recovering a lock
> twice (unless the server does Microsoft stacked locks, which our client
> doesn't support). I'd prefer to just go for simplicity here, and
> recover everything.
>
> ...and yes, I agree that we should select the locks to recover based on
> the state. As I said, that was the expectation in the first place.

Ok. I sent out a patch but not sure if you want something that just
removes the check all together.





>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>