Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
CC:     Neil Brown <neilb@suse.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v4 5/5] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Topic: [PATCH v4 5/5] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Index: AQHY7R03KN21kqk5ukm1fCsNXCWZkK4o/W6A
Date:   Mon, 31 Oct 2022 21:00:39 +0000
Message-ID: <3A83C32A-B851-49A5-8C6D-7CFB67B97136@oracle.com>
References: <20221031113742.26480-1-jlayton@kernel.org>
 <20221031113742.26480-6-jlayton@kernel.org>
In-Reply-To: <20221031113742.26480-6-jlayton@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?mBvQ1MfwFiVwbjINV6mKoNaNebBq4pRH3odIewj6uKyq0oE6QckV6UbLFGq8?=
 =?us-ascii?Q?wteAOPSy2qQLKkDha1wyh8C6N1Bmlnbgj7eqD+W4tUHQQYTAmv7rdd2gyf87?=
 =?us-ascii?Q?tjJoInsNU00KIM09RujvHxdPEaI6qeWwovY4VeIrEJ8fx2vJqI6nmTfGkncJ?=
 =?us-ascii?Q?Wpbq0lwJc9ueJURXnnkn4GqBJhXpffvJygbd7PHHb7KqQ5hAob9ZQXXeZpMr?=
 =?us-ascii?Q?7TbhKu5Mlm2Vk1+jTTrYMx9JklECMc7f5tRLJ7VUYTgmGCAo+kuMmIZcL6Qd?=
 =?us-ascii?Q?K6VdiIqq5bzB6NK+SEbQQDcSt1POv3f/1VSYvoPTSOIcgkNUOnRT1kzxnmgu?=
 =?us-ascii?Q?tbrDr7HCl//PyHvP2Mn4jGGNIXgagXvKFOYr9/wA3EbfWnk/HFOuoKienZP7?=
 =?us-ascii?Q?Yn9js3UK5wS/Rq9jiYz6+TjhStWV/xcqF+t6EjKAbomQSpQg1LAv+2i55Xbu?=
 =?us-ascii?Q?wkL00UnpiEFXXfYca6cYI58zzfzVN9vSlLy3GKsUOWeTw5rfMPrt+kkpG5rY?=
 =?us-ascii?Q?uFK+rLURZwQLDQRMiLniy9otIR7nmSxUkn6MntsuSg+TVytBJ61JjFuc96UD?=
 =?us-ascii?Q?VMQrcvM25sd69vzo7uh8rQkqacj0FC3O16BdxrY4oTD2sCdHaEy79xmkNQsi?=
 =?us-ascii?Q?BdL4vsUj3TvBaKyrKW6175dlcOkzhiG1PCF8Pa4X5ikEjnvJZ4/aPlEGQvTu?=
 =?us-ascii?Q?K7oewMzk0ZVanNRYGiq/zFODCVwNeb4YY0JESacp4iRwY/4DuyjgM1qtQyTQ?=
 =?us-ascii?Q?zpzk90g9VRq7fHSHMF8AXxvnRkKDWP3YjITWp5ZxizVFoV14CONyKI6mL1t7?=
 =?us-ascii?Q?3PsncjqKECJVy/VqZSvCn+/7DL417gt1u67VQpNtyI+pUvu7OIItbNMgXLz4?=
 =?us-ascii?Q?FsRAbJzZEQKSXwApoBDBTSZhbmGhmcrPbBBwGle4gey8HmnxIqGsOMPdQsjY?=
 =?us-ascii?Q?vFYk2e8rBWvTxi21r3jhCpiwVJWuLhErZxBxQCe/SeoxthAVV8hF28EcsikR?=
 =?us-ascii?Q?fYeW97I1kYQ87zdR2QsVeXBCuBTiubwkewxvFFY2rPooc2RCMq2zOZYewAzG?=
 =?us-ascii?Q?DnADuStzwxu+ytXImUIvpj6PWQ6AqwHBHew+syu+M4AZZ2R1M+TzIoeaadZW?=
 =?us-ascii?Q?u3cHszjsAgcDipKK2B9VpKzihj1YlZcZX2ZKsHlwVvLSp40fHWDBIPCnsj/s?=
 =?us-ascii?Q?xKOm4yc4jx4rQsMKCqJMe2u3+ELkaPU46aTsuduNu/VDftzTJztlpr3ADGlI?=
 =?us-ascii?Q?xeqAgmKGXUvgJQcp7+NWf27C1jk+FT6wML/gVxr4c2jeizNKRkANsHP2o4mG?=
 =?us-ascii?Q?0uThdEx+IWclG9nUxj2cVuaOmL5oz8XqRfbnEViFOOlYgQaBqN0wRpGleVFV?=
 =?us-ascii?Q?YwsdyHgSv6keMkFRhjG0pfgTrv0DUbPn36jY+q0jQSVHMEUadsh4qq2yF9BO?=
 =?us-ascii?Q?d/+dN+mQOjGzQExg4KMMd7Sa4Chqftr7T6avgG5NRcYR7H9Po0pRasPdp8xY?=
 =?us-ascii?Q?Yv51tTiknftoQixVnr2s0kxVsQPKkCF3j63tSyTvEabonAPgqXClkuVUjGNC?=
 =?us-ascii?Q?s6WMA1Lknx4m9oeky+mazCbbQVTNBU4DK0utZgGbtQ2Ghq2mzqAuD4ELIsgO?=
 =?us-ascii?Q?MQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5ACA2CC4F9C7A348AE41B41E7488B3FE@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0f6f383d-ff5d-44e3-8534-08dabb82f736
X-MS-Exchange-CrossTenant-originalarrivaltime: 31 Oct 2022 21:00:39.3410
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: c7r6K84FuePBf4uWI/TYEE54mLzu0Yt/uTlC1jpQ9pHhG3I8jae+tVc6EPSvc+2mKFwOwazNy/XgTFx2nysfgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6088
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-31_21,2022-10-31_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 spamscore=0 bulkscore=0
 suspectscore=0 phishscore=0 malwarescore=0 mlxscore=0 mlxlogscore=791
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2210310130
X-Proofpoint-ORIG-GUID: B05VklVElL1YugCjkRwtvlOF6SW58oA8
X-Proofpoint-GUID: B05VklVElL1YugCjkRwtvlOF6SW58oA8
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Oct 31, 2022, at 7:37 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> When a GC entry gets added to the LRU, kick off SYNC_NONE writeback
> so that we can be ready to close it when the time comes. This should
> help minimize delays when freeing objects reaped from the LRU.

Tested against a btrfs export.

So, the current code does indeed do a synchronous fsync when
garbage collecting files (via nfsd_file_delayed_close()).
That indicates that it's at least safe to do, and 3/5 isn't
changing the safety of the filecache by moving the vfs_fsync()
call into nfsd_file_free(). These calls take between 10 and
20 milliseconds each.

But I see the filemap_flush() call added here taking dozens of
milliseconds on my system for large files. This is done before
the WRITE reply is sent to the client, so it adds significant
latency to large UNSTABLE WRITEs. In the current code, the
vfs_fsync() in nfsd_file_put() does not seem to fire except in
very rare circumstances, so it doesn't seem to have much if any
impact.

My scalability concerns therefore are with code that pre-dates
this patch series. I can deal with that later in a separate
series. Do we need to keep this one?


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 47cdc6129a7b..c43b6cff03e2 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -325,6 +325,20 @@ nfsd_file_fsync(struct nfsd_file *nf)
> 		nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> }
>=20
> +static void
> +nfsd_file_flush(struct nfsd_file *nf)
> +{
> +	struct file *file =3D nf->nf_file;
> +	struct address_space *mapping;
> +
> +	if (!file || !(file->f_mode & FMODE_WRITE))
> +		return;
> +
> +	mapping =3D file->f_mapping;
> +	if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
> +		filemap_flush(mapping);
> +}
> +
> static int
> nfsd_file_check_write_error(struct nfsd_file *nf)
> {
> @@ -484,9 +498,14 @@ nfsd_file_put(struct nfsd_file *nf)
>=20
> 		/* Try to add it to the LRU.  If that fails, decrement. */
> 		if (nfsd_file_lru_add(nf)) {
> -			/* If it's still hashed, we're done */
> -			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags))
> +			/*
> +			 * If it's still hashed, we can just return now,
> +			 * after kicking off SYNC_NONE writeback.
> +			 */
> +			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> +				nfsd_file_flush(nf);
> 				return;
> +			}
>=20
> 			/*
> 			 * We're racing with unhashing, so try to remove it from
> --=20
> 2.38.1
>=20

--
Chuck Lever