| Message ID | 1392737765-41942-1-git-send-email-andros@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2014-02-18 at 10:36 -0500, andros@netapp.com wrote: > From: Andy Adamson <andros@netapp.com> > > Do not return an error when nfs4_copy_delegation_stateid succeeds. > > Signed-off-by: Andy Adamson <andros@netapp.com> > --- > fs/nfs/nfs4state.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > index ba18958..1cfde97 100644 > --- a/fs/nfs/nfs4state.c > +++ b/fs/nfs/nfs4state.c > @@ -1120,8 +1120,11 @@ int nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state, > if (ret == -EIO) > /* A lost lock - don't even consider delegations */ > goto out; > - if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) > + /* returns true if delegation stateid found and copied */ > + if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) { > + ret = 0; > goto out; > + } > if (ret != -ENOENT) > /* nfs4_copy_delegation_stateid() didn't over-write > * dst, so it still has the lock stateid which we now Ouch! That looks like it would trigger looping in both the read and write code when we're holding a delegation. Is that what you end up seeing? It looks like it was introduced by commit ef1820f9be27b...
On Feb 18, 2014, at 11:33 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Tue, 2014-02-18 at 10:36 -0500, andros@netapp.com wrote: >> From: Andy Adamson <andros@netapp.com> >> >> Do not return an error when nfs4_copy_delegation_stateid succeeds. >> >> Signed-off-by: Andy Adamson <andros@netapp.com> >> --- >> fs/nfs/nfs4state.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c >> index ba18958..1cfde97 100644 >> --- a/fs/nfs/nfs4state.c >> +++ b/fs/nfs/nfs4state.c >> @@ -1120,8 +1120,11 @@ int nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state, >> if (ret == -EIO) >> /* A lost lock - don't even consider delegations */ >> goto out; >> - if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) >> + /* returns true if delegation stateid found and copied */ >> + if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) { >> + ret = 0; >> goto out; >> + } >> if (ret != -ENOENT) >> /* nfs4_copy_delegation_stateid() didn't over-write >> * dst, so it still has the lock stateid which we now > > Ouch! That looks like it would trigger looping in both the read and > write code when we're holding a delegation. Is that what you end up > seeing? Actually, I saw it trying to run Anna’s intra server to server copy code. In this case, the server side copy failed and normal READ/WRITE from the client then suceeded. In that case I did not see any looping... > > It looks like it was introduced by commit ef1820f9be27b… Yep. Should I include that commit in the comment? —>Andy > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-02-18 at 17:56 +0000, Adamson, Andy wrote: > On Feb 18, 2014, at 11:33 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > > On Tue, 2014-02-18 at 10:36 -0500, andros@netapp.com wrote: > >> From: Andy Adamson <andros@netapp.com> > >> > >> Do not return an error when nfs4_copy_delegation_stateid succeeds. > >> > >> Signed-off-by: Andy Adamson <andros@netapp.com> > >> --- > >> fs/nfs/nfs4state.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > >> index ba18958..1cfde97 100644 > >> --- a/fs/nfs/nfs4state.c > >> +++ b/fs/nfs/nfs4state.c > >> @@ -1120,8 +1120,11 @@ int nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state, > >> if (ret == -EIO) > >> /* A lost lock - don't even consider delegations */ > >> goto out; > >> - if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) > >> + /* returns true if delegation stateid found and copied */ > >> + if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) { > >> + ret = 0; > >> goto out; > >> + } > >> if (ret != -ENOENT) > >> /* nfs4_copy_delegation_stateid() didn't over-write > >> * dst, so it still has the lock stateid which we now > > > > Ouch! That looks like it would trigger looping in both the read and > > write code when we're holding a delegation. Is that what you end up > > seeing? > > Actually, I saw it trying to run Anna’s intra server to server copy code. In this case, the server side copy failed and normal READ/WRITE from the client then suceeded. In that case I did not see any looping... > > > > > It looks like it was introduced by commit ef1820f9be27b… > > Yep. Should I include that commit in the comment? I added a 'Fixes:' and 'Link:' to the patch before I applied it. I also added a stable fix request for 3.12+...
On Tue, 18 Feb 2014 11:33:42 -0500 Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Tue, 2014-02-18 at 10:36 -0500, andros@netapp.com wrote: > > From: Andy Adamson <andros@netapp.com> > > > > Do not return an error when nfs4_copy_delegation_stateid succeeds. > > > > Signed-off-by: Andy Adamson <andros@netapp.com> > > --- > > fs/nfs/nfs4state.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > > index ba18958..1cfde97 100644 > > --- a/fs/nfs/nfs4state.c > > +++ b/fs/nfs/nfs4state.c > > @@ -1120,8 +1120,11 @@ int nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state, > > if (ret == -EIO) > > /* A lost lock - don't even consider delegations */ > > goto out; > > - if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) > > + /* returns true if delegation stateid found and copied */ > > + if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) { > > + ret = 0; > > goto out; > > + } > > if (ret != -ENOENT) > > /* nfs4_copy_delegation_stateid() didn't over-write > > * dst, so it still has the lock stateid which we now > > Ouch! That looks like it would trigger looping in both the read and > write code when we're holding a delegation. Is that what you end up > seeing? > > It looks like it was introduced by commit ef1820f9be27b... > Oops- that was a little careless. Thanks for the heads-up! NeilBrown
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index ba18958..1cfde97 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -1120,8 +1120,11 @@ int nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state, if (ret == -EIO) /* A lost lock - don't even consider delegations */ goto out; - if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) + /* returns true if delegation stateid found and copied */ + if (nfs4_copy_delegation_stateid(dst, state->inode, fmode)) { + ret = 0; goto out; + } if (ret != -ENOENT) /* nfs4_copy_delegation_stateid() didn't over-write * dst, so it still has the lock stateid which we now