Received-SPF: pass (google.com: domain of linux-nfs+bounces-3796-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Jan Kara" <jack@suse.cz>
Cc: "Trond Myklebust" <trond.myklebust@hammerspace.com>,
 linux-nfs@vger.kernel.org, "Jan Kara" <jack@suse.cz>
Subject: Re: [PATCH 3/3] nfs: Block on write congestion
In-reply-to: <20240613082821.849-3-jack@suse.cz>
References:
 <20240612153022.25454-1-jack@suse.cz>, <20240613082821.849-3-jack@suse.cz>
Date: Fri, 14 Jun 2024 08:56:10 +1000
Message-id: <171831937031.14261.6690155619843129747@noble.neil.brown.name>

On Thu, 13 Jun 2024, Jan Kara wrote:
> Commit 6df25e58532b ("nfs: remove reliance on bdi congestion")
> introduced NFS-private solution for limiting number of writes
> outstanding against a particular server. Unlike previous bdi congestion
> this algorithm actually works and limits number of outstanding writeback
> pages to nfs_congestion_kb which scales with amount of client's memory
> and is capped at 256 MB. As a result some workloads such as random
> buffered writes over NFS got slower (from ~170 MB/s to ~126 MB/s). The
> fio command to reproduce is:
>=20
> fio --direct=3D0 --ioengine=3Dsync --thread --invalidate=3D1 --group_report=
ing=3D1
>   --runtime=3D300 --fallocate=3Dposix --ramp_time=3D10 --new_group --rw=3Dr=
andwrite
>   --size=3D64256m --numjobs=3D4 --bs=3D4k --fsync_on_close=3D1 --end_fsync=
=3D1
>=20
> This happens because the client sends ~256 MB worth of dirty pages to
> the server and any further background writeback request is ignored until
> the number of writeback pages gets below the threshold of 192 MB. By the
> time this happens and clients decides to trigger another round of
> writeback, the server often has no pages to write and the disk is idle.
>=20
> To fix this problem and make the client react faster to eased congestion
> of the server by blocking waiting for congestion to resolve instead of
> aborting writeback. This improves the random 4k buffered write
> throughput to 184 MB/s.
>=20
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/nfs/client.c           |  1 +
>  fs/nfs/write.c            | 12 ++++++++----
>  include/linux/nfs_fs_sb.h |  1 +
>  3 files changed, 10 insertions(+), 4 deletions(-)
>=20
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index 3b252dceebf5..8286edd6062d 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -994,6 +994,7 @@ struct nfs_server *nfs_alloc_server(void)
> =20
>  	server->change_attr_type =3D NFS4_CHANGE_TYPE_IS_UNDEFINED;
> =20
> +	init_waitqueue_head(&server->write_congestion_wait);
>  	atomic_long_set(&server->writeback, 0);
> =20
>  	ida_init(&server->openowner_id);
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index c6255d7edd3c..21a5f1e90859 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -423,8 +423,10 @@ static void nfs_folio_end_writeback(struct folio *foli=
o)
> =20
>  	folio_end_writeback(folio);
>  	if (atomic_long_dec_return(&nfss->writeback) <
> -	    NFS_CONGESTION_OFF_THRESH)
> +	    NFS_CONGESTION_OFF_THRESH) {
>  		nfss->write_congested =3D 0;
> +		wake_up_all(&nfss->write_congestion_wait);
> +	}
>  }
> =20
>  static void nfs_page_end_writeback(struct nfs_page *req)
> @@ -698,12 +700,14 @@ int nfs_writepages(struct address_space *mapping, str=
uct writeback_control *wbc)
>  	struct nfs_pageio_descriptor pgio;
>  	struct nfs_io_completion *ioc =3D NULL;
>  	unsigned int mntflags =3D NFS_SERVER(inode)->flags;
> +	struct nfs_server *nfss =3D NFS_SERVER(inode);
>  	int priority =3D 0;
>  	int err;
> =20
> -	if (wbc->sync_mode =3D=3D WB_SYNC_NONE &&
> -	    NFS_SERVER(inode)->write_congested)
> -		return 0;
> +	/* Wait with writeback until write congestion eases */
> +	if (wbc->sync_mode =3D=3D WB_SYNC_NONE && nfss->write_congested)
> +		wait_event(nfss->write_congestion_wait,
> +			   nfss->write_congested =3D=3D 0);

If there is a network problem or the server reboot, this could wait
indefinitely.  Maybe wait_event_idle() ??

Is this only called from the writeback thread that is dedicated to this
fs?  If so that should be fine.  If it can be reached from a user
processes, then wait_event_killable() might be needed.

Thanks,
NeilBrown


> =20
>  	nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGES);
> =20
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 92de074e63b9..38a128796a76 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -140,6 +140,7 @@ struct nfs_server {
>  	struct rpc_clnt *	client_acl;	/* ACL RPC client handle */
>  	struct nlm_host		*nlm_host;	/* NLM client handle */
>  	struct nfs_iostats __percpu *io_stats;	/* I/O statistics */
> +	wait_queue_head_t	write_congestion_wait;	/* wait until write congestion e=
ases */
>  	atomic_long_t		writeback;	/* number of writeback pages */
>  	unsigned int		write_congested;/* flag set when writeback gets too high */
>  	unsigned int		flags;		/* various flags */
> --=20
> 2.35.3
>=20
>=20