> On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
> ---
> 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.
> set_deny(open->op_share_deny, stp);
> fp->fi_share_deny |=
> --
> 2.37.1 (Apple Git-137.1)
>
--
Chuck Lever
On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>
> > On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
> > ---
> > 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);
--
Jeff Layton <[email protected]>
On 2/2/23 1:22 PM, Jeff Layton wrote:
> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
not sure what I was thinking. The check was totally wrong.
>
> 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);
LGTM.
Thanks,
-Dai
> On Feb 2, 2023, at 4:22 PM, Jeff Layton <[email protected]> wrote:
>
> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>>
>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
>>> ---
>>> 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?
Can you post an official version of this patch with Reported-by
and Fixes tags?
--
Chuck Lever
On 2/3/23 6:50 AM, Chuck Lever III wrote:
>
>> On Feb 2, 2023, at 4:22 PM, Jeff Layton <[email protected]> wrote:
>>
>> On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
>>>> On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
>
>
>
On Fri, 2023-02-03 at 14:50 +0000, Chuck Lever III wrote:
>
> > On Feb 2, 2023, at 4:22 PM, Jeff Layton <[email protected]> wrote:
> >
> > On Thu, 2023-02-02 at 19:41 +0000, Chuck Lever III wrote:
> > >
> > > > On Feb 2, 2023, at 2:36 AM, Pumpkin <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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?
>
> Can you post an official version of this patch with Reported-by
> and Fixes tags?
>
>
Sure, but I may not have time to test it out for a bit though.
--
Jeff Layton <[email protected]>