According to page 112 in RFC 7530, NFSv4 read operation should be allowed on a open state with share access OPEN4_SHARE_ACCESS_WRITE to accommodate clients whose write implementation may unavoidably do reads (e.g., due to buffer cache constraints). This condition is taken care in normal flow in nfs4_read() in here: https://github.com/nfs-ganesha/nfs-ganesha/blob/next/src/Protocols/NFS/nf...

But if the open has granted a write delegation(OPEN_DELEGATE_WRITE), then the check for deleg state in nfs4_read() does not allow Read op as per RFC mentioned above but return NFS4ERR_BAD_STATEID. https://github.com/nfs-ganesha/nfs-ganesha/blob/next/src/Protocols/NFS/nf... case STATE_TYPE_DELEG: /* Check if the delegation state allows READ or * if the open share state allows READ access * in case of WRITE delegation */ sdeleg = &state_found->state_data.deleg; if (!((sdeleg->sd_type & OPEN_DELEGATE_READ) || (sdeleg->share_access & OPEN4_SHARE_ACCESS_READ))) { /* Invalid delegation for this operation. */ LogDebug(COMPONENT_STATE, "Delegation type:%d state:%d", sdeleg->sd_type, sdeleg->sd_state); res_READ4->status = NFS4ERR_BAD_STATEID; goto out; } state_open = NULL; break; The ubuntu nfs client when it wants to write data, always opens file with OPEN4_SHARE_ACCESS_WRITE and not OPEN4_SHARE_ACCESS_BOTH like other linux clients, so with delegation enabled if we just do "echo data > file.txt ; cat file.txt", it hangs forever with read op getting NFS4ERR_BAD_STATEID and client retrying forever. I feel this whole block under 'case STATE_TYPE_DELEG' is not needed at all, as the share access check will anyway happen below and we need not check the delegation state while doing read operation.

_______________________________________________ Devel mailing list -- devel(a)lists.nfs-ganesha.org To unsubscribe send an email to devel-leave(a)lists.nfs-ganesha.org

Soumya, I got your point that open_state may not exist when the reads are done with deleg_state, so you need this block of code. So the check here should have been similar to the check done below as per RFC. But I did not understand the point of checking share_deny as well. When a client performs open with share_deny set to say read, it means other clients should be denied read but not itself right? So what is the point of checking my own 'sdeleg->share_deny' or even 'state_open->state_data.share.share_deny' as done in share lock check below.

That is the reason I said, we do not need any check of my own share_access or deleg_type checks while doing read, the conflicting lock and delegation checks will anyway be performed by open already. Regards, Deepthi On 05/02/20, 4:03 PM, "Soumya Koduri" <skoduri(a)redhat.com&gt; wrote: On 2/5/20 3:21 PM, des(a)vmware.com wrote: > According to page 112 in RFC 7530, NFSv4 read operation should be allowed on a open state with share access OPEN4_SHARE_ACCESS_WRITE to accommodate clients whose write implementation may unavoidably do reads (e.g., due to buffer cache constraints). > This condition is taken care in normal flow in nfs4_read() in here: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.... > RFC doesn't clearly state if the same applies even in case of delegations. So I am not sure about it though logically it may make sense to allow reads if its the same client. Nevertheless, we do need below block. In case there is delegation granted, READs are performed using delegation state and there is no open state and hence shall bypass share_access checks below. Maybe we can check for sdeleg->share_deny instead of sdeleg->share_access in this block to verify if ACCESS_READ is denied. Thanks, Soumya > But if the open has granted a write delegation(OPEN_DELEGATE_WRITE), then the check for deleg state in nfs4_read() does not allow Read op as per RFC mentioned above but return NFS4ERR_BAD_STATEID. > > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.... > > case STATE_TYPE_DELEG: > /* Check if the delegation state allows READ or > * if the open share state allows READ access > * in case of WRITE delegation */ > sdeleg = &state_found->state_data.deleg; > if (!((sdeleg->sd_type & OPEN_DELEGATE_READ) || > (sdeleg->share_access & OPEN4_SHARE_ACCESS_READ))) { > /* Invalid delegation for this operation. */ > LogDebug(COMPONENT_STATE, > "Delegation type:%d state:%d", > sdeleg->sd_type, > sdeleg->sd_state); > res_READ4->status = NFS4ERR_BAD_STATEID; > goto out; > } > > state_open = NULL; > break; > > The ubuntu nfs client when it wants to write data, always opens file with OPEN4_SHARE_ACCESS_WRITE and not OPEN4_SHARE_ACCESS_BOTH like other linux clients, so with delegation enabled if we just do "echo data > file.txt ; cat file.txt", it hangs forever with read op getting NFS4ERR_BAD_STATEID and client retrying forever. > > I feel this whole block under 'case STATE_TYPE_DELEG' is not needed at all, as the share access check will anyway happen below and we need not check the delegation state while doing read operation. > _______________________________________________ > Devel mailing list -- devel(a)lists.nfs-ganesha.org > To unsubscribe send an email to devel-leave(a)lists.nfs-ganesha.org >

-----Original Message----- From: Jeff Layton [mailto:jlayton@redhat.com] Sent: Wednesday, February 5, 2020 11:09 AM To: Soumya Koduri <skoduri(a)redhat.com>; Deepthi Shivaramu <des(a)vmware.com>; devel(a)lists.nfs-ganesha.org Cc: Frank Filz <ffilz(a)redhat.com&gt; Subject: [NFS-Ganesha-Devel] Re: NFSv4 Read not allowed on file opened with OPEN4_SHARE_ACCESS_WRITE and got write delegation(OPEN_DELEGATE_WRITE) On Thu, 2020-02-06 at 00:17 +0530, Soumya Koduri wrote: > Hi Deepthi, > > On 2/5/20 4:28 PM, Deepthi Shivaramu wrote: > > Soumya, > > I got your point that open_state may not exist when the reads are done with deleg_state, so you need this block of code. So the check here should have been similar to the check done below as per RFC. > > > > But I did not understand the point of checking share_deny as well. When a client performs open with share_deny set to say read, it means other clients should be denied read but not itself right? So what is the point of checking my own 'sdeleg->share_deny' or even 'state_open->state_data.share.share_deny' as done in share lock check below. > > you are right. Those checks would have already been performed in OPEN, > unless its special stateID and it bypasses OPEN which in case > state_found shall be NULL and there are checks for Deleg and share > conflicts in the subsequent block. > Request Frank/Jeff to comment on if/why its needed for regular READ > and if the same applies in case of delegation as well. > It looks like there is a comment that explains this in the code: /* Even if file is open for write, the client * may do accidently read operation (caching). * Because of this, READ is allowed if not * explicitly denied. See page 112 in RFC 7530 * for more details. */ IIUC -- the client can do a read on a write-only stateid -- eg to do a read-modify- write cycle to update a full page in the pagecache. The server _should_ generally allow that, but if someone has set DENY_READ then it should not. Thus the check. I'd suggest (though I'm not sure) that you still need to check whether you have a delegation in play or not. One would hope that sane servers would avoid passing out delegations that might conflict with deny modes. In practice, it's probably easiest to just assume that an open for WRITE also implies READ access. The RFC seems to allow for that (see p112 again). If you have real questions about this, you may want to email nfsv4(a)ietf.org and ask for clarification there.

To summarize, there are 2 things we are discussing here: 1. Allow READ operation when OPEN is done with SHARE_ACCESS_WRITE and WRITE delegation is granted. This is not happening today and we all agree that we need to fix this.

2. In nfs4_read() function, we want to check if any OPENs from other clients exist with 'deny=read' and fail the READ(for both with and without delegation). But the point I am trying to make is, we are not checking here for other clients' deny modes but we are checking our own deny mode. So this will mean, If say client A does an OPEN with "share_access = write, deny = read", it will deny clientA to read as well instead of just denying READ ops from other clients. /* Even if file is open for write, the client * may do accidently read operation (caching). * Because of this, READ is allowed if not * explicitly denied. See page 112 in RFC 7530 * for more details. */ if (state_open->state_data.share.share_deny &. <---- this is the deny mode of the state which came in READ op. OPEN4_SHARE_DENY_READ) { /* Bad open mode, return NFS4ERR_OPENMODE */ res_READ4->status = NFS4ERR_OPENMODE; Please correct me if I am wrong in my understanding here.