> On Jul 9, 2024, at 3:18 AM, Sagi Grimberg <sagi@xxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> On Sun, 2024-07-07 at 01:42 +0300, Sagi Grimberg wrote:
>>>>>>> Many applications open files with O_WRONLY, fully intending to write
>>>>>>> into the opened file. There is no reason why these applications should
>>>>>>> not enjoy a write delegation handed to them.
>>>>>>>
>>>>>>> Cc: Dai Ngo <dai.ngo@xxxxxxxxxx>
>>>>>>> Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>> Note: I couldn't find any reason to why the initial implementation chose
>>>>>>> to offer write delegations only to NFS4_SHARE_ACCESS_BOTH, but it seemed
>>>>>>> like an oversight to me. So I figured why not just send it out and see who
>>>>>>> objects...
>>>>>>>
>>>>>>> fs/nfsd/nfs4state.c | 10 +++++-----
>>>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>>> index a20c2c9d7d45..69d576b19eb6 100644
>>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>>> @@ -5784,15 +5784,15 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>>>>> * "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
>>>>>>> * on its own, all opens."
>>>>>>> *
>>>>>>> - * Furthermore the client can use a write delegation for most READ
>>>>>>> - * operations as well, so we require a O_RDWR file here.
>>>>>>> - *
>>>>>>> - * Offer a write delegation in the case of a BOTH open, and ensure
>>>>>>> - * we get the O_RDWR descriptor.
>>>>>>> + * Offer a write delegation in the case of a BOTH open (ensure
>>>>>>> + * a O_RDWR descriptor) Or WRONLY open (with a O_WRONLY descriptor).
>>>>>>> */
>>>>>>> if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
>>>>>>> nf = find_rw_file(fp);
>>>>>>> dl_type = NFS4_OPEN_DELEGATE_WRITE;
>>>>>>> + } else if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>>>>> + nf = find_writeable_file(fp);
>>>>>>> + dl_type = NFS4_OPEN_DELEGATE_WRITE;
>>>>>>> }
>>>>>>>
>>>>>>> /*
>>>>> Thanks Sagi, I'm glad to see this posting!
>>>>>
>>>>>
>>>>>> I *think* the main reason we limited this before is because a write
>>>>>> delegation is really a read/write delegation. There is no such thing as
>>>>>> a write-only delegation.
>>>>> I recall (quite dimly) that Dai found some bad behavior
>>>>> in a subtle corner case, so we decided to leave this on
>>>>> the table as a possible future enhancement. Adding Dai.
>>>>
>>>> If I remember correctly, granting write delegation on OPEN with
>>>> NFS4_SHARE_ACCESS_WRITE without additional changes causes the git
>>>> test to fail.
>>>
>>> That's good to know.
>>> Have you reported this over the list before?
>>
>> I submitted a patch to allow the client to use write delegation, granted
>> on OPEN with NFS4_SHARE_ACCESS_WRITE, to read:
>>
>> https://lore.kernel.org/linux-nfs/1688089960-24568-3-git-send-email-dai.ngo@xxxxxxxxxx/
>>
>> This patch fixed the problem with the git test. However Jeff reported another
>> problem might be related to this patch:
>>
>> https://lore.kernel.org/linux-nfs/6756f84c4ff92845298ce4d7e3c4f0fb20671d3d.camel@xxxxxxxxxx
>>
>> I did not investigate this pynfs problem since we dropped this support.
>
> 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
