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: AQHY6k5k3iaHcLvdwUOL1QTSt6QmjK4jykqAgAAebwCAAAbTAIAABgIAgAAZQACAAAYeAIAAE48A
Date:   Fri, 28 Oct 2022 18:53:21 +0000
Message-ID: <2D64526B-6270-49B9-AC2C-F0118DCF2AF9@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>
 <A040CDCA-5E3F-470F-8D69-8FF9DA4325FE@oracle.com>
 <098163d8067962f84a06af5d03379e2157974625.camel@kernel.org>
In-Reply-To: <098163d8067962f84a06af5d03379e2157974625.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?76jJG84QFi9Y4SlD0vcAhZTsCE4bL9yCJI/F2oYMANioUnsDtrNvEl8zw6wf?=
 =?us-ascii?Q?KWeUv06d1fLfLcdNWcKLbR35IKv8sM91xTC7ezQMlupCWmoYkvKwKEicqWjp?=
 =?us-ascii?Q?/ljA393wWyJVdSk/GsEmOC0SF9t0PRyiUlCIdImeSLndrJffPUckNo8u/HHh?=
 =?us-ascii?Q?mxa6sVjshVdDenV6wmP1GTh0c6XvQhHpVMTRVMxNjmb0u/cT1yylJIyNpfVS?=
 =?us-ascii?Q?a8E1FfwLR8Yo9rQfqLWe+P1X01PpmZalmHP3f6vX7WC9dxVvz04YRXn0FY2r?=
 =?us-ascii?Q?Kqhf2nbG0tK8wc0pwuS+PnTTxC81GWaRAAKjqHcQ1KeQ2ciz6SKTaa7WUbQK?=
 =?us-ascii?Q?V9dkvtvZ81oymJxGUhrB2UZIFdigdpyKvLarWApWcyniv4wqNfOB+0wVBU3Z?=
 =?us-ascii?Q?+uzWIOHp9w5HQj0sE8gqlPJFVubBCEnrlrswZbh2dFx5negAFiNCvq3GDSrF?=
 =?us-ascii?Q?SLpyttKWfHfrfRRkDhx0MAxBf8Pd8t21m1msnLb3/4JkYAfqBCqV7paQ7wPs?=
 =?us-ascii?Q?qmqPywpjXTT4+zlUKABS+KLqGFqh1b2NxhW3FdtmvibLioHO5C4jLA27DgqZ?=
 =?us-ascii?Q?dP52TnU5+viCA0tf9Lfk5dTrM4uhlMkmWIf4vL9Lgwy+eOHObuWYXlmxfcIp?=
 =?us-ascii?Q?VBlNlb2T1MKHVoW8lRVJ9Mhi6ogZqpE/HpsArh59KtVzQVN/UtBI4Nm9mZ0u?=
 =?us-ascii?Q?E1iCnFRYEV6xll4ixSHVAseLp/YChpSYYCPXFV5OsGd9aC+gPq2RnlW/REHU?=
 =?us-ascii?Q?iTUAEQvoeBGFKt6zBQW0VrwAT8ucV5izYFyxq9k0AEMkK+FSVOdd3l9K/woN?=
 =?us-ascii?Q?1sGqv/wJJf/187iBhcsDhcXvGuiZCmSk7nAyni0LaYwOqnSmIwIZWnRENGCO?=
 =?us-ascii?Q?7tt5RX98qcX7cG3NcQcMBi0Ysc9ip6pT+QCn39ogAHlIpp9W4Jvl7Rb3tNZ1?=
 =?us-ascii?Q?KJGdBxG03R4aRbR9lZMwGJyYyCm9HbLjUxSg7G+c0pfChsGgd4SmtZArbc9j?=
 =?us-ascii?Q?zRYihXvlQf7EQ3yPpgv5GlbgMZaDS4KvMS4jRC89KBMEdmqhYORNJqb6KjHv?=
 =?us-ascii?Q?I5CHejmugwceOTRpXh1CSwfKDk8tNpi29rUK/742DCecz/NFkf9SyzPP04Ij?=
 =?us-ascii?Q?JN/CZkZpp2zugCrPzyuzcLQDatZH0+xl0nDKwJLjcQY5fkfw4MqO6toKP4N9?=
 =?us-ascii?Q?Gu9pDG4NcgVtMe+RUyq+z/JAliAbu3UbVIS4Wr+UoBiUN8Ur9h26+1540+Df?=
 =?us-ascii?Q?bEPiz02tKRFon5CqwN1ecVhW/2/NlbNC2tFIsuaVqD8HSoZ8bK4PlxR+FXXb?=
 =?us-ascii?Q?qsSa7Kn6MKyKg+IeduSGeG7V4m/XD+x4EEdMH9XDcP3mG8u+vsrn22ibB7cv?=
 =?us-ascii?Q?fRvTso5IMtmiYfWw9G1/uh/BWLO22OgXuAKrcdWWUZAlT6aQKgsLJLbeWaSu?=
 =?us-ascii?Q?22Z8RNLJZnUu8rLfhF8tGn+CHpsnLe4Fe8LWwMC3oIGBXM5Yo/0kIgj+Yges?=
 =?us-ascii?Q?XCITh5PULQ4hoA4FS1g77kZMQYKFMY1x25qz6eMKiTViqRTMYwgQDzhCukhY?=
 =?us-ascii?Q?370MtmjY+4p5EhhOZ8xWtfXiQjGsxXLFxhLXT1apEeoYl0SHphaY25UkZI2s?=
 =?us-ascii?Q?WQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C31761E38F8CF94EAFC68639DBDFDE35@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: a96aeb18-38b4-4926-218a-08dab915af96
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2022 18:53:21.7493
 (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: f5kVqANjLjTfLSSQTTrSfxYaCBWwWHTrrqDyO2FIRVnhq1X1NVUj1NsrCGLXARm5EhJzG9Cb/x2qxK+YQMTMuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5737
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 spamscore=0 adultscore=0
 mlxlogscore=999 malwarescore=0 suspectscore=0 phishscore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210280119
X-Proofpoint-ORIG-GUID: -EPgI6t0djY0shP979r5IVzPOHJlJbce
X-Proofpoint-GUID: -EPgI6t0djY0shP979r5IVzPOHJlJbce
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 1:43 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Fri, 2022-10-28 at 17:21 +0000, Chuck Lever III wrote:
>>=20
>>> 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
>>>>> The problem with not calling vfs_fsync is that we might miss writebac=
k
>>>>> 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 er=
ror
>>>>> 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.
>>=20
>> I'm not clear either. I think I just have some hand-wavy requirements.
>>=20
>> 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.
>>=20
>=20
> The garbage collector doesn't call nfsd_file_put directly, though it
> will call nfsd_file_free, which now does a vfs_fsync.

OK, thought I saw some email fly by that suggested using nfsd_file_put
in the garbage collector. But... the vfs_fsync you mention can possibly
trigger I/O and wait for it (it's not the SYNC_NONE flush) in the GC
thread. Rare, but I'd rather not have even that possibility if we can
avoid it.


>> 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.
>>=20
>=20
> If this is what you want, then kicking off SYNC_NONE writeback when we
> put it on the LRU is the right thing to do.
>=20
> We want to ensure that when the thing is reaped from the LRU, that the
> final vfs_fsync has to write nothing back. The penultimate put that adds
> it to the LRU is almost certainly going to come in the context of an
> nfsd thread, so kicking off background writeback at that point seems
> reasonable.

IIUC the opposing idea is to do a synchronous writeback in nfsd_file_put
and then nothing in nfsd_file_free. Why isn't that adequate to achieve
the same result ?

Thinking aloud:

- Suppose a client does some UNSTABLE NFSv3 WRITEs to a file
- The client then waits long enough for the nfsd_file to get aged out
  of the filecache
- A local writer on the server triggers a writeback error on the file
- The error is cleared by other activity
- The client sends a COMMIT

Wouldn't the server miss the chance to bump its write verifier in that
case?


> Only files that aren't touched again get reaped off the LRU eventually,
> so there should be no danger of nfsd redirtying it again.

At the risk of rat-holing... IIUC the only case we should care about
is if there are outstanding NFSv3 WRITEs that haven't been COMMITed.
Seems like NFSv3-specific code, and not the filecache, should deal
with that case, and leave nfsd_file_put/free out of it...? Again, no
clear idea how it would, but just thinking about the layering here.


> By the time we
> get to reaping it, everything should be written back and the inode will
> be ready to close with little delay.


--
Chuck Lever