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

[Jeff Layton <jlayton@kernel.org>]

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