Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <76dbd9cebe142067b9322d66c23f9b77f8075cf0.camel@redhat.com>
Subject: Re: [PATCH RFC v12 1/3] fs/lock: add new callback,
 lm_lock_conflict, to lock_manager_operations
From:   Jeff Layton <jlayton@redhat.com>
To:     Chuck Lever III <chuck.lever@oracle.com>,
        Bruce Fields <bfields@fieldses.org>
Cc:     Dai Ngo <dai.ngo@oracle.com>, Al Viro <viro@zeniv.linux.org.uk>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Date:   Thu, 10 Feb 2022 12:32:02 -0500
In-Reply-To: <2AEF8E7D-2F4E-4D88-8B71-48195C6E45ED@oracle.com>
References: <1644468729-30383-1-git-send-email-dai.ngo@oracle.com>
         <1644468729-30383-2-git-send-email-dai.ngo@oracle.com>
         <20220210142826.GD21434@fieldses.org>
         <2AEF8E7D-2F4E-4D88-8B71-48195C6E45ED@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
User-Agent: Evolution 3.42.3 (3.42.3-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2022-02-10 at 16:50 +0000, Chuck Lever III wrote:
> 
> > On Feb 10, 2022, at 9:28 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index bbf812ce89a8..726d0005e32f 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1068,6 +1068,14 @@ struct lock_manager_operations {
> > > 	int (*lm_change)(struct file_lock *, int, struct list_head *);
> > > 	void (*lm_setup)(struct file_lock *, void **);
> > > 	bool (*lm_breaker_owns_lease)(struct file_lock *);
> > > +	/*
> > > +	 * This callback function is called after a lock conflict is
> > > +	 * detected. This allows the lock manager of the lock that
> > > +	 * causes the conflict to see if the conflict can be resolved
> > > +	 * somehow. If it can then this callback returns false; the
> > > +	 * conflict was resolved, else returns true.
> > > +	 */
> > > +	bool (*lm_lock_conflict)(struct file_lock *cfl);
> > > };
> > 
> > I don't love that name.  The function isn't checking for a lock
> > conflict--it'd have to know *what* the lock is conflicting with.  It's
> > being told whether the lock is still valid.
> > 
> > I'd prefer lm_lock_expired(), with the opposite return values.
> 
> Or even lm_lock_is_expired(). I agree that the sense of the
> return values should be reversed.
> 
> 
> The block comment does not belong in struct lock_manager_operations,
> IMO.
> 
> Jeff's previous review comment was:
> 
> > > @@ -1059,6 +1062,9 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
> > > 		list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
> > > 			if (!posix_locks_conflict(request, fl))
> > > 				continue;
> > > +			if (fl->fl_lmops && fl->fl_lmops->lm_lock_conflict &&
> > > +				!fl->fl_lmops->lm_lock_conflict(fl))
> > > +				continue;
> > 
> > The naming of this op is a little misleading. We already know that there
> > is a lock confict in this case. The question is whether it's resolvable
> > by expiring a tardy client. That said, I don't have a better name to
> > suggest at the moment.
> > 
> > A comment about what this function actually tells us would be nice here.
> 
> 
> I agree that a comment that spells out the API contract would be
> useful. But it doesn't belong in the middle of struct
> lock_manager_operations, IMO.
> 
> I usually put such information in the block comment that precedes
> the individual functions (nfsd4_fl_lock_conflict in this case).
> 
> Even so, the patch description has this information already.
> Jeff, I think the patch description is adequate for this
> purpose -- more information appears later in 3/3. What do you
> think?
> 

I'd be fine with that.
-- 
Jeff Layton <jlayton@redhat.com>