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@redhat.com> wrote: On 2/5/20 3:21 PM, des@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.com... > 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.com... > > 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@lists.nfs-ganesha.org > To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org >

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@ietf.org and ask for clarification there. Cheers, -- Jeff Layton <jlayton@redhat.com>

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. Regards, Deepthi On 06/02/20, 12:46 AM, "Frank Filz" <ffilzlnx@mindspring.com> wrote: > -----Original Message----- > From: Jeff Layton [mailto:jlayton@redhat.com] > Sent: Wednesday, February 5, 2020 11:09 AM > To: Soumya Koduri <skoduri@redhat.com>; Deepthi Shivaramu > <des@vmware.com>; devel@lists.nfs-ganesha.org > Cc: Frank Filz <ffilz@redhat.com> > 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@ietf.org and > ask for clarification there. Yea, it looks like we should allow READ on a WRITE Delegation stateid. We probably should not grant a WRITE delegation if there is a conflicting OPEN with Deny Read (which unless we should rarely see, usually you either see Deny Write or Deny Read/Write, though one can probably make a case for OPEN Write/Deny Write, OPEN Read/Deny Read for some kind of pipe like thing that will have a single producer and a single consumer. On the other hand, a delegation would be inappropriate for either side of that... Frank

Thanks Frank for confirming the bugs. I will go ahead and fix the first one and send out for review since its urgent. Like I have explained before, with ubuntu clients and delegation enabled the reads after write don’t get through at all without this fix. The second one I will request Soumya to have a look and decide the fix since it involves multiple layers. And also like Frank mentioned linux NFS clients do not set deny mode, so we need to fix it to be RFC compliant. Regards, Deepthi On 06/02/20, 8:13 PM, "Frank Filz" <ffilzlnx@mindspring.com> wrote: > 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. Yes, we definitely need to fix this one. > 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. And yes, that code is wrong. This check would have to be done down in the FSAL because that's where we keep the share data now (in struct fsal_share). What we should do is something like this: if ((state_open-> state_data.share.share_access & OPEN4_SHARE_ACCESS_READ) == 0) { if (share->share_deny_read > ((state_open->state_data.share.share_deny & OPEN4_SHARE_DENY_READ) == 0)) { /* We may or may not have a deny read but at least one other state holder has a deny read */ Don't allow; } } Names of things need to change to what's appropriate in the FSAL code, but the general idea is there. We will need to understand if the thread synchronization is such that we would block a new OPEN Deny_Read while this READ is in progress, on the other hand, I'm not going to stress out about that technicality... WE have WRITE access which means WE can change the file, someone else trying to prevent other processes from READing the file isn't actually a big deal. In fact, really we should ALWAYS allow the READ, I'm not sure what use cases anyone had in mind where we would want to block the READ. NORMALLY if DENY modes are being used, someone with WRITE access will Deny_Read and Deny_Write and will effectively have exclusive access to the file. So we're talking about what should be done in some weird corner case. So yea, we have a problem that should be fixed, and we can get close to what RFC 7530 says we should do but maybe not exactly there. I think that’s good enough. Note that the code as written will work fine for Linux clients... They never use DENY modes... I'm guessing Windows clients don't try and READ if they don't have READ access... A Windows client doing read/modify/write of a page or whatever is going to open the file READ/WRITE (AND probably DENY_READ/DENY_WRITE since Windows defaults to that...). I don't know if there are other clients that use DENY modes (there was an interesting patch set several years ago to add optional deny modes to Linux open system call, it died on the branch). Frank