Re: [PATCH RFC v25 0/7] NFSD: Initial implementation of NFSv4 Courteous Server

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 v25 0/7] NFSD: Initial implementation of NFSv4 Courteous Server
Date: Mon, 23 May 2022 11:40:26 -0400	[thread overview]
Message-ID: <20220523154026.GD24163@fieldses.org> (raw)
In-Reply-To: <9b394762-660b-4742-b54a-2b385485b412@oracle.com>

On Mon, May 02, 2022 at 06:38:03PM -0700, dai.ngo@oracle.com wrote:
> 
> On 5/2/22 6:21 PM, J. Bruce Fields wrote:
> >On Mon, May 02, 2022 at 09:12:52PM -0400, J. Bruce Fields wrote:
> >>Looks good to me.
> >And the only new test failures are due to the new DELAYs on OPEN.
> >Somebody'll need to fix up pynfs.  (I'm not volunteering for now.)
> 
> I will fix it, since I broke it :-)

By the way, I have three more notes on courtesy server stuff that I
wanted to dump into email before I forget them:

1. I do still recommend fixing up those pynfs failures.  The ones I see
   are in RENEW3, LKU10, CLOSE9, CLOSE8, but there may be others.

2. In the lock case, nfsd4_lock() holds an st_mutex while calling
   vfs_lock_file(), which may end up needing to wait for the laundromat.
   As I said in review, I don't see a potential deadlock there, so I'm
   fine with the code going in as is.

   But, as a note for possible cleanup, or if this does turn into a
   problem later: vfs_lock_file could return to nfsd4_lock(), and
   nfsd4_lock() could easily drop the st_mutex, wait, and retry.

   I think the only trick part would be deciding on conventions for the
   caller to tell vfs_lock_file() that it shouldn't wait in this case
   (non-nfsd callers will still want to wait), and for vfs_lock_file()
   to indicate the caller needs to retry.  Probably something in
   fl_flags for the former, and an agreed-on error return for the
   latter?

3. One other piece of future work would be optimizing the conflicting
   lock case.  A very premature optimization at this point, but I'm just
   leaving my notes here in case someone's interested:

   The loop in posix_lock_inode() is currently O(N^2) in the number of
   expirable clients holding conflicting locks, because each time we
   encounter one, we wait and then restart.  In practice I doubt that
   matters--if you have a lot of clients to expire, the time rescanning
   the list will likely be trivial compared to the time spent waiting
   for nfsdcld to commit the expiry of each client to stable storage.

   *However*, it might be a more significant optimization if we first
   allowed more parallelism in nfsdcld.  And that might also benefit
   some other cases (e.g., lots of clients reconnecting after a crash).
   We'd need paralle nfsdcld--no idea what that would involve--and I
   think it'd also help to update the kernel<->nfsdcld protocol with a
   separate commit operation, so that nfsd could issue a bunch of client
   changes and then a single commit to wait for them all.

   That done, we could modify the loop in vfs_lock_file() so that, in
   the case where multiple clients hold conflicting locks, the loop
   marks them all for expiry in one pass, then waits just once at the
   end.

--b.