diff mbox series

[rfc] nfsd: offer write delegation for O_WRONLY opens

Message ID 20240706224207.927978-1-sagi@grimberg.me (mailing list archive)
State New
Headers show
Series [rfc] nfsd: offer write delegation for O_WRONLY opens | expand

Commit Message

Sagi Grimberg July 6, 2024, 10:42 p.m. UTC
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@oracle.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
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(-)

Comments

Sagi Grimberg July 6, 2024, 10:43 p.m. UTC | #1
Actually CCing Dai Ngo...
On 07/07/2024 1:42, 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@oracle.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> 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;
>   	}
>   
>   	/*
Jeff Layton July 7, 2024, 11:06 a.m. UTC | #2
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@oracle.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> 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?
Sagi Grimberg July 7, 2024, 6:19 p.m. UTC | #3
On 07/07/2024 14:06, Jeff Layton 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@oracle.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>> 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?

That is a good point. As far as I can tell, the client will still send 
ACCESS to the server
in this case...

looks like vfs do_open->may_open will call inode_permission -> 
nfs_permission -> nfs_do_access...
I don't see any type of bypass if a delegation is present (correctly afaiu).

iiuc the only user that can omit talking to the server for access is the 
permissions field that is returned
in the delegation, and nfsd keeps it cleared, so all users will need to 
issue access (if not already cached).
Rick Macklem July 7, 2024, 7:05 p.m. UTC | #4
On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > ---
> > 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.

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. */

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).
Ideally there would be testing with as many clients as possible (at a
bakeathon?).

rick
>
> --
> Jeff Layton <jlayton@kernel.org>
>
Sagi Grimberg July 7, 2024, 8:21 p.m. UTC | #5
On 07/07/2024 22:05, Rick Macklem wrote:
> On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>> 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 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.
Rick Macklem July 7, 2024, 9:49 p.m. UTC | #6
On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
>
> On 07/07/2024 22:05, Rick Macklem wrote:
> > On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> >>> ---
> >>> 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.
Rick Macklem July 7, 2024, 10:05 p.m. UTC | #7
On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
>
> On 07/07/2024 22:05, Rick Macklem wrote:
> > On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> >>> ---
> >>> 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 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.
I guess the point I was trying to make was that, if a client explicitly
asks for a write delegation, then you issue it and the client screws up,
"the client got what it asked for".

However, if the client has not previously seen a write delegation for this
case and is not asking for it, then if it is buggy in this respect, a
server upgrade
results in a client failure (fun and bothersome to track down, even if
it really a
client bug). ie. There is a risk in this change (as Jeff noted) and imho that
needs to be considered (and tested for as far as practicable).

Anyhow, all the best and good luck with it, rick
ps: I suspect you guys are like me, in that you do not have easy access
      to other clients, like the resurrected CITI Windows client or the
      VMware one. (I think there is also a NFSv4.1 client in Windows
      server edition?)
Sagi Grimberg July 8, 2024, 6:23 a.m. UTC | #8
On 08/07/2024 0:49, Rick Macklem wrote:
> On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>>
>>
>> On 07/07/2024 22:05, Rick Macklem wrote:
>>> On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
>>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>> ---
>>>>> 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?

To me, it seems simple enough to make it set the want flags, but I don't
have any context to why it wasn't done up until now...

Perhaps Trond and/or Anna can chime in.
Chuck Lever July 8, 2024, 2:18 p.m. UTC | #9
> On Jul 7, 2024, at 7:06 AM, Jeff Layton <jlayton@kernel.org> 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@oracle.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>> 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.


> 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?

My understanding is that a write delegation is no more
than a promise by the server to tell the client if another
client wants access to the file. So granting a write
delegation on a read-only or write-only OPEN should be
fine to do (at the discretion of the server, of course).

The issue about the ACE is moot for NFSD right now because
NFSD returns an empty ACE. That should require the client to
continue to send ACCESS operations to the server as needed.


--
Chuck Lever
Dai Ngo July 8, 2024, 5:39 p.m. UTC | #10
On 7/8/24 7:18 AM, Chuck Lever III wrote:
>
>> On Jul 7, 2024, at 7:06 AM, Jeff Layton <jlayton@kernel.org> 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@oracle.com>
>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>> 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.

