Received-SPF: pass (google.com: domain of linux-kernel+bounces-178198-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] fsnotify: clear PARENT_WATCHED flags lazily
In-Reply-To: <CAOQ4uxjKdkXLi6w2az9a3dnEkX1-w771ZUz1Lr2ToFFUGvf8Ng@mail.gmail.com>
References: <20240510221901.520546-1-stephen.s.brennan@oracle.com>
 <CAOQ4uxjKdkXLi6w2az9a3dnEkX1-w771ZUz1Lr2ToFFUGvf8Ng@mail.gmail.com>
Date: Mon, 13 May 2024 17:04:12 -0700
Message-ID: <87bk59gsxv.fsf@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?dWF5U3dHenlUaE42OUFsMnNUQzdBeGh6TVprQWt1R1hwMlhMZ0tRUWZ4K0lr?=
 =?utf-8?B?cGl2OGk3cDcxb3ZDUGdlYm1HdStyMy9oeGVMdDFOQ21ZcG1SWS9JdlRCdUJK?=
 =?utf-8?B?aFk1enpMUVcrSEg0RFpsd0Q0YVJiRnIrT3djdHV5MkorUXBjMmlKNWZTQkJC?=
 =?utf-8?B?eDg1WFpUMEo5MlN0b2RqSXROMEdzZzYraWRwT1dOYWEwNWFSNllhOFRaWENP?=
 =?utf-8?B?bUNwdUhOUlNram1ZUmdGNnhEVVFjd0huMDh5b3BwdTViL0EyWmZCLzRrbXk3?=
 =?utf-8?B?VndFTzJ3UUEwZUhpR0VsSW80Ym1hYmI1eDFwcjlsd09NR3NycllzaEVZUmRQ?=
 =?utf-8?B?U2FGekxIK1BrTUV1OWZSN25Od3FIZXNTSGljSzR2Zm1veXYvQ2tFU1dnNCt4?=
 =?utf-8?B?WnBkKzgyRzc5S3pkSE5JNTJQS29EbDEyamRzZmp6dlNWSFRKWlBSZGptYWJT?=
 =?utf-8?B?cUZnMFlzV1hmRWZ3aUgxNnlvT0l0WnhVM1lOcHhUTlNNZ01HeFFkaUlhZUFR?=
 =?utf-8?B?U2x2c3p0bG45c1QvR3pMeWNpT3pKcGF3dDBxR1lDbnhISXBpeDRwekVkdDJt?=
 =?utf-8?B?SDYzc0lheUFVclRXdmNXRHVrRmhiSXUzN2xiY01VQUYwL0tTOVllam0rSWRq?=
 =?utf-8?B?NytHeWpuWldndVdiU0J6SjFBQmpyWE1MYmRmdFBOOGI4M3lmOHZZRGVWSGp2?=
 =?utf-8?B?cXE1RGxKNzBTaVdLVWloQkFEUVVnM1hGcnBvcGdSRTJCdzhCOWhManZnckZJ?=
 =?utf-8?B?WVdxR0k1UVppTy8vV21iV0t0RDErQU9EbWxoV21JbWlyRi9TdUd3NzVlL3pv?=
 =?utf-8?B?R1I5c3d5RDRvK2pHSWI2Y3loQTlQRndVOUFOOHJheXJYcUVzSGJNcUtHTWNy?=
 =?utf-8?B?cXQ2QjBjVUkvVDNUWVZ4Z1g1aTN4MFIzY3VjcGJkdXlsRXkxWTNjM2J3eEgz?=
 =?utf-8?B?OGwrT0hmd25iRkVMTVlXTWZ2MGlZQXkwS09zT2tWOXk1NnhRbmZEcEJPNWlB?=
 =?utf-8?B?VUtsVlNHN1RaVDJjZk5wWGI5MDJKUWZqN0daaGQ4TitPQjNoR08zYU5Hc2V0?=
 =?utf-8?B?aEYzUC9JNDlGYXM5MnRsYTFVYUNkMG9Uc3dDOFg0eXNUVlhUUnZNM1VieDJY?=
 =?utf-8?B?SENxRm4vaFBrYk9rQTYrdklhdkFpNWZMbENmanlmOFRPcUZQMjRBMnNHS0xr?=
 =?utf-8?B?NDR1RXJpYTliS2N0WHRheFhBZFpTSXErNXd6VHFSUi9aeEdvM2ZPTlFhaUhr?=
 =?utf-8?B?Y1dKK0xDalBjVlNldFB3VStsbFF3aXR1M0ZsckJSYjVTNGVHZjk5dGNVOXc4?=
 =?utf-8?B?SWZIUE1PU1Q4dGFQUWpYL3dNbzI4ZnU4RVRSVFNaNnc3SkZiN290T1hpOXhB?=
 =?utf-8?B?UzUzTjUyZ3BTNXRoYlJQQzZNZFkyWVVUY3VtL0Y4RmsyUWRhK2YvcjFSNDNm?=
 =?utf-8?B?L1lIUjZIMURNdVliS2VET1JiTTZDWFJrQzhZTm1MVzlpb0JDVmpBSmxGRFFk?=
 =?utf-8?B?TW5KQitvUlZkd0t1TXVsZjNoQldYTVhpelA4UFZkeWx5VTJ1RlhVQUVGYXdj?=
 =?utf-8?B?bGxMRUR3YlYrK1lTb1lQN1daZmtNd0dpdHlkN08vcHQ1dGJzVnhRUHc1dTdZ?=
 =?utf-8?B?YkxJMWpkN1ZGTThMdGl0eW1JRFViTmlWT0ZFNG5STWpkbHdUYmRUSDNkTHBX?=
 =?utf-8?B?QUYwaFE5Uk1LU1FucmN4WFhWVElTMzhWSGlJRFUwTEdsbHo5S3ZhK3NtMFFl?=
 =?utf-8?B?L1U3emVIV3gwZ0lVWFVKYlRvYkUyeTNma0pPR094MUk3WmNOMnFCdnl0Wmd0?=
 =?utf-8?B?UCtVT1ZWN3Q4NWRkeHNFTVVFY1Z2RElnUG9HWDBPWTRsRlVZcERFZEJkZVFy?=
 =?utf-8?B?dW5KVjlXL3ZZNDRyMElPbnBhMkF5bXo4VkJIOVRibGNwV0trcXRrNmJaZkwr?=
 =?utf-8?B?SGw1WG9oRWV0eFRmenp5R0RNNDZpY0Nybnh3SUt6Z1VhcVpRQ2t2SVZJNEdH?=
 =?utf-8?B?eGVHQllwbnRGMEtERzRkMXNzMGtWcnowOVBBTkpxL3FTT3lYVUJNbkNtN0NV?=
 =?utf-8?B?T3A4VGlQeHd3bVN5cHdMUmJNYnZ3ZHJCZ1E3Z2RpdFJwRkZTZEZMaHN6bElY?=
 =?utf-8?B?T1hVUCtBWFJPUjVXdjV0K3NUVldRY1k3NDlXYmVoYkhXUzN2ajkveWNWZGln?=
 =?utf-8?B?ZVp3c20ybngyejlUZXo5NEpnc0VQM1dpbE85VXlzb2MrNE4xM3d6WjVsSGxJ?=
 =?utf-8?Q?vTWp5f8fj5Q+yu3ruDj3J649pLKlbYSXfvp8bfMiYE=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	rq2RZO2uXwGH8LxgErjIx/WLHNR81uTmoGVhfw3xGvLIoT75x4ghcMLV06LIYrqoJ6GQ4TzcxGGtbTeqK/yvVZTkTR16tPsjnhzuHOho6IyHdZ1qyuwFQs+ypcHXtqHpbMv6a8vnkpObP43/KRX+7LM5001HrQ2LqUG/TDDUWNcpXqAMemrJtz+QWj47jWQTHMbILQKVLvTRXrPa0EWsrK2TilLIGGLuqBui1Ec9iVaTZORNHXhSkbSeUesOIqAOCZlazq5127Fjun0qkX+dcQZeq4v4Q2fGvH50eQQba6xXqCGI6WjbBajpacCy3g6Z/o0rqMY37i7XWPIOhPbrYSeJWssiqG4wtsLE0Lsvr8xSPw1pjxz9PMhMr/I41uODsGxcod1tHOdCrx3oQVJIBgmyVDVO1kRfneq5bDwVRUaekTt8sR2Xy1Mk8UwwQCDGShjYQBLnBmSaIYYBjsQustAZ5Ik9egKDSyH6hLiXF+fmpf0weehZ784+oUo+3/KjpjnJz9OspP1HG9frzJr4CE1V843hGjM73/iF1/FTIx+g7DaMZ3SIXI0PR4YSHl85NkjVwDyy6warYy6jAlhjVmV9A9vzm9FqDHePnNXbskI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fb5d8127-69a7-4bb9-3f00-08dc73a963a9
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6597.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 00:04:13.9697
 (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: 5ngM7c8V7mbh8gUDPPuNO1JXMT9vV3lfbt7juBR+iWpoHEimgFL9Su6rl7XnKSIB8LIFPvkTHzbbU4WJmLrbFpr9ybjQAEfmADiNuzY/2j4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5809
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-13_17,2024-05-10_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 spamscore=0 suspectscore=0 mlxscore=0 malwarescore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405130165
X-Proofpoint-ORIG-GUID: ZAFd65XRq7SPrQ7NUgAQ4wy7FHCP6HLD
X-Proofpoint-GUID: ZAFd65XRq7SPrQ7NUgAQ4wy7FHCP6HLD

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, May 10, 2024 at 6:21=E2=80=AFPM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> Hi Amir, Jan, et al,
>
> Hi Stephen,
>
>>
>> It's been a while since I worked with you on the patch series[1] that ai=
med to
>> make __fsnotify_update_child_dentry_flags() a sleepable function. That w=
ork got
>> to a point that it was close to ready, but there were some locking issue=
s which
>> Jan found, and the kernel test robot reported, and I didn't find myself =
able to
>> tackle them in the amount of time I had.
>>
>> But looking back on that series, I think I threw out the baby with the
>> bathwater. While I may not have resolved the locking issues associated w=
ith the
>> larger change, there was one patch which Amir shared, that probably reso=
lves
>> more than 90% of the issues that people may see. I'm sending that here, =
since it
>> still applies to the latest master branch, and I think it's a very good =
idea.
>>
>> To refresh you, the underlying issue I was trying to resolve was when
>> directories have many dentries (frequently, a ton of negative dentries),=
 the
>> __fsnotify_update_child_dentry_flags() operation can take a while, and i=
t
>> happens under spinlock.
>>
>> Case #1 - if the directory has tens of millions of dentries, then you co=
uld get
>> a soft lockup from a single call to this function. I have seen some case=
s where
>> a single directory had this many dentries, but it's pretty rare.
>>
>> Case #2 - suppose you have a system with many CPUs and a busy directory.=
 Suppose
>> the directory watch is removed. The caller will begin executing
>> __fsnotify_update_child_dentry_flags() to clear the PARENT_WATCHED flag,=
 but in
>> parallel, many other CPUs could wind up in __fsnotify_parent() and decid=
e that
>> they, too, must call __fsnotify_update_child_dentry_flags() to clear the=
 flags.
>> These CPUs will all spin waiting their turn, at which point they'll re-d=
o the
>> long (and likely, useless) call. Even if the original call only took a s=
econd or
>> two, if you have a dozen or so CPUs that end up in that call, some CPUs =
will
>> spin a long time.
>>
>> Amir's patch to clear PARENT_WATCHED flags lazily resolves that easily. =
In
>> __fsnotify_parent(), if callers notice that the parent is no longer watc=
hing,
>> they merely update the flags for the current dentry (not all the other
>> children). The __fsnotify_recalc_mask() function further avoids excess c=
alls by
>> only updating children if the parent started watching. This easily handl=
es case
>> #2 above. Perhaps case #1 could still cause issues, for the cases of tru=
ly huge
>> dentry counts, but we shouldn't let "perfect" get in the way of "good en=
ough" :)
>>
>
> The story sounds good :)
> Only thing I am worried about is: was case #2 tested to prove that
> the patch really imploves in practice and not only in theory?
>
> I am not asking that you write a test for this or even a reproducer
> just evidence that you collected from a case where improvement is observe=
d
> and measurable.

I had not done so when you sent this, but I should have done it
beforehand. In any case, now I have. I got my hands on a 384-CPU machine
and extended my negative dentry creation tool so that it can run a
workload in which it constantly runs "open()" followed by "close()" on
1000 files in the same directory, per thread (so a total of 384,000
files, a large but not unreasonable amount of dentries).

Then I simply run "inotifywait /path/to/dir" a few times. Without the
patch, softlockups are easy to reproduce. With the patch, I haven't been
able to get a single soft lockup.

https://github.com/brenns10/kernel_stuff/tree/master/negdentcreate

    make
    mkdir test

    # create 384k files inside "test"
    ./negdentcreate -p test -c 384000 -t 384 -o create

    # start a loop opening and closing those files
    negdentcreate -p test -c 384000 -t 384 -o open -l

    # in another window:
    inotifywait test

Stephen

>
> Thanks,
> Amir.
>
>> [1]: https://lore.kernel.org/all/20221013222719.277923-1-stephen.s.brenn=
an@oracle.com/
>>
>> Amir Goldstein (1):
>>   fsnotify: clear PARENT_WATCHED flags lazily
>>
>>  fs/notify/fsnotify.c             | 26 ++++++++++++++++++++------
>>  fs/notify/fsnotify.h             |  3 ++-
>>  fs/notify/mark.c                 | 32 +++++++++++++++++++++++++++++---
>>  include/linux/fsnotify_backend.h |  8 +++++---
>>  4 files changed, 56 insertions(+), 13 deletions(-)
>>
>> --
>> 2.43.0
>>