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@xxxxxxx>
> ---
> 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
