Re: [PATCH 22/23] NFS: swap-out must always use STABLE writes.

From: Trond Myklebust <trondmy@hammerspace.com>
To: "mgorman@suse.de" <mgorman@suse.de>,
	"dhowells@redhat.com" <dhowells@redhat.com>,
	"hch@infradead.org" <hch@infradead.org>,
	"neilb@suse.de" <neilb@suse.de>,
	"anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"chuck.lever@oracle.com" <chuck.lever@oracle.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>
Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 22/23] NFS: swap-out must always use STABLE writes.
Date: Wed, 26 Jan 2022 03:45:42 +0000	[thread overview]
Message-ID: <e50bf6286a89d60ee0879e55a30b15d84e97d9a4.camel@hammerspace.com> (raw)
In-Reply-To: <164299611287.26253.13462969110743208198.stgit@noble.brown>

On Mon, 2022-01-24 at 14:48 +1100, NeilBrown wrote:
> The commit handling code is not safe against memory-pressure
> deadlocks
> when writing to swap.  In particular, nfs_commitdata_alloc() blocks
> indefinitely waiting for memory, and this can consume all available
> workqueue threads.
> 
> swap-out most likely uses STABLE writes anyway as COND_STABLE
> indicates
> that a stable write should be used if the write fits in a single
> request, and it normally does.  However if we ever swap with a small
> wsize, or gather unusually large numbers of pages for a single write,
> this might change.
> 
> For safety, make it explicit in the code that direct writes used for
> swap
> must always use FLUSH_COND_STABLE.

OK. Your explanation above has me extremely confused.

If you want to avoid commit, then you should be using FLUSH_STABLE,
since that forces the writes to be synchronous. FLUSH_COND_STABLE can
and will use unstable writes if it sees that there are more writes to
come.

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfs/direct.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 43a956d7fd62..29c007b2a17a 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -791,7 +791,7 @@ static const struct nfs_pgio_completion_ops
> nfs_direct_write_completion_ops = {
>   */
>  static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req
> *dreq,
>                                                struct iov_iter *iter,
> -                                              loff_t pos)
> +                                              loff_t pos, int
> ioflags)
>  {
>         struct nfs_pageio_descriptor desc;
>         struct inode *inode = dreq->inode;
> @@ -799,7 +799,7 @@ static ssize_t
> nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
>         size_t requested_bytes = 0;
>         size_t wsize = max_t(size_t, NFS_SERVER(inode)->wsize,
> PAGE_SIZE);
>  
> -       nfs_pageio_init_write(&desc, inode, FLUSH_COND_STABLE, false,
> +       nfs_pageio_init_write(&desc, inode, ioflags, false,
>                               &nfs_direct_write_completion_ops);
>         desc.pg_dreq = dreq;
>         get_dreq(dreq);
> @@ -905,6 +905,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb,
> struct iov_iter *iter,
>         struct nfs_direct_req *dreq;
>         struct nfs_lock_context *l_ctx;
>         loff_t pos, end;
> +       int ioflags = swap ? FLUSH_COND_STABLE : FLUSH_STABLE;

This is an unacceptable change in behaviour for the non-swap case, so
NACK.

>  
>         dfprintk(FILE, "NFS: direct write(%pD2, %zd@%Ld)\n",
>                 file, iov_iter_count(iter), (long long) iocb-
> >ki_pos);
> @@ -947,7 +948,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb,
> struct iov_iter *iter,
>         if (!swap)
>                 nfs_start_io_direct(inode);
>  
> -       requested = nfs_direct_write_schedule_iovec(dreq, iter, pos);
> +       requested = nfs_direct_write_schedule_iovec(dreq, iter, pos,
> ioflags);
>  
>         if (mapping->nrpages) {
>                 invalidate_inode_pages2_range(mapping,
> 
> 

-- 
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022

www.hammer.space