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>