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.
Cheers,
--
Jeff Layton <jlayton(a)redhat.com>