| Message ID | 1387122710-23603-1-git-send-email-bhalevy@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Benny, On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <bhalevy@primarydata.com> wrote: > Otherwise the lockowner may by added to "matches" more than once. > > Signed-off-by: Benny Halevy <bhalevy@primarydata.com> > --- > fs/nfsd/nfs4state.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 0874998..b04f765 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4192,6 +4192,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str > /* It is the openowner seqid that will be incremented in encode in the > * case of new lockowners; so increment the lock seqid manually: */ > lo->lo_owner.so_seqid = lock->lk_new_lock_seqid + 1; > + INIT_LIST_HEAD(&lo->lo_list); > hash_lockowner(lo, strhashval, clp, open_stp); > return lo; > } > @@ -4646,7 +4647,6 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > if (status) > goto out; > > - status = nfserr_locks_held; > INIT_LIST_HEAD(&matches); > > list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) { > @@ -4654,25 +4654,30 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > continue; > if (!same_owner_str(sop, owner, clid)) > continue; > + lo = lockowner(sop); > list_for_each_entry(stp, &sop->so_stateids, > st_perstateowner) { > - lo = lockowner(sop); > - if (check_for_locks(stp->st_file, lo)) > - goto out; > + if (check_for_locks(stp->st_file, lo)) { > + status = nfserr_locks_held; > + goto locks_held; > + } > list_add(&lo->lo_list, &matches); > + break; If so_stateids is empty, lockowner is skipped. It was skipped before the patch as well but I guess that need to be fixed, right? Thanks, Tao -- 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 12/16/2013 05:43 PM, Peng Tao wrote: > Hi Benny, > > On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <bhalevy@primarydata.com> wrote: >> Otherwise the lockowner may by added to "matches" more than once. >> >> Signed-off-by: Benny Halevy <bhalevy@primarydata.com> >> --- >> fs/nfsd/nfs4state.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 0874998..b04f765 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -4192,6 +4192,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str >> /* It is the openowner seqid that will be incremented in encode in the >> * case of new lockowners; so increment the lock seqid manually: */ >> lo->lo_owner.so_seqid = lock->lk_new_lock_seqid + 1; >> + INIT_LIST_HEAD(&lo->lo_list); >> hash_lockowner(lo, strhashval, clp, open_stp); >> return lo; >> } >> @@ -4646,7 +4647,6 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, >> if (status) >> goto out; >> >> - status = nfserr_locks_held; >> INIT_LIST_HEAD(&matches); >> >> list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) { >> @@ -4654,25 +4654,30 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, >> continue; >> if (!same_owner_str(sop, owner, clid)) >> continue; >> + lo = lockowner(sop); >> list_for_each_entry(stp, &sop->so_stateids, >> st_perstateowner) { >> - lo = lockowner(sop); >> - if (check_for_locks(stp->st_file, lo)) >> - goto out; >> + if (check_for_locks(stp->st_file, lo)) { >> + status = nfserr_locks_held; >> + goto locks_held; >> + } >> list_add(&lo->lo_list, &matches); >> + break; > If so_stateids is empty, lockowner is skipped. It was skipped before > the patch as well but I guess that need to be fixed, right? I'm not sure that's a valid state at all. Benny > > Thanks, > Tao > -- 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 Sun, Dec 15, 2013 at 05:51:50PM +0200, Benny Halevy wrote: > Otherwise the lockowner may by added to "matches" more than once. > > Signed-off-by: Benny Halevy <bhalevy@primarydata.com> > --- > fs/nfsd/nfs4state.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 0874998..b04f765 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4192,6 +4192,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str > /* It is the openowner seqid that will be incremented in encode in the > * case of new lockowners; so increment the lock seqid manually: */ > lo->lo_owner.so_seqid = lock->lk_new_lock_seqid + 1; > + INIT_LIST_HEAD(&lo->lo_list); This doesn't really fix any bug--we don't depend on this list head being initialized anywhere as far as I can see. If you think it's useful anyway fo rdebugging purposes or something, that's fine, but stick this in a separate patch from the actual bugfix. > hash_lockowner(lo, strhashval, clp, open_stp); > return lo; > } > @@ -4646,7 +4647,6 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > if (status) > goto out; > > - status = nfserr_locks_held; > INIT_LIST_HEAD(&matches); > > list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) { > @@ -4654,25 +4654,30 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, > continue; > if (!same_owner_str(sop, owner, clid)) > continue; > + lo = lockowner(sop); > list_for_each_entry(stp, &sop->so_stateids, > st_perstateowner) { > - lo = lockowner(sop); > - if (check_for_locks(stp->st_file, lo)) > - goto out; > + if (check_for_locks(stp->st_file, lo)) { > + status = nfserr_locks_held; > + goto locks_held; > + } > list_add(&lo->lo_list, &matches); > + break; I'm a little lost here: it looks like if sop->so_stateids has more than one entry, then we'll decide to release lo just because the first entry doesn't have any associated locks (when subsequent entries still might). Instead of breaking at the end I think you just want to move the list_add after the loop, to ensure that we check all the stateid's. > } > } > /* Clients probably won't expect us to return with some (but not all) > * of the lockowner state released; so don't release any until all > * have been checked. */ > status = nfs_ok; > +locks_held: > while (!list_empty(&matches)) { > - lo = list_entry(matches.next, struct nfs4_lockowner, > + lo = list_first_entry(&matches, struct nfs4_lockowner, > lo_list); > /* unhash_stateowner deletes so_perclient only > * for openowners. */ > list_del(&lo->lo_list); > - release_lockowner(lo); > + if (status == nfs_ok) > + release_lockowner(lo); Again, we don't depend on lo_list being initialized anywhere, so this is really a sort of cleanup unrelated to this bugfix. And if you think it may be asking for trouble to leave lo_list on a list that doesn't exist any more, OK, but make that argument in a separate patch. --b. > } > out: > nfs4_unlock_state(); > -- > 1.8.3.1 > -- 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 Wed, Dec 18, 2013 at 3:44 AM, Benny Halevy <bhalevy@primarydata.com> wrote: > > > On 12/16/2013 05:43 PM, Peng Tao wrote: >> Hi Benny, >> >> On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <bhalevy@primarydata.com> wrote: >>> Otherwise the lockowner may by added to "matches" more than once. >>> >>> Signed-off-by: Benny Halevy <bhalevy@primarydata.com> >>> --- >>> fs/nfsd/nfs4state.c | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index 0874998..b04f765 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -4192,6 +4192,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str >>> /* It is the openowner seqid that will be incremented in encode in the >>> * case of new lockowners; so increment the lock seqid manually: */ >>> lo->lo_owner.so_seqid = lock->lk_new_lock_seqid + 1; >>> + INIT_LIST_HEAD(&lo->lo_list); >>> hash_lockowner(lo, strhashval, clp, open_stp); >>> return lo; >>> } >>> @@ -4646,7 +4647,6 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, >>> if (status) >>> goto out; >>> >>> - status = nfserr_locks_held; >>> INIT_LIST_HEAD(&matches); >>> >>> list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) { >>> @@ -4654,25 +4654,30 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, >>> continue; >>> if (!same_owner_str(sop, owner, clid)) >>> continue; >>> + lo = lockowner(sop); >>> list_for_each_entry(stp, &sop->so_stateids, >>> st_perstateowner) { >>> - lo = lockowner(sop); >>> - if (check_for_locks(stp->st_file, lo)) >>> - goto out; >>> + if (check_for_locks(stp->st_file, lo)) { >>> + status = nfserr_locks_held; >>> + goto locks_held; >>> + } >>> list_add(&lo->lo_list, &matches); >>> + break; >> If so_stateids is empty, lockowner is skipped. It was skipped before >> the patch as well but I guess that need to be fixed, right? > > I'm not sure that's a valid state at all. OK. I see the comments in lookup_or_create_lock_state() that says: /* XXX: a lockowner always has exactly one stateid: */ And lookup_or_create_lock_state() does implement that way. So so_stateid always has exactly one member for lockowner. But then the original code (before the patch) is working properly, right? The list_for_each_entry can be replaced with list_first_entry and the added break doesn't seem necessary. Or is the situation somehow obsolete? Thanks, Tao -- 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
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0874998..b04f765 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4192,6 +4192,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str /* It is the openowner seqid that will be incremented in encode in the * case of new lockowners; so increment the lock seqid manually: */ lo->lo_owner.so_seqid = lock->lk_new_lock_seqid + 1; + INIT_LIST_HEAD(&lo->lo_list); hash_lockowner(lo, strhashval, clp, open_stp); return lo; } @@ -4646,7 +4647,6 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, if (status) goto out; - status = nfserr_locks_held; INIT_LIST_HEAD(&matches); list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) { @@ -4654,25 +4654,30 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, continue; if (!same_owner_str(sop, owner, clid)) continue; + lo = lockowner(sop); list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) { - lo = lockowner(sop); - if (check_for_locks(stp->st_file, lo)) - goto out; + if (check_for_locks(stp->st_file, lo)) { + status = nfserr_locks_held; + goto locks_held; + } list_add(&lo->lo_list, &matches); + break; } } /* Clients probably won't expect us to return with some (but not all) * of the lockowner state released; so don't release any until all * have been checked. */ status = nfs_ok; +locks_held: while (!list_empty(&matches)) { - lo = list_entry(matches.next, struct nfs4_lockowner, + lo = list_first_entry(&matches, struct nfs4_lockowner, lo_list); /* unhash_stateowner deletes so_perclient only * for openowners. */ list_del(&lo->lo_list); - release_lockowner(lo); + if (status == nfs_ok) + release_lockowner(lo); } out: nfs4_unlock_state();
Otherwise the lockowner may by added to "matches" more than once. Signed-off-by: Benny Halevy <bhalevy@primarydata.com> --- fs/nfsd/nfs4state.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)