Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 17 Nov 2023 05:00:09 -0800
From:   Christoph Hellwig <hch@infradead.org>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Chuck Lever III <chuck.lever@oracle.com>,
        Benjamin Coddington <bcodding@redhat.com>,
        Christoph Hellwig <hch@infradead.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: Blocklayoutdriver deadlock with knfsd
Message-ID: <ZVdj2U+kJ7DlI22g@infradead.org>
References: <1CC82EC5-6120-4EE4-A7F0-019CF7BC762C@redhat.com>
 <787DAC8F-5294-4876-9725-096D639B3D9C@oracle.com>
 <465037c9585f910c2192bf896139e7bf01d587d4.camel@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <465037c9585f910c2192bf896139e7bf01d587d4.camel@kernel.org>
Precedence: bulk

On Thu, Nov 16, 2023 at 03:32:34PM -0500, Jeff Layton wrote:
> One thing that might help would be make the nfsd threadpool more
> dynamic. If it could just spin up another thread, we'd be OK.
> 
> Maybe the server could keep an emergency thread around that is just for
> processing LAYOUTRETURN/DELEGRETURN (maybe also CLOSE, etc.) calls?
> Sometimes these ops are mixed into compounds with other sorts of calls
> though, so we'd need to deal with that somehow.

I think having an extra thread just for LAYOUTRETURN/DELEGRETURN is
fundamentally the right thing to do, as they need to be processed
to allow everyone else to progress.

> > If nfsd threads are waiting indefinitely, that's a potential DoS
> > vector. Ideally the thread should preserve the waiting request
> > somehow (or return NFS4ERR_DELAY, maybe?). At some later point
> > when the lease conflict is resolved, the requests can be reprocessed.
> > 
> > That's my naive 800,000 ft view.
> > 

That is probably a good idea on top of the above.

> So you can always get stuck in that inner break_layout call. We could
> try to use IOCB_NOWAIT for I/Os coming from nfsd, which would prevent
> that, but that seems like it could change the I/O behavior in other ways
> we don't want. It's not clear to me how that would work alongside
> IOCB_SYNC anyway.

So I definitively think using IOCB_NOWAIT from nfsd and not block
the thread for trivial I/O is a good thing.  This might even enable
not offloading I/O to threads until the IOCB_NOWAIT failed.  But any
IOCB_NOWAIT that returned -EAGAIN needs to eventually fall back to
doing blocking I/O as there is no progress guarantee for IOCB_NOWAIT,
and some I/O simply is entirely impossible with IOCB_NOWAIT.

> It's not immediately clear to me how we'd do this with the existing
> IOCB_* flags, so we might need a new one (IOCB_NFSD?). Then, we could
> just make sure that break_layout call is always nonblocking if that flag
> is set.

I'd name it about the behavior that it controls and not the callers,
but otherwise this too seems like a good idea.