On 7/8/24 11:56 AM, Sagi Grimberg wrote:
On 08/07/2024 20:39, Dai Ngo wrote:
On 7/8/24 7:18 AM, Chuck Lever III wrote:
On Jul 7, 2024, at 7:06 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
On Sun, 2024-07-07 at 01:42 +0300, Sagi Grimberg wrote:
Many applications open files with O_WRONLY, fully intending to write
into the opened file. There is no reason why these applications
should
not enjoy a write delegation handed to them.
Cc: Dai Ngo <dai.ngo@xxxxxxxxxx>
Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
---
Note: I couldn't find any reason to why the initial implementation
chose
to offer write delegations only to NFS4_SHARE_ACCESS_BOTH, but it
seemed
like an oversight to me. So I figured why not just send it out and
see who
objects...
fs/nfsd/nfs4state.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a20c2c9d7d45..69d576b19eb6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5784,15 +5784,15 @@ nfs4_set_delegation(struct nfsd4_open
*open, struct nfs4_ol_stateid *stp,
* "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
* on its own, all opens."
*
- * Furthermore the client can use a write delegation for most READ
- * operations as well, so we require a O_RDWR file here.
- *
- * Offer a write delegation in the case of a BOTH open, and ensure
- * we get the O_RDWR descriptor.
+ * Offer a write delegation in the case of a BOTH open (ensure
+ * a O_RDWR descriptor) Or WRONLY open (with a O_WRONLY
descriptor).
*/
if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) ==
NFS4_SHARE_ACCESS_BOTH) {
nf = find_rw_file(fp);
dl_type = NFS4_OPEN_DELEGATE_WRITE;
+ } else if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+ nf = find_writeable_file(fp);
+ dl_type = NFS4_OPEN_DELEGATE_WRITE;
}
/*
Thanks Sagi, I'm glad to see this posting!
I *think* the main reason we limited this before is because a write
delegation is really a read/write delegation. There is no such
thing as
a write-only delegation.
I recall (quite dimly) that Dai found some bad behavior
in a subtle corner case, so we decided to leave this on
the table as a possible future enhancement. Adding Dai.
If I remember correctly, granting write delegation on OPEN with
NFS4_SHARE_ACCESS_WRITE without additional changes causes the git
test to fail.
That's good to know.
Have you reported this over the list before?
I submitted a patch to allow the client to use write delegation, granted
on OPEN with NFS4_SHARE_ACCESS_WRITE, to read:
https://lore.kernel.org/linux-nfs/1688089960-24568-3-git-send-email-dai.ngo@xxxxxxxxxx/
This patch fixed the problem with the git test. However Jeff reported another
problem might be related to this patch:
https://lore.kernel.org/linux-nfs/6756f84c4ff92845298ce4d7e3c4f0fb20671d3d.camel@xxxxxxxxxx
I did not investigate this pynfs problem since we dropped this support.
The cause of the failure is because the client does read-modify-write
using the write delegation stateid.
Does the fact that this is the delegation stateid matters here? How?
The use of write delegation stateid for read only matters if the server
decides to allow it to be used for read since it's safe because no other
clients can write to the same file.
-Dai
This happens when an application
does partial write the client side, the Linux client reads the whole
page from the server, modifies a section and writes the whole page
back.
Well, the test shouldn't fail for sure. There are servers out there that
hand out write delegations for O_WRONLY opens, so the client is already
facing this issue today (I guess no one noticed).
I think this is the case with the t0000-basic test in the git
test suite.
I think this behavior is allowed in section 9.1.2 of RFC 8881.
Yes, agree.