The cause of the failure is because the client does read-modify-write
using the write delegation stateid. 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. 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.

-Dai

>
>
>> 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?
> My understanding is that a write delegation is no more
> than a promise by the server to tell the client if another
> client wants access to the file. So granting a write
> delegation on a read-only or write-only OPEN should be
> fine to do (at the discretion of the server, of course).
>
> The issue about the ACE is moot for NFSD right now because
> NFSD returns an empty ACE. That should require the client to
> continue to send ACCESS operations to the server as needed.
>
>
> --
> Chuck Lever
>
>
Sagi Grimberg July 8, 2024, 6:56 p.m. UTC | #11
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@kernel.org> 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@oracle.com>
>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>> ---
>>>> 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?

>
> 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?

> 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.
Dan Shelton July 8, 2024, 9:01 p.m. UTC | #12
On Mon, 8 Jul 2024 at 16:19, Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jul 7, 2024, at 7:06 AM, Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> >> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> >> ---
> >> 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.

So could you please test this change against other clients, such as:
- Windows ms-nfs41-client (https://github.com/kofemann/ms-nfs41-client/)
- FreeBSD NFS4.1 client
- Solaris 11 NFS4 client

Dan
Dai Ngo July 8, 2024, 9:41 p.m. UTC | #13
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@kernel.org> 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@oracle.com>
>>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>> ---
>>>>> 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@oracle.com/

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@kernel.org

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.
Sagi Grimberg July 9, 2024, 7:18 a.m. UTC | #14
On 09/07/2024 0:41, Dai Ngo wrote:
>
> 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@kernel.org> 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@oracle.com>
>>>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>>> ---
>>>>>> 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@oracle.com/ 
>
>
> 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@kernel.org 
>
>
> I did not investigate this pynfs problem since we dropped this support.

I see, thanks for the info. I initially thought that the client has an 
issue. But now
I see that you referred to nfsd that is unable to process a READ with a 
writedeleg stid
(which is allowed).

Is there any appetite to address this? Or are we fine with not handing 
out delegations
in these case? The patch I sent served its goal, which was to understand 
if there is
some reasoning behind the lack of it.
Chuck Lever July 9, 2024, 1:55 p.m. UTC | #15
> On Jul 9, 2024, at 3:18 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
> 
> 
> 
> On 09/07/2024 0:41, Dai Ngo wrote:
>> 
>> 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@kernel.org> 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@oracle.com>
>>>>>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>>>>>>> ---
>>>>>>> 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@oracle.com/ 
>> 
>> 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@kernel.org 
>> 
>> I did not investigate this pynfs problem since we dropped this support.
> 
> I see, thanks for the info. I initially thought that the client has an issue. But now
> I see that you referred to nfsd that is unable to process a READ with a writedeleg stid
> (which is allowed).
> 
> Is there any appetite to address this?

Yes, as an NFSD co-maintainer, I would like to see the
READ stateid issue addressed. We just got distracted
by other things in the meantime.


> Or are we fine with not handing out delegations
> in these case?

Well, we're fine in as much as the current situation does
allow NFSD to hand out /some/ write delegations. We know
the open r/w case is working correctly, and can now move
on to the more obscure cases.

(Like, I would like NFSD to populate the delegation ACE
too, at some point).


--
Chuck Lever
Sagi Grimberg July 10, 2024, 7:11 a.m. UTC | #16
> Yes, as an NFSD co-maintainer, I would like to see the
> READ stateid issue addressed. We just got distracted
> by other things in the meantime.

OK, so reading the correspondence from the last time, it seems that
the breakage was the usage of anon stateid on a read. The spec says that
the client should use a stateid associated with a open/deleg to avoid
self-recall, but allowed to use the anon stateid.

I think that Dai's patch is a good starting point but needs to add 
handling of
the anon stateid case. The server should check if the client holds a 
delegation,
if so simply allow, if another client holds a deleg, it should recall?

Thoughts?
Chuck Lever July 10, 2024, 2:45 p.m. UTC | #17
> On Jul 10, 2024, at 3:11 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
> 
> 
>> Yes, as an NFSD co-maintainer, I would like to see the
>> READ stateid issue addressed. We just got distracted
>> by other things in the meantime.
> 
> OK, so reading the correspondence from the last time, it seems that
> the breakage was the usage of anon stateid on a read. The spec says that
> the client should use a stateid associated with a open/deleg to avoid
> self-recall, but allowed to use the anon stateid.
> 
> I think that Dai's patch is a good starting point but needs to add handling of
> the anon stateid case. The server should check if the client holds a delegation,
> if so simply allow, if another client holds a deleg, it should recall?

For an anon stateid, NFSD might just always recall if
there is a delegation on that file. The use of anon is
kind of a legacy behavior, IIUC, so no need to go to a
lot of trouble to make it optimal.

(This is my starting position; I'm open to be convinced
NFSD should take more pain for this use case).


--
Chuck Lever
Sagi Grimberg July 17, 2024, 10:56 p.m. UTC | #18
On 10/07/2024 17:45, Chuck Lever III wrote:
>
>> On Jul 10, 2024, at 3:11 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
>>
>>
>>> Yes, as an NFSD co-maintainer, I would like to see the
>>> READ stateid issue addressed. We just got distracted
>>> by other things in the meantime.
>> OK, so reading the correspondence from the last time, it seems that
>> the breakage was the usage of anon stateid on a read. The spec says that
>> the client should use a stateid associated with a open/deleg to avoid
>> self-recall, but allowed to use the anon stateid.
>>
>> I think that Dai's patch is a good starting point but needs to add handling of
>> the anon stateid case. The server should check if the client holds a delegation,
>> if so simply allow, if another client holds a deleg, it should recall?
> For an anon stateid, NFSD might just always recall if
> there is a delegation on that file. The use of anon is
> kind of a legacy behavior, IIUC, so no need to go to a
> lot of trouble to make it optimal.
>
> (This is my starting position; I'm open to be convinced
> NFSD should take more pain for this use case).

Hey Chuck, didn't forget about this.

I'll look into this when I find some time (which I lack these days).
Martin Wege Sept. 25, 2024, 2:44 p.m. UTC | #19
On Mon, Jul 8, 2024 at 12:05 AM Rick Macklem <rick.macklem@gmail.com> wrote:
>
> On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
> >
> >
> >
> > On 07/07/2024 22:05, Rick Macklem wrote:
> > > On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> > >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > >>> ---
> > >>> 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 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.
> I guess the point I was trying to make was that, if a client explicitly
> asks for a write delegation, then you issue it and the client screws up,
> "the client got what it asked for".
>
> However, if the client has not previously seen a write delegation for this
> case and is not asking for it, then if it is buggy in this respect, a
> server upgrade
> results in a client failure (fun and bothersome to track down, even if
> it really a
> client bug). ie. There is a risk in this change (as Jeff noted) and imho that
> needs to be considered (and tested for as far as practicable).
>
> Anyhow, all the best and good luck with it, rick
> ps: I suspect you guys are like me, in that you do not have easy access
>       to other clients, like the resurrected CITI Windows client or the
>       VMware one. (I think there is also a NFSv4.1 client in Windows
>       server edition?)

Windows Server only has a NFS4.1 server, but no client.
NFSv4.1 clients for Windows are available from OpenText and
https://github.com/kofemann/ms-nfs41-client

Thanks,
Martin
Rick Macklem Sept. 25, 2024, 9:33 p.m. UTC | #20
On Wed, Sep 25, 2024 at 8:03 AM Martin Wege <martin.l.wege@gmail.com> wrote:
>
> CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca.
>
>
> On Mon, Jul 8, 2024 at 12:05 AM Rick Macklem <rick.macklem@gmail.com> wrote:
> >
> > On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
> > >
> > >
> > >
> > > On 07/07/2024 22:05, Rick Macklem wrote:
> > > > On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> > > >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > > >>> ---
> > > >>> 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 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.
> > I guess the point I was trying to make was that, if a client explicitly
> > asks for a write delegation, then you issue it and the client screws up,
> > "the client got what it asked for".
> >
> > However, if the client has not previously seen a write delegation for this
> > case and is not asking for it, then if it is buggy in this respect, a
> > server upgrade
> > results in a client failure (fun and bothersome to track down, even if
> > it really a
> > client bug). ie. There is a risk in this change (as Jeff noted) and imho that
> > needs to be considered (and tested for as far as practicable).
> >
> > Anyhow, all the best and good luck with it, rick
> > ps: I suspect you guys are like me, in that you do not have easy access
> >       to other clients, like the resurrected CITI Windows client or the
> >       VMware one. (I think there is also a NFSv4.1 client in Windows
> >       server edition?)
>
> Windows Server only has a NFS4.1 server, but no client.
> NFSv4.1 clients for Windows are available from OpenText and
> https://github.com/kofemann/ms-nfs41-client
I'll believe you. (I have no access to Windows servers.)
What was weird is that I had someone with a problem a few
years ago, who claimed they were using a NFSv4.1 client in Windows server.
I even "corrected" them, but they claimed they did have it and
indicated that they had gotten it from Microsoft? Maybe they had the
OpenText one and he didn't realize it. I hadn't heard of the OpenText
one (used to be Hummingbird long ago).

Anyhow, thanks for the update, rick

>
> Thanks,
> Martin
>
Martin Wege Oct. 5, 2024, 12:37 p.m. UTC | #21
On Wed, Sep 25, 2024 at 11:33 PM Rick Macklem <rick.macklem@gmail.com> wrote:
>
> On Wed, Sep 25, 2024 at 8:03 AM Martin Wege <martin.l.wege@gmail.com> wrote:
> >
> > CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca.
> >
> >
> > On Mon, Jul 8, 2024 at 12:05 AM Rick Macklem <rick.macklem@gmail.com> wrote:
> > >
> > > On Sun, Jul 7, 2024 at 1:21 PM Sagi Grimberg <sagi@grimberg.me> wrote:
> > > >
> > > >
> > > >
> > > > On 07/07/2024 22:05, Rick Macklem wrote:
> > > > > On Sun, Jul 7, 2024 at 4:07 AM Jeff Layton <jlayton@kernel.org> 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@oracle.com>
> > > > >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > > > >>> ---
> > > > >>> 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 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.
> > > I guess the point I was trying to make was that, if a client explicitly
> > > asks for a write delegation, then you issue it and the client screws up,
> > > "the client got what it asked for".
> > >
> > > However, if the client has not previously seen a write delegation for this
> > > case and is not asking for it, then if it is buggy in this respect, a
> > > server upgrade
> > > results in a client failure (fun and bothersome to track down, even if
> > > it really a
> > > client bug). ie. There is a risk in this change (as Jeff noted) and imho that
> > > needs to be considered (and tested for as far as practicable).
> > >
> > > Anyhow, all the best and good luck with it, rick
> > > ps: I suspect you guys are like me, in that you do not have easy access
> > >       to other clients, like the resurrected CITI Windows client or the
> > >       VMware one. (I think there is also a NFSv4.1 client in Windows
> > >       server edition?)
> >
> > Windows Server only has a NFS4.1 server, but no client.
> > NFSv4.1 clients for Windows are available from OpenText and
> > https://github.com/kofemann/ms-nfs41-client
> I'll believe you. (I have no access to Windows servers.)
> What was weird is that I had someone with a problem a few
> years ago, who claimed they were using a NFSv4.1 client in Windows server.

The Windows NFS41 client project was done by CITI at the behest of
Microsoft&SUN Microsystems. Then Oracle took over SUN, stopped
funding, and instead tried to milk money out of the stakeholders for
"support", and the project was quickly abandoned.

https://github.com/kofemann/ms-nfs41-client is based on CITIs original
work where they left off, with major improvements to get it useable
for production systems

> I even "corrected" them, but they claimed they did have it and
> indicated that they had gotten it from Microsoft? Maybe they had the
> OpenText one and he didn't realize it. I hadn't heard of the OpenText
> one (used to be Hummingbird long ago).

Nope, the binaries for the MS NFS41 client popped up around 2008-2010.
Read https://www.heise.de/news/Quelloffener-Windows-Treiber-fuer-NFSv4-1-fertiggestellt-1103643.html
OpenText is NFS4.0, CITI is NFS4.1, the resurrected CITI is NFS4.2

Thanks,
Martin
diff mbox series

Patch

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;
 	}
 
 	/*