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 v2 3/3] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Topic: [PATCH v2 3/3] nfsd: start non-blocking writeback after adding
 nfsd_file to the LRU
Thread-Index: AQHY6k5k3iaHcLvdwUOL1QTSt6QmjK4jykqAgAAebwCAAAbTAIAABgIAgAAZQAA=
Date:   Fri, 28 Oct 2022 17:21:27 +0000
Message-ID: <A040CDCA-5E3F-470F-8D69-8FF9DA4325FE@oracle.com>
References: <20221027215213.138304-1-jlayton@kernel.org>
 <20221027215213.138304-4-jlayton@kernel.org>
 <D32F829C-434C-4BA4-9057-C9769C2F4655@oracle.com>
 <ae07f54d107cf1848c0a36dd16e437185a0304c3.camel@kernel.org>
 <65194BBE-F4C7-4CD6-A618-690D1CCE235C@oracle.com>
 <cc4bfa448efedd0017fc7b20b8b7475907acbc5e.camel@kernel.org>
In-Reply-To: <cc4bfa448efedd0017fc7b20b8b7475907acbc5e.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?nmYB+6KA4DZIkIMr/LoBCD5vDod/6gldFPBriV5JbSZgbAm7n/bulHPQCC1/?=
 =?us-ascii?Q?HHebF0yt0lorg6zM3nDgLDbS/P2BfauWK35cHcvos8lomSizmprKZ+wWZ4jQ?=
 =?us-ascii?Q?+qsK/BfQ2JeuQue+nP314oFWPD8xwf7yy8Dl9iv1Walyx63nCYkPa0n349ee?=
 =?us-ascii?Q?jd61fwXhTkTlKABdmOilcmSlOpIJPHvQ651JR5EGpQUUyPF8v/TcbbmI2+/p?=
 =?us-ascii?Q?Hea2aw45yUWeOWb390A2FUo9ox0R7IIZAeTZJYcdSZcguy0Fj6i7Q7jyGfOK?=
 =?us-ascii?Q?XKeUTQCO0Bhz8voW+f0PlSUjxy0DMiKyVuNon1JU6Dbl1E2qGv0HPs3ug2QU?=
 =?us-ascii?Q?N2hDIZ2b0xPYVPHaI0k24TRrkRU8ctpswM5ckvqM1EJRvdnyGZOuOiwRCVqR?=
 =?us-ascii?Q?7W6jRibD0Ef64TKB578PLMSrwfBWenqq40rYx6BSSfCnlhn+YeQu5/eQK80k?=
 =?us-ascii?Q?CM/FIzVpL1AtuCPqLAOWqbI7S2tf0tFgj50YWfxZJQq5Hsc792tCvmsnBM1z?=
 =?us-ascii?Q?oDWr73jinnd2JlHDNAY42kPy8+a01f85ne/KnfNmktqy4FPXV0NBOm3rdoRI?=
 =?us-ascii?Q?dVTV2W1BjthALS2sMJ7eZUQbmTbQKQjWvUDuBIpTnruZRan8wfRnkmuWCMUH?=
 =?us-ascii?Q?yHDzog2IUQWklPH+DXWmUQ0BDtkrnqz3FBgr0WX8Wk5igAbgc/Y478u7Wz3A?=
 =?us-ascii?Q?nBR1QgoD/kpRUSClIhaH84z3aAMOoW3btvSCi02pvSMiaOJm1ZP3TWZk8PcC?=
 =?us-ascii?Q?lJCKQJ205AIo5C59Ep2bSAp5Y2AanSGKfQQoFASYOxE3PN7It1fbeWjtTkKT?=
 =?us-ascii?Q?/kPxi4L4xdLqPalUSIwIj19CJtPengSstFKQINbl2BRKwvezIGiBk4u39PXz?=
 =?us-ascii?Q?VTRYv5zbevWHZYSsaCGXAWd6wya7fwnDphNBrmamyMAaqfp30Lo0mxgcUWXf?=
 =?us-ascii?Q?Er//3IgNu/cbGLh7cQ3ayT5EchM3uctGy6KKtW3GBOMXVORCSjR88wtxALas?=
 =?us-ascii?Q?KqzCew85Sd4y2eRRQyY5dSJ7P+YqrX76mgBUmqvmrUPK9qDEjCLE4mBz6eXe?=
 =?us-ascii?Q?nT7yeBphZ6hIdjp5wy0vIfZ2i+JI6mXHZzL/lej8lg93MbM2RejqeWVFeLbN?=
 =?us-ascii?Q?ZHszUg92O/BEei1azinhtTa0qPNoeMx/ODIUGkPP0xqp+5V732h7YRRJh5j8?=
 =?us-ascii?Q?zHkPJ9gjlUc16KNbkNO+dX/t8VcCWhv0//ioBIrZhZwB/hXE6BFvU4QcJsWS?=
 =?us-ascii?Q?zAdnuDkUUVCiq4E0a1zyJyD09wYFMKD+IJG/nqyOvGW3S1uEDu1bDi5riQOr?=
 =?us-ascii?Q?1O9yGKs2DWLFycyXFlPTmJ4C1VYXQIEIQqYPxf808LAUPlg5V4OIVfQgf6AG?=
 =?us-ascii?Q?jIr2wO8J+y1h0Yw4gUv3BziPphWXJeFy32ucdVNdn6eAAM/k/XxZGHek+s7y?=
 =?us-ascii?Q?GBUYKhevK7R/Yg6hiADX3/uP/Z6O/J72rm06l8xQ4MBHDdVvv9W2NtEZ++L4?=
 =?us-ascii?Q?ZmHk3J1j4K9LFbiFvYIBPC1UuYmSgXdvLF3gG7RzWMbpqXnEVXeZ4LlhVA9w?=
 =?us-ascii?Q?vouwtkAt4euXvPn3Ml26pwn4y04GU7y1otKKjjc4fSp6xPkMznbKkvc/7Q67?=
 =?us-ascii?Q?IQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E113EFAE5CDEE046AC1C649D958D83A8@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: 921ad7af-2e68-44df-df5f-08dab908d894
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2022 17:21:27.0389
 (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: kbC9QChSr9gSK94oZLGlkhqbUGrqIuVVVZDNPBaFP2qIcUT7kRAXZwxwd1t+3JZApFAr1vHEX9VADm4jcnqOjg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4958
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_07,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxscore=0 mlxlogscore=999
 spamscore=0 phishscore=0 bulkscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2210280108
X-Proofpoint-ORIG-GUID: QYAmMBM10gyjU7OfhtJtT7E-24r_6oZP
X-Proofpoint-GUID: QYAmMBM10gyjU7OfhtJtT7E-24r_6oZP
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 11:51 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-10-28 at 15:29 +0000, Chuck Lever III wrote:
>>=20
>>> On Oct 28, 2022, at 11:05 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> On Fri, 2022-10-28 at 13:16 +0000, Chuck Lever III wrote:
>>>>=20
>>>>> On Oct 27, 2022, at 5:52 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 out when the time comes.
>>>>=20
>>>> For a large file, a background flush still has to walk the file's
>>>> pages to see if they are dirty, and that consumes time, CPU, and
>>>> memory bandwidth. We're talking hundreds of microseconds for a
>>>> large file.
>>>>=20
>>>> Then the final flush does all that again.
>>>>=20
>>>> Basically, two (or more!) passes through the file for exactly the
>>>> same amount of work. Is there any measured improvement in latency
>>>> or throughput?
>>>>=20
>>>> And then... for a GC file, no-one is waiting on data persistence
>>>> during nfsd_file_put() so I'm not sure what is gained by taking
>>>> control of the flushing process away from the underlying filesystem.
>>>>=20
>>>>=20
>>>> Remind me why the filecache is flushing? Shouldn't NFSD rely on
>>>> COMMIT operations for that? (It's not obvious reading the code,
>>>> maybe there should be a documenting comment somewhere that
>>>> explains this arrangement).
>>>>=20
>>>=20
>>>=20
>>> Fair point. I was trying to replicate the behaviors introduced in these
>>> patches:
>>>=20
>>> b6669305d35a nfsd: Reduce the number of calls to nfsd_file_gc()
>>> 6b8a94332ee4 nfsd: Fix a write performance regression
>>>=20
>>> AFAICT, the fsync is there to catch writeback errors so that we can
>>> reset the write verifiers (AFAICT). The rationale for that is described
>>> here:
>>>=20
>>> 055b24a8f230 nfsd: Don't garbage collect files that might contain write=
 errors
>>=20
>> Yes, I've been confused about this since then :-)
>>=20
>> So, the patch description says:
>>=20
>>    If a file may contain unstable writes that can error out, then we wan=
t
>>    to avoid garbage collecting the struct nfsd_file that may be
>>    tracking those errors.
>>=20
>> That doesn't explain why that's a problem, it just says what we plan to
>> do about it.
>>=20
>>=20
>>> The problem with not calling vfs_fsync is that we might miss writeback
>>> errors. The nfsd_file could get reaped before a v3 COMMIT ever comes in=
.
>>> nfsd would eventually reopen the file but it could miss seeing the erro=
r
>>> if it got opened locally in the interim.
>>=20
>> That helps. So we're surfacing writeback errors for local writers?
>>=20
>=20
> Well for non-v3 writers anyway. I suppose you could hit the same
> scenario with a mixed v3 and v4 workload if you were unlucky enough, or
> mixed v3 and ksmbd workload, etc...
>=20
>> I guess I would like this flushing to interfere as little as possible
>> with the server's happy zone, since it's not something clients need to
>> wait for, and an error is exceptionally rare.
>>=20
>> But also, we can't let writeback errors hold onto a bunch of memory
>> indefinitely. How much nfsd_file and page cache memory might be be
>> pinned by a writeback error, and for how long?
>>=20
>=20
> You mean if we were to stop trying to fsync out when closing? We don't
> keep files in the cache indefinitely, even if they have writeback
> errors.
>=20
> In general, the kernel attempts to write things out, and if it fails it
> sets a writeback error in the mapping and marks the pages clean. So if
> we're talking about files that are no longer being used (since they're
> being GC'ed), we only block reaping them for as long as writeback is in
> progress.
>=20
> Once writeback ends and it's eligible for reaping, we'll call vfs_fsync
> a final time, grab the error and reset the write verifier when it's
> non-zero.
>=20
> If we stop doing fsyncs, then that model sort of breaks down. I'm not
> clear on what you'd like to do instead.

I'm not clear either. I think I just have some hand-wavy requirements.

I think keeping the flushes in the nfsd threads and away from single-
threaded garbage collection makes sense. Keep I/O in nfsd context, not
in the filecache garbage collector. I'm not sure that's guaranteed
if the garbage collection thread does an nfsd_file_put() that flushes.

And, we need to ensure that an nfsd_file isn't pinned forever -- the
flush has to make forward progress so that the nfsd_file is eventually
released. Otherwise, writeback errors become a DoS vector.

But, back to the topic of this patch: my own experiments with background
syncing showed that it introduces significant overhead and it wasn't
really worth the trouble. Basically, on intensive workloads, the garbage
collector must not stall or live-lock because it's walking through
millions of pages trying to figure out which ones are dirty.


>>> I'm not sure we need to worry about that so much for v4 though. Maybe w=
e
>>> should just do this for GC files?
>>=20
>> I'm not caffeinated yet. Why is it not a problem for v4? Is it because
>> an open or delegation stateid will prevent the nfsd_file from going
>> away?
>>=20
>=20
>=20
> Yeah, more or less.
>=20
> I think that for a error to be lost with v4, it would require the client
> to have an application access pattern that would expose it to that
> possibility on a local filesystem as well. I don't think we have any
> obligation to do more there.
>=20
> Maybe that's a false assumption though.
>=20
>> Sorry for the noise. It's all a little subtle.
>>=20
>=20
> Very subtle. The more we can get this fleshed out into comments the
> better, so I welcome the questions.
>=20
>>=20
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>> fs/nfsd/filecache.c | 37 +++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 31 insertions(+), 6 deletions(-)
>>>>>=20
>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>>> index d2bbded805d4..491d3d9a1870 100644
>>>>> --- a/fs/nfsd/filecache.c
>>>>> +++ b/fs/nfsd/filecache.c
>>>>> @@ -316,7 +316,7 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key,=
 unsigned int may)
>>>>> }
>>>>>=20
>>>>> static void
>>>>> -nfsd_file_flush(struct nfsd_file *nf)
>>>>> +nfsd_file_fsync(struct nfsd_file *nf)
>>>>> {
>>>>> 	struct file *file =3D nf->nf_file;
>>>>>=20
>>>>> @@ -327,6 +327,22 @@ nfsd_file_flush(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;
>>>>> +	unsigned long nrpages;
>>>>> +
>>>>> +	if (!file || !(file->f_mode & FMODE_WRITE))
>>>>> +		return;
>>>>> +
>>>>> +	nrpages =3D file->f_mapping->nrpages;
>>>>> +	if (nrpages) {
>>>>> +		this_cpu_add(nfsd_file_pages_flushed, nrpages);
>>>>> +		filemap_flush(file->f_mapping);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> static void
>>>>> nfsd_file_free(struct nfsd_file *nf)
>>>>> {
>>>>> @@ -337,7 +353,7 @@ nfsd_file_free(struct nfsd_file *nf)
>>>>> 	this_cpu_inc(nfsd_file_releases);
>>>>> 	this_cpu_add(nfsd_file_total_age, age);
>>>>>=20
>>>>> -	nfsd_file_flush(nf);
>>>>> +	nfsd_file_fsync(nf);
>>>>>=20
>>>>> 	if (nf->nf_mark)
>>>>> 		nfsd_file_mark_put(nf->nf_mark);
>>>>> @@ -500,12 +516,21 @@ nfsd_file_put(struct nfsd_file *nf)
>>>>>=20
>>>>> 	if (test_bit(NFSD_FILE_GC, &nf->nf_flags)) {
>>>>> 		/*
>>>>> -		 * If this is the last reference (nf_ref =3D=3D 1), then transfer
>>>>> -		 * it to the LRU. If the add to the LRU fails, just put it as
>>>>> -		 * usual.
>>>>> +		 * If this is the last reference (nf_ref =3D=3D 1), then try
>>>>> +		 * to transfer it to the LRU.
>>>>> +		 */
>>>>> +		if (refcount_dec_not_one(&nf->nf_ref))
>>>>> +			return;
>>>>> +
>>>>> +		/*
>>>>> +		 * If the add to the list succeeds, try to kick off SYNC_NONE
>>>>> +		 * writeback. If the add fails, then just fall through to
>>>>> +		 * decrement as usual.
>>>>=20
>>>> These comments simply repeat what the code does, so they seem
>>>> redundant to me. Could they instead explain why?
>>>>=20
>>>>=20
>>>>> 		 */
>>>>> -		if (refcount_dec_not_one(&nf->nf_ref) || nfsd_file_lru_add(nf))
>>>>> +		if (nfsd_file_lru_add(nf)) {
>>>>> +			nfsd_file_flush(nf);
>>>>> 			return;
>>>>> +		}
>>>>> 	}
>>>>> 	__nfsd_file_put(nf);
>>>>> }
>>>>> --=20
>>>>> 2.37.3
>>>>>=20
>>>>=20
>>>> --
>>>> Chuck Lever
>>>>=20
>>>>=20
>>>>=20
>>>=20
>>> --=20
>>> Jeff Layton <jlayton@kernel.org>
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever