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