On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@xxxxxxxxxxx> wrote:
>
>
>
> On 07/07/2024 22:05, Rick Macklem wrote:
> > On Sun, Jul 7, 2024 at 4:07 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;
> >>> }
> >>>
> >>> /*
> >>
> >> 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.
> >>
> >> Suppose the user is prevented from doing reads against the inode (by
> >> permission bits or ACLs). The server gives out a WRITE delegation on a
> >> O_WRONLY open. Will the client allow cached opens for read regardless
> >> of the server's permissions? Or, does it know to check vs. the server
> >> if the client tries to do an open for read in this situation?
> > I was curious and tried a simple test yesterday, using the FreeBSD server
> > configured to issue a write delegation for a write-only open.
> > The test simply opened write-only first and then read-only, for a file
> > with mode 222. It worked as expected for both the Linux and FreeBSD
> > clients (ie. returned a failure for the read-only open).
> > I've attached the packet capture for the Linux client, in case you are
> > interested.
>
> Nice, thanks for testing!
>
> >
> > I do believe this is allowed by the RFCs. In fact I think the RFCs
> > allow a server
> > to issue a write delegation for a read open (not that I am convinced that is a
> > good idea). The main thing to check is what the ACE in the write delegation
> > reply looks like, since my understanding is that the client is expected to do
> > an Access unless the ACE allows access.
> > Here's a little snippet:
> > nfsace4 permissions; /* Defines users who don't
> > need an ACCESS call as
> > part of a delegated
> > open. */
>
> Yes, I had the same understanding...
>
> >
> > Now, is it a good idea to do this?
> > Well, my opinion (as the outsider;-) is that the server should follow whatever
> > the client requests, as far as practicable. ie. The OPEN_WANT flags:
> > const OPEN4_SHARE_ACCESS_WANT_NO_PREFERENCE = 0x0000;
> > const OPEN4_SHARE_ACCESS_WANT_READ_DELEG = 0x0100;
> > const OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG = 0x0200;
> > const OPEN4_SHARE_ACCESS_WANT_ANY_DELEG = 0x0300;
> > const OPEN4_SHARE_ACCESS_WANT_NO_DELEG = 0x0400;
> > If the client does not provide these (4.0 or 4.1/4.2, but no flags), then I
> > suppose I might be concerned that there is a buggy client out there that
> > would do what Jeff suggested (ie. assume it can open the file for reading
> > after getting a write delegation).
>
> Well, that is obviously not what the Linux client is asking for today,
> but even if it
> did, what would it set in the OPEN_WANT flags for O_WRONLY open? it would
> set OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG.
I didn't think to mention that the client was a rather old 6.3 kernel.
If the Linux client is still not setting OPEN4_SHARE_ACCESS_WANT_xxx
flags, then maybe it should do so?
And maybe the client developers will be encouraged to set them, if the server
uses them?
I'll admit I had assumed they weren't there just because my kernel is
out of date.
rick
>
> I do agree that there is an argument to be made that Linux nfsd should not
> hand out any delegation that is not explicitly solicited by the client,
> but I don't see
> how this particular delegation against a wronly open is any different
> from any
> other type of delegation that Linux nfsd hands out today.
