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:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Neil Brown <neilb@suse.de>
Subject: Re: [PATCH v3 4/4] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Topic: [PATCH v3 4/4] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Index: AQHY6v8cX8B2IAnCiUGFnEk+Gt8JNK4kNxkAgAALLICAAAeCgA==
Date:   Fri, 28 Oct 2022 20:57:27 +0000
Message-ID: <90233EDB-0366-4F66-9278-1FFF1AEF1C9B@oracle.com>
References: <20221028185712.79863-1-jlayton@kernel.org>
 <20221028185712.79863-5-jlayton@kernel.org>
 <89288CA4-F679-482C-B9D1-68C583D7F5BA@oracle.com>
 <d9079c1a165ed41b9ccc1d1434edc06624073338.camel@kernel.org>
In-Reply-To: <d9079c1a165ed41b9ccc1d1434edc06624073338.camel@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?yiLjHsPdLhlHJzRPRLUuhm6gNgoP6YfWEy0KwlXFJMmeERAqfLovSnAdwYZn?=
 =?us-ascii?Q?8vfb5CVXLzqcHMZBo2nipa3Amunsn9mMLdF1MLZRnZYCsGLdS1UyX8PF+zEY?=
 =?us-ascii?Q?byAoZgWyI9PoE67R8n64UHw78S2luAZ/5OT5CMhpXnNCHNW8xuWZDXTWHBep?=
 =?us-ascii?Q?rcG831LP1iqW1M4EYnEKq4dp6ashQ+ikChds0cMPy2cdFwtmXJx58eNDNk8A?=
 =?us-ascii?Q?VqBB2zf3SBS7/5Mkg9JPemyN6Hf8QfvFWyxNlhN/ANweCLNdj9Duj+OQZk+i?=
 =?us-ascii?Q?AOlQNnPBJQj1vqTtLNq5UEqgi69WXBZkNyTLAAArLdhRNOcGdpSRzOLULGTo?=
 =?us-ascii?Q?dW4h+fWJ3i14TpGXqApU5s3HJJbHel0DVfSMcv+wHCiw2/wAhuwukMFxb5xf?=
 =?us-ascii?Q?A4Vfq/1nwdABjGTEsYe0pwf5jI1qmSIpWqzYYdP3kAhPOP1jug7Xg1uG6Q7n?=
 =?us-ascii?Q?mTREVO6hHFNhKaQ2N/btdY1sfYMafB79yMtlVYbcgyzrhPr3TiM57bAIw3x0?=
 =?us-ascii?Q?ZtR2Bbuqa9d93g11/rplqPR3gYSrBIdwUNz1Ovx1CZxPVD6IO60TQj6ZJlV9?=
 =?us-ascii?Q?YVJPUnmZfaNqyZy8O+vZ0hLoCkgE+b2HqyZza6WHATI0QCq0HD/6Awv1GpUJ?=
 =?us-ascii?Q?hg7Wcrm7a9sq4RCiQRuLMwt2xD0kk2WB0gLBIV98bGI8Kl+7M8zXrAO3zOhh?=
 =?us-ascii?Q?n9ZKRAb21hJ9a2UAPgHnC8YDgw5ttuMV2paU5y7tE4jNEpERMfZOh/JWFbS3?=
 =?us-ascii?Q?W+q2j4AOZ6XnAJxomjcxaDuB3zP407xD7YHhtV/BtZmp2lCj9p07vdtHkJiJ?=
 =?us-ascii?Q?QzOfPMiqAdPBrxtm1u9pEy/9FbqWb5wXHglqxUtv8beEn8lfTg31tb/8RL0M?=
 =?us-ascii?Q?P1Tg4Khz/RbxjzZ3XVyXnfWQ2JKXwmWxGIBq492xfxd0srcOnDuqI7gDNbAG?=
 =?us-ascii?Q?GnHln9ZaMxtv/sW7mOzFtl9dYv6Ja8t0H8FbdWUkZQ9zVaRHmE8xg8B2Mu16?=
 =?us-ascii?Q?4SSvaoZx1RMO/8w74rBkcynoT1q5oMpRbvT/xzNbLmE/NOzdXwyochyeFs+t?=
 =?us-ascii?Q?/qDf6lvtUlCIWJB4T4QVnAAQZJUrh4kmtPNwk9eIDqxPkRwAsCQTZT3YXPXH?=
 =?us-ascii?Q?CX3NyTRH28qs7lh18AtL4+nfLd4J1yFGMO6wcsQuP/eMSNzrlP6+zSnSQ2FC?=
 =?us-ascii?Q?MJx7pnsg/sReyvDMcldTXdiqgihy7CczQ2RW7YzwQVj/GiFlty4CXX3iWj2z?=
 =?us-ascii?Q?eKA6vjot9WHaZC7KgtlKrYMpommIdwPB5gc0KXQwiy5z8DB+USwbEb3u6Kfl?=
 =?us-ascii?Q?WiZMeo2rDVYDtvwWf9G4fU0glfl9A9HNGaVkpk3V7aJaZB080Hx9F7JSyN5D?=
 =?us-ascii?Q?Vxr95HHqPBK6ZMWAWVZjDVfn/5kmeDEf0BPFGNwkhvoGKJc6QruB9tErQ50k?=
 =?us-ascii?Q?Td4Na3CTqVeW3ReZyYtRQMUw1bIuiyJzs/1NqgqjVTgM3b3DIPH+ggU44QHI?=
 =?us-ascii?Q?uxAREH0OPbkVMwhAt9K51xezZlZG+CrXSTlZ8zX5Tt8xd8F/O+R8houBP3Zq?=
 =?us-ascii?Q?oOc5XkyrYddMCtNIk64yje0XZGA8AVzp5SjCiuYAMy1bnZw/LUnrUADSJRrD?=
 =?us-ascii?Q?Bw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <870FFED3756E5A40BB82C468FD0E9794@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: 849cdd95-242c-4351-6b39-08dab92705bb
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2022 20:57:27.7222
 (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: tUy13B8PSBqnczCk1y6++W7jN4YppW6LTqeZw3kXRVBMDDffkkAZLWOmZ2QT18P9clfCp9X+f6OcSZpnw1OFFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6507
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-28_10,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 mlxscore=0 spamscore=0 phishscore=0 malwarescore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210280132
X-Proofpoint-GUID: XMaUvCh8wVoMs34TEJ3Tlz8NYndPebT_
X-Proofpoint-ORIG-GUID: XMaUvCh8wVoMs34TEJ3Tlz8NYndPebT_
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 28, 2022, at 4:30 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-10-28 at 19:50 +0000, Chuck Lever III wrote:
>>=20
>>> On Oct 28, 2022, at 2:57 PM, 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.
>>>=20
>>> 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
>> nfsd_write() calls nfsd_file_put() after every nfsd_vfs_write(). In some
>> cases, this new logic adds an async flush after every UNSTABLE NFSv3 WRI=
TE.
>>=20
>> I'll need to see performance measurements demonstrating no negative
>> impact on throughput or latency of NFSv3 WRITEs with large payloads.
>>=20
>>=20
>=20
> In your earlier mail, you mentioned that you wanted the writeback work
> to be done in the context of nfsd threads. nfsd_file_put is how nfsd
> threads put their references so this seems like the right place to do
> it.
>=20
> If you're concerned about calling filemap_flush too often because we
> have an entry that's cycling onto and off of the LRU, then another
> (maybe better) idea might be to kick off writeback when we clear the
> REFERENCED flag.

I think we are doing just about that today by flushing in nfsd_file_put
right when the REFERENCED bit is set. :-)

But yes: that is essentially it. nfsd is a good place to do the flush,
but we don't want to flush too often, because that will be noticeable.


> That would need to be done in the gc thread context, however.

Apparently it is already doing this via filp_close(), though it's
not clear how often that call needs to wait for I/O. You could
schedule a worker to complete the tear down if the open file has
dirty pages.

To catch errors that might occur when the client is delaying its
COMMITs for a long while, maybe don't evict nfsd_files that have
dirty pages...?


>>> 			/*
>>> 			 * We're racing with unhashing, so try to remove it from
>>> --=20
>>> 2.37.3
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever