Re: [PATCH v5 3/3] locks: allow support for write delegation

[dai.ngo@oracle.com]

On 5/24/23 12:03 PM, Jeff Layton wrote:
> On Wed, 2023-05-24 at 11:05 -0700, dai.ngo@oracle.com wrote:
>> On 5/24/23 10:41 AM, Chuck Lever III wrote:
>>>> On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>
>>>> On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote:
>>>>>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote:
>>>>>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock
>>>>>>> to be used for write delegation.
>>>>>>>
>>>>>>> First consumer is NFSD.
>>>>>>>
>>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>>>> ---
>>>>>>> fs/locks.c | 7 -------
>>>>>>> 1 file changed, 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>>> index df8b26a42524..08fb0b4fd4f8 100644
>>>>>>> --- a/fs/locks.c
>>>>>>> +++ b/fs/locks.c
>>>>>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>>>>>>> if (is_deleg && !inode_trylock(inode))
>>>>>>> return -EAGAIN;
>>>>>>>
>>>>>>> - if (is_deleg && arg == F_WRLCK) {
>>>>>>> - /* Write delegations are not currently supported: */
>>>>>>> - inode_unlock(inode);
>>>>>>> - WARN_ON_ONCE(1);
>>>>>>> - return -EINVAL;
>>>>>>> - }
>>>>>>> -
>>>>>>> percpu_down_read(&file_rwsem);
>>>>>>> spin_lock(&ctx->flc_lock);
>>>>>>> time_out_leases(inode, &dispose);
>>>>>> I'd probably move this back to the first patch in the series.
>>>>>>
>>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>>> I asked him to move it to the end. Is it safe to take out this
>>>>> check before write delegation is actually implemented?
>>>>>
>>>> I think so, but it don't think it doesn't make much difference either
>>>> way. The only real downside of putting it at the end is that you might
>>>> have to contend with a WARN_ON_ONCE if you're bisecting.
>>> My main concern is in fact preventing problems during bisection.
>>> I can apply 3/3 and then 1/3, if you're good with that.
>> I'm good with that. You can apply 3/3 then 1/3 and drop 2/3 so I
>> don't have to send out v6.
>>
> I'm fine with that too, particularly if other vendors don't recall on a
> getattr currently.
>
> I wonder though, if we need some clarification in the spec on
> CB_GETATTR?
>
>      https://www.rfc-editor.org/rfc/rfc8881.html#section-10.4.3
>
> In that section, there is a big distinction about the client being able
> to tell that the data was modified from the point where the delegation
> was handed out.
>
> There is always a point in time where a client has buffered writes that
> haven't been flushed to the server yet, but that's true when it doesn't
> have a delegation too. Mostly the client tries to start some writeback
> fairly quickly so any lag how the in the change attr/size update is
> usually short lived.
>
> I don't think the Linux client materially changes its writeback behavior
> based on a write delegation, so I guess (as Olga pointed out) the main
> benefit from a write delegation is being able to do delegated opens for
> write. A getattr's results won't be changed by extra opens or closes, so
> yeah...I guess the utility of CB_GETATTR is really limited.
>
> I guess it _might_ be useful in the case where the server has handed out
> a write delegation, but hasn't gotten any writes. That would at least
> tell the client that something has changed, even if the deleg holder
> hasn't gotten around to writing anything back yet. The problem is that
> it's common for applications to open O_RDWR and only do reads.
>
> Maybe we ought to take this discussion to the IETF list? It seems like
> the spec mandates that you must recall the delegation if you don't
> implement CB_GETATTR, but I don't see much in way of harm in ignoring
> that.

Yes, I think we should, for clarification.
Jeff, would you mind to drive this discussion on IETF since you can
present the issue much clearer than I would.

Thanks,
-Dai