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 v5 3/5] nfsd: rework refcounting in filecache
Thread-Topic: [PATCH v5 3/5] nfsd: rework refcounting in filecache
Thread-Index: AQHY7gDKf+UJMCSoA0e+WwOAy53hT64qlIAAgAAEOACAAAdXgIAACmQAgAEyK4CAABNLAA==
Date:   Wed, 2 Nov 2022 18:07:25 +0000
Message-ID: <4E9232A2-E4C0-4877-96B1-5D745085BB05@oracle.com>
References: <20221101144647.136696-1-jlayton@kernel.org>
 <20221101144647.136696-4-jlayton@kernel.org>
 <166733783854.19313.2332783814411405159@noble.neil.brown.name>
 <bb8a4e623371ad4ac9d49f2cbe0d4880e8ba52ef.camel@kernel.org>
 <166734032156.19313.13594422911305212646@noble.neil.brown.name>
 <4ed166accb2fd4a1aa6e4013ca7639bc2e610e37.camel@kernel.org>
 <db00762c5e0b983c72bee2c6bfa4476b2090a942.camel@kernel.org>
In-Reply-To: <db00762c5e0b983c72bee2c6bfa4476b2090a942.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?7guzHWCnjnWsxwXLa6SamJ2u0YUtLYy8Xd/ts36UqE5myDJAweTWgAwkY1c3?=
 =?us-ascii?Q?efAvaiFAW01MLaaAHH/fW+Y1Gem3stivYLzEpfs0dFCZGVvaYh+fsiEvLRpg?=
 =?us-ascii?Q?/OfwpzVTtcXIGIo1+W9BLe0KkihK1B2EYdYYK8Or5NmH0alqvJKI0FCum5VY?=
 =?us-ascii?Q?9u8HqOspraCl5OnKuEBi0f7F/r67iCd7FwuRVNQfTt6bYof786hs/1qw+YNW?=
 =?us-ascii?Q?yID8vVERlU3vJ1biBnYHgx8KeWz9qEnG9q8Z8AQGJdZ6DCuydwl/NcpX9/39?=
 =?us-ascii?Q?WXuenSyqsmC9eCiPVnzm+tJWT04l05ihjTNIlqhZWybmvACsohbGYJ4t8ldw?=
 =?us-ascii?Q?DQ6t2hscT9uc1ACXYcR0eU0jsZQb0MBx0Jij00jjxEiS4K9qEvKKdG5Rmx8P?=
 =?us-ascii?Q?5dlb3Tc7HzjRO9IPB41uh+4QKkj9DGcg1xecprjNq0OhkOAgDodmlEMc84Rt?=
 =?us-ascii?Q?2IOONi4sgro8U/LaWXt6IdSpjehLaoduuPQtWyC60L1csnmt7WUgxHpVFR03?=
 =?us-ascii?Q?tP6EpCCmXZ+j6SHAw8mICY0CfdI6Zvua4TIar6aTSyC2DxrQs/a4bv3k/0rb?=
 =?us-ascii?Q?vfRDSu1hZnyvdueLQcG60NaBlZorsUIs/FUxTmwb1SE+hzQdps+xEBkQ2n/q?=
 =?us-ascii?Q?boV2M8gvh00wAsCYk1OJGFej55XSf2ojugdsjRTblC5U/zRT++l0SnW3LJSU?=
 =?us-ascii?Q?lw9G7aNtdfcfl/yEUvRy80qi8XtpMZq/SCzvWlSQL+K6i8fpGWSoh0GJ+U7C?=
 =?us-ascii?Q?HyBiII3Xm6jQ6qI4Ph+8ZovD5L3J1ExSSDIRcZfRv0ga/OylKh33n/+AjzjH?=
 =?us-ascii?Q?c3HBOBlfLXU55WweOKxq4J2yVOgoN2Dw91U5JPH5iOUfM5aMyYJI1ONAbO3R?=
 =?us-ascii?Q?r/4WXVbOAflv8rrzJT5Ksu82Eh/HaQqU8KylDzkaIaPWpCz43A4JVYMff5P8?=
 =?us-ascii?Q?iFLiQFlXqmvXB77c54AChON8OX4sonet39fIiiJxdpy+dw+w0AadiEEF+OHY?=
 =?us-ascii?Q?xcXwd3sFYCeLN8Cjvtit6yEaMKAkOA5yXR3dDqbXlR7y7qlr39icexoSswGb?=
 =?us-ascii?Q?W+IpKyk4rd1chZp2CyX6PHLGe/zg7p7hl8Z1A66VVwwfwRBddjPeSn8lVGJx?=
 =?us-ascii?Q?WgO7n5qVSXdNsWOCHk1WAQiuE1LERZkA8W7EOgaSlF8r5s8d4fBmoeij2T6q?=
 =?us-ascii?Q?Mq6zwv/VqSWH1Y79dL2+hcKo4m3mxhiAHfhQMY2dEslRChGAo2uObVk2/7am?=
 =?us-ascii?Q?b5g2UgurYTEp+0j0w79YBCixcV8XshtZiAWQsCFBRjv617/GPjmRNEZRb/7h?=
 =?us-ascii?Q?dtnFcj2CJWJYo7nh1gdj+KuyFptVih3r1UKlTA6jYLHb2aFQFytPWeeAgqQw?=
 =?us-ascii?Q?F+wXpjeG1I1q2GeS2T1wmJIAaQcRoElm0duWhQLZ3alWJr60MkGTrSgSieyE?=
 =?us-ascii?Q?IaPhpN1G9cfyVQPFKmLZymql4X7cNRfMcaS9dHTIMRVHOA+RsvYpxhaNfUvg?=
 =?us-ascii?Q?Xm593tz388PuQCZPlMOgtsnaa+hFNI8AiL31DP4sd/4yLUcmc73vuoWRws85?=
 =?us-ascii?Q?nZa20mdryRLidiCPtSkxVl0zYwGC57YDKXJ13j1hb/RcHplDr5UrCd96J6hL?=
 =?us-ascii?Q?Kw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EBE2AB0479193A4F9F91EE4C9ADFEBF5@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: 8b0c1502-6899-4ec5-5483-08dabcfd1898
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Nov 2022 18:07:25.1574
 (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: 5b5Z+ovz7LjMZr1MRaow1OPOpio9PEaZhGMaUDZGVq3RfLgOuisL8LevtREZQH+SpIoiUXORJfExYEYKGI0yTg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4948
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-11-02_14,2022-11-02_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 mlxscore=0 adultscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211020117
X-Proofpoint-GUID: x_BYApeYwSQa8LA7czdS9luJeJo7JHaL
X-Proofpoint-ORIG-GUID: x_BYApeYwSQa8LA7czdS9luJeJo7JHaL
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 Nov 2, 2022, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Tue, 2022-11-01 at 18:42 -0400, Jeff Layton wrote:
>> On Wed, 2022-11-02 at 09:05 +1100, NeilBrown wrote:
>>> On Wed, 02 Nov 2022, Jeff Layton wrote:
>>>> On Wed, 2022-11-02 at 08:23 +1100, NeilBrown wrote:
>>>>> On Wed, 02 Nov 2022, Jeff Layton wrote:
>>>>>> The filecache refcounting is a bit non-standard for something search=
able
>>>>>> by RCU, in that we maintain a sentinel reference while it's hashed. =
This
>>>>>> in turn requires that we have to do things differently in the "put"
>>>>>> depending on whether its hashed, which we believe to have led to rac=
es.
>>>>>>=20
>>>>>> There are other problems in here too. nfsd_file_close_inode_sync can=
 end
>>>>>> up freeing an nfsd_file while there are still outstanding references=
 to
>>>>>> it, and there are a number of subtle ToC/ToU races.
>>>>>>=20
>>>>>> Rework the code so that the refcount is what drives the lifecycle. W=
hen
>>>>>> the refcount goes to zero, then unhash and rcu free the object.
>>>>>>=20
>>>>>> With this change, the LRU carries a reference. Take special care to
>>>>>> deal with it when removing an entry from the list.
>>>>>=20
>>>>> The refcounting and lru management all look sane here.
>>>>>=20
>>>>> You need to have moved the final put (and corresponding fsync) to
>>>>> different threads.  I think I see you and Chuck discussing that and I
>>>>> have no sense of what is "right".=20
>>>>>=20
>>>>=20
>>>> Yeah, this is a tough call. I get Chuck's reticence.
>>>>=20
>>>> One thing we could consider is offloading the SYNC_NONE writeback
>>>> submission to a workqueue. I'm not sure though whether that's a win --
>>>> it might just add needless context switches. OTOH, that would make it
>>>> fairly simple to kick off writeback when the REFERENCED flag is cleare=
d,
>>>> which would probably be the best time to do it.
>>>>=20
>>>> An entry that ends up being harvested by the LRU scanner is going to b=
e
>>>> touched by it at least twice: once to clear the REFERENCED flag, and
>>>> again ~2s later to reap it.
>>>>=20
>>>> If we schedule writeback when we clear the flag then we have a pretty
>>>> good indication that nothing else is going to be using it (though I
>>>> think we need to clear REFERENCED even when nfsd_file_check_writeback
>>>> returns true -- I'll fix that in the coming series).
>>>>=20
>>>> In any case, I'd probably like to do that sort of change in a separate
>>>> series after we get the first part sorted.
>>>>=20
>>>>> But it would be nice to explain in
>>>>> the comment what is being moved and why, so I could then confirm that
>>>>> the code matches the intent.
>>>>>=20
>>>>=20
>>>> I'm happy to add comments, but I'm a little unclear on what you're
>>>> confused by here. It's a bit too big of a patch for me to give a full
>>>> play-by-play description. Can you elaborate on what you'd like to see?
>>>>=20
>>>=20
>>> I don't need blow-by-blow, but all the behavioural changes should at
>>> least be flagged in the intro, and possibly explained.
>>> The one I particularly noticed is in nfsd_file_close_inode() which
>>> previously used nfsd_file_dispose_list() which hands the final close of=
f
>>> to nfsd_filecache_wq.
>>> But this patch now does the final close in-line so an fsnotify event
>>> might now do the fsync.  I was assuming that was deliberate and wanted
>>> it to be explained.  But maybe it wasn't deliberate?
>>>=20
>>=20
>> Good catch! That wasn't a deliberate change, or at least I missed the
>> subtlety that the earlier code attempted to avoid it. fsnotify callbacks
>> are run under the srcu_read_lock. I don't think we want to run a fsync
>> under that if we can at all help it.
>>=20
>> What we can probably do is unhash it and dequeue it from the LRU, and
>> then do a refcount_dec_and_test. If that comes back true, we can then
>> queue it to the nfsd_fcache_disposal infrastructure to be closed and
>> freed. I'll have a look at that tomorrow.
>>=20
>=20
> Ok, I looked over the notification handling in here again and there is a
> bit of a dilemma:
>=20
> Neil is correct that we currently just put the reference directly in the
> notification callback, and if we put the last reference at that point
> then we could end up waiting on writeback.

I expect that for an unlink, that is the common case.


> There are two notification callbacks:
>=20
> 1/ fanotify callback to tell us when the link count on a file has gone
> to 0.
>=20
> 2/ the setlease_notifier which is called when someone wants to do a
> setlease
>=20
> ...both are called under the srcu_read_lock(), and they are both fairly
> similar situations. We call different functions for them today, but we
> may be OK to unify them since their needs are similar.
>=20
> When I originally added these, I made them synchronous because it's best
> if nfsd can clean up and get out the way quickly when either of these
> events happen. At that time though, the code didn't call vfs_fsync at
> all, much less always on the last put.
>=20
> We have a couple of options:
>=20
> 1/ just continue to do them synchronously: We can sleep under the
> srcu_read_lock, so we can do both of those synchronously, but blocking
> it for a long period of time could cause latency issues elsewhere.
>=20
> 2/ move them to the delayed freeing infrastructure. That should be fine
> but we could end doing stuff like silly renaming when someone deletes an
> open file on an NFS reexport.

Isn't the NFS re-export case handled already by nfsd_file_close_inode_sync(=
) ?
In that case, the fsync / close is done synchronously before the unlink, bu=
t
the caller is an nfsd thread, so that should be safe.


> Thoughts? What's the best approach here?
>=20
> Maybe we should just leave them synchronous for now, and plan to address
> this in a later set?

I need to collect some experimental evidence, but we shouldn't be adding
or removing notification calls with your patch set. It ought to be safe
to leave it for a subsequent fix.


>>> The movement of flush_delayed_fput() threw me at first, but I think I
>>> understand it now - the new code for close_inode_sync is much cleaner,
>>> not needing dispose_list_sync.
>>>=20
>>=20
>> Yep, I think this is cleaner too.
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever