On Wed, Dec 18, 2013 at 3:44 AM, Benny Halevy <bhalevy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
>>> Otherwise the lockowner may by added to "matches" more than once.
>>>
>>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
