[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <526a7ea9-f209-16ed-732b-6c105582c18a@oracle.com>
Date: Fri, 3 Feb 2023 09:57:34 -0800
From: dai.ngo@...cle.com
To: Chuck Lever III <chuck.lever@...cle.com>,
Jeff Layton <jlayton@...nel.org>
Cc: Pumpkin <cc85nod@...il.com>,
Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] NFSD: fix deny mode logic in nfs4_upgrade_open
On 2/3/23 6:50 AM, Chuck Lever III wrote:
>
>> On Feb 2, 2023, at 4:22 PM, Jeff Layton <jlayton@...nel.org> wrote:
>>
>> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <cc85nod@...il.com> wrote:
>>>>
>>>> If the upgrading deny mode is invalid or conflicts with other client, we
>>>> should try to resolve it, but the if-condition makes those error handling
>>>> cannot be executed.
>>>>
>>>> Signed-off-by: Pumpkin <cc85nod@...il.com>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 4ef529379..ebdfaf0f9 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -5298,7 +5298,7 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
>>>> /* test and set deny mode */
>>>> spin_lock(&fp->fi_lock);
>>>> status = nfs4_file_check_deny(fp, open->op_share_deny);
>>>> - if (status == nfs_ok) {
>>>> + if (status != nfs_ok) {
>>>> if (status != nfserr_share_denied) {
>>> if status == nfs_ok then status will definitely not equal
>>> share_denied. So this check is a bit nonsensical as it stands.
>>>
>>> Usually I prefer "switch (status)" in situations like this
>>> because that avoids this kind of issue and I find it easier
>>> to read quickly.
>>>
>>> Jeff, you are the original author of this function, and
>>> Dai, your commit is the last one to touch this area. Can
>>> you guys have a look? The one-liner looks correct, but I
>>> might be missing something.
>>>
>> Yeah, that code is clearly broken and it looks like it was done in
>> 3d69427151806 (NFSD: add support for share reservation conflict to
>> courteous server).
>>
>> I don't believe that one-liner is correct though. If the result is
>> nfs_ok, then we want to set the deny mode here and that won't happen.
>>
>> Something like this maybe? (completely untested):
>>
>> ---------------8<-------------------
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index c39e43742dd6..af22dfdc6fcc 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -5282,16 +5282,17 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp,
>> /* test and set deny mode */
>> spin_lock(&fp->fi_lock);
>> status = nfs4_file_check_deny(fp, open->op_share_deny);
>> - if (status == nfs_ok) {
>> - if (status != nfserr_share_denied) {
>> - set_deny(open->op_share_deny, stp);
>> - fp->fi_share_deny |=
>> - (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
>> - } else {
>> - if (nfs4_resolve_deny_conflicts_locked(fp, false,
>> - stp, open->op_share_deny, false))
>> - status = nfserr_jukebox;
>> - }
>> + switch (status) {
>> + case nfs_ok:
>> + set_deny(open->op_share_deny, stp);
>> + fp->fi_share_deny |=
>> + (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
>> + break;
>> + case nfserr_share_denied:
>> + if (nfs4_resolve_deny_conflicts_locked(fp, false,
>> + stp, open->op_share_deny, false))
>> + status = nfserr_jukebox;
>> + break;
>> }
>> spin_unlock(&fp->fi_lock);
> Would pynfs have a case or two that could test this?
pynfs has tests for open upgrade and downgrade but it only
tests the upgrade/downgrade functionality without any deny mode
and only from 1 client.
We need a test that does open upgrade/downgrade with share
deny mode and with 2 clients, one is courtesy client. I will
look into creating one.
-Dai
>
> Can you post an official version of this patch with Reported-by
> and Fixes tags?
>
>
> --
> Chuck Lever
>
>
>
Powered by blists - more mailing lists