On Thu, Oct 4, 2018 at 12:42 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx > >
