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;
Date:   Fri, 29 Apr 2022 15:59:30 -0400
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     dai.ngo@oracle.com
Cc:     chuck.lever@oracle.com, jlayton@redhat.com,
        viro@zeniv.linux.org.uk, linux-nfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH RFC v23 1/7] NFSD: add courteous server support for
 thread with only delegation
Message-ID: <20220429195930.GJ7107@fieldses.org>
References: <1651129595-6904-1-git-send-email-dai.ngo@oracle.com>
 <1651129595-6904-2-git-send-email-dai.ngo@oracle.com>
 <20220429145543.GD7107@fieldses.org>
 <6ce5af72-52ba-7cf0-8295-7929b9b0b4a8@oracle.com>
 <20220429195510.GH7107@fieldses.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220429195510.GH7107@fieldses.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Fri, Apr 29, 2022 at 03:55:10PM -0400, J. Bruce Fields wrote:
> On Fri, Apr 29, 2022 at 10:21:21AM -0700, dai.ngo@oracle.com wrote:
> > 
> > On 4/29/22 7:55 AM, J. Bruce Fields wrote:
> > >On Thu, Apr 28, 2022 at 12:06:29AM -0700, Dai Ngo wrote:
> > >>+static bool client_has_state_tmp(struct nfs4_client *clp)
> > >Why the "_tmp"?
> > >
> > >>+{
> > >>+	if (!list_empty(&clp->cl_delegations) &&
> > >>+			!client_has_openowners(clp) &&
> > >>+			list_empty(&clp->async_copies))
> > >I would have expected
> > >
> > >	if (!list_empty(&clp->cl_delegations) ||
> > >		client_has_openowners(clp) ||
> > >		!list_empty(&clp->async_copies))
> > 
> > In patch 1, we want to allow *only* clients with non-conflict delegation
> > to be in COURTESY state, not with opens and locks. So for that, we can not
> > use the existing client_has_state (until patch 6), so I just created
> > client_has_state_tmp for it.
> 
> Got it, so, I recommend just moving this logic into
> nfs4_anylock_blockers instead, and replacing the call to
> client_has_state_tmp() with a call to client_has_state().
> 
> The logic of nfs4_anylock_blockers() is then basically "return true if anyone
> might be waiting on this client; and if the client has some class of
> state that we don't handle yet, just assume it might have someone
> waiting on it."

And, yeah, the end result is probably the same, but this would just make
the patches easier to read.

--b.