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;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/6] SUNRPC: rename and refactor svc_get_next_xprt()
In-reply-to: <ZMv5S7k4iCQgYXZ4@tissot.1015granger.net>
References: <20230802073443.17965-1-neilb@suse.de>,
 <20230802073443.17965-3-neilb@suse.de>,
 <ZMv5S7k4iCQgYXZ4@tissot.1015granger.net>
Date:   Fri, 04 Aug 2023 07:12:03 +1000
Message-id: <169109712368.32308.6546907686830224026@noble.neil.brown.name>
Precedence: bulk

On Fri, 04 Aug 2023, Chuck Lever wrote:
> On Wed, Aug 02, 2023 at 05:34:39PM +1000, NeilBrown wrote:
> > svc_get_next_xprt() does a lot more than just get an xprt.  It also
> > decides if it needs to sleep, depending not only on the availability of
> > xprts but also on the need to exit or handle external work.
> > 
> > So rename it to svc_rqst_wait_for_work() and only do the testing and
> > waiting.  Move all the waiting-related code out of svc_recv() into the
> > new svc_rqst_wait_for_work().
> > 
> > Move the dequeueing code out of svc_get_next_xprt() into svc_recv().
> > 
> > Previously svc_xprt_dequeue() would be called twice, once before waiting
> > and possibly once after.  Now instead rqst_should_sleep() is called
> > twice.  Once to decide if waiting is needed, and once to check against
> > after setting the task state do see if we might have missed a wakeup.
> > 
> > signed-off-by: NeilBrown <neilb@suse.de>
> 
> I've tested and applied this one and the previous one to the thread
> scheduling branch, with a few minor fix-ups. Apologies for how long
> this took, I'm wrestling with a SATA/block bug on the v6.6 test
> system that is being very sassy and hard to nail down.

I'm happy that we are making progress and the series is getting improved
along the way.  Good lock with the block bug.

> 
> I need to dive into the backchannel patch next. I'm trying to think
> of how I want to test that one.
> 

I think lock-grant call backs should be easiest to trigger.
However I just tried mounting the filesystem twice with nosharecache,
and the locking that same file on both mounts.  I expected one to block
and hoped I would see the callback when the first lock was dropped.
However the second lock didn't block! That's a bug.
I haven't dug very deep yet, but I think the client is getting a
delegation for the first open (O_RDWR) so the server doesn't see the
lock.
Then when the lock arrives on the second mount, there is no conflicting
lock and the write delegation maybe isn't treated as a conflict?

I'll try to look more early next week.

NeilBrown