Bruce, while working on the state mutex elimination stuff I saw these two issues
in nfsd4_release_lockowner. These patches are untested but I just wanted to quickly get
your take on them.
[PATCH 1/2] nfsd4: break from inner lookup loop in nfsd4_release_lockowner on first match
looks like a potential list corruption risk.
[PATCH 2/2] nfsd4: ignore nfsv4.1 lockowners in nfsd4_release_lockowner
I'm not sure that 100% needed but since we keep both nfsv4.0 and v4.1 owners
hashed on the same lists we don't want a v4.0 operation to accidentally
touch v4.1 state.
Benny
Hi Benny,
On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <[email protected]> wrote:
> Otherwise the lockowner may by added to "matches" more than once.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
>
changes from v1:
[PATCH v2 1/2] nfsd4: break from inner lookup loop in nfsd4_release_lockowner on first match
- initialize lo_list
- in case locks our found delete all previous matches from temporary list
[PATCH 2/2] nfsd4: ignore nfsv4.1 lockowners in nfsd4_release_lockowner
- added comment
On 12/13/2013 12:01 PM, Benny Halevy wrote:
> Bruce, while working on the state mutex elimination stuff I saw these two issues
> in nfsd4_release_lockowner. These patches are untested but I just wanted to quickly get
> your take on them.
>
> [PATCH 1/2] nfsd4: break from inner lookup loop in nfsd4_release_lockowner on first match
>
> looks like a potential list corruption risk.
>
> [PATCH 2/2] nfsd4: ignore nfsv4.1 lockowners in nfsd4_release_lockowner
>
> I'm not sure that 100% needed but since we keep both nfsv4.0 and v4.1 owners
> hashed on the same lists we don't want a v4.0 operation to accidentally
> touch v4.1 state.
>
> Benny
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Otherwise the lockowner may by added to "matches" more than once.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 0874998..84007b6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4660,6 +4660,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
if (check_for_locks(stp->st_file, lo))
goto out;
list_add(&lo->lo_list, &matches);
+ break;
}
}
/* Clients probably won't expect us to return with some (but not all)
--
1.8.3.1
On 12/16/2013 05:43 PM, Peng Tao wrote:
> Hi Benny,
>
> On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <[email protected]> wrote:
>> Otherwise the lockowner may by added to "matches" more than once.
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> 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
>
On Fri, Dec 13, 2013 at 06:12:43AM -0800, Christoph Hellwig wrote:
> > list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> > if (sop->so_is_open_owner)
> > continue;
> > + if (sop->so_client->cl_minorversion)
> > + continue;
> > if (!same_owner_str(sop, owner, clid))
> > continue;
> > list_for_each_entry(stp, &sop->so_stateids,
>
> This needs at least a good comment as it's not very obvious from
> glancing over the code. That being said is same_owner_str so much
> overhead that it's really worth it?
>
Right, this seems redundant with the cli_id comparison in
same_owner_str. That could be reordered to ensure it precedes the
memcmp if we think that's worthwhile.
--b.
On Fri, Dec 13, 2013 at 12:03:40PM +0200, Benny Halevy wrote:
> Otherwise the lockowner may by added to "matches" more than once.
Whoops, thanks, looks right.
The lo = assignment should probably also be moved up out of this loop.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0874998..84007b6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4660,6 +4660,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> if (check_for_locks(stp->st_file, lo))
> goto out;
> list_add(&lo->lo_list, &matches);
> + break;
> }
> }
> /* Clients probably won't expect us to return with some (but not all)
> --
> 1.8.3.1
>
> list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> if (sop->so_is_open_owner)
> continue;
> + if (sop->so_client->cl_minorversion)
> + continue;
> if (!same_owner_str(sop, owner, clid))
> continue;
> list_for_each_entry(stp, &sop->so_stateids,
This needs at least a good comment as it's not very obvious from
glancing over the code. That being said is same_owner_str so much
overhead that it's really worth it?
RELEASE_LOCKOWNER is a NFSv4.0 operation only so it can quickly skip
lockowners created by nfsv4.1 clients.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b04f765..7d79494 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4652,6 +4652,9 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) {
if (sop->so_is_open_owner)
continue;
+ /* This is NFSv4.0 only operation, skip NFSv4.x lockowners */
+ if (sop->so_client->cl_minorversion)
+ continue;
if (!same_owner_str(sop, owner, clid))
continue;
lo = lockowner(sop);
--
1.8.3.1
On Wed, Dec 18, 2013 at 3:44 AM, Benny Halevy <[email protected]> wrote:
>
>
> On 12/16/2013 05:43 PM, Peng Tao wrote:
>> Hi Benny,
>>
>> On Sun, Dec 15, 2013 at 11:51 PM, Benny Halevy <[email protected]> wrote:
>>> Otherwise the lockowner may by added to "matches" more than once.
>>>
>>> Signed-off-by: Benny Halevy <[email protected]>
>>> ---
>>> 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
RELEASE_LOCKOWNER is a NFSv4.0 operation only so it can quickly skip
lockowners created by nfsv4.1 clients.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 84007b6..00424f2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4652,6 +4652,8 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
list_for_each_entry(sop, &nn->ownerstr_hashtbl[hashval], so_strhash) {
if (sop->so_is_open_owner)
continue;
+ if (sop->so_client->cl_minorversion)
+ continue;
if (!same_owner_str(sop, owner, clid))
continue;
list_for_each_entry(stp, &sop->so_stateids,
--
1.8.3.1
Otherwise the lockowner may by added to "matches" more than once.
Signed-off-by: Benny Halevy <[email protected]>
---
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;
}
}
/* 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();
--
1.8.3.1