Received-SPF: pass (google.com: domain of linux-kernel+bounces-180419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Jan Kara <jack@suse.cz>
Cc: Amir Goldstein <amir73il@gmail.com>, 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: <20240515171503.loxpdv3xumgbc44w@quack3>
References: <20240510221901.520546-1-stephen.s.brennan@oracle.com>
 <CAOQ4uxjKdkXLi6w2az9a3dnEkX1-w771ZUz1Lr2ToFFUGvf8Ng@mail.gmail.com>
 <87bk59gsxv.fsf@oracle.com> <20240515171503.loxpdv3xumgbc44w@quack3>
Date: Wed, 15 May 2024 15:14:52 -0700
Message-ID: <87ttiypvs3.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?ZzhpVUc1RVBGUkNpRVlPMkZzUUdyTGJuSldvcjIvWGhlTTEweHN6MDYrdHgy?=
 =?utf-8?B?UWRFVnlPYjE0SWZ0bm1nei9OUzFmR29GY080L0poUFh2Y0VHZ3BaM1cwV3hG?=
 =?utf-8?B?Y09xWUJqdnhlK0Y5ckR5MUF0M3lZYTR1N2ExQ0NYSWgvVm5mV1orUHF3SVdh?=
 =?utf-8?B?UXQ3WEN2VjJCU3FVZWMwWk1SWVhXbndabXMvSWpWdkowdEk3bjRISE5GbVRj?=
 =?utf-8?B?UjJOaWlqMlAyYXVZNG5CSXhSdE8vY0pzcDRiMG5rOWwvVWNSR2hIZkJKMWYx?=
 =?utf-8?B?bGdMWUJUMGR0dTUwSVRYUEFMOEhlYmNYTUhGOVZNNWZjU1JzdFhCSmlMTlE0?=
 =?utf-8?B?MzJ2MXRIVnFMMG9LR2VGTndDR0Y5QnEwQUd6MjZUMWVGa3lGZStYaFd2WWVU?=
 =?utf-8?B?Q2JIY3F2UXZINCs4T0FRNkhNUEdLbHdKVjBidVlsTjNvMkN2ZTQyUzhZTnFX?=
 =?utf-8?B?eFRYSFR6eVM3Mk92K2JYRTJwa20ybkc4NERYWDJyZnhhVzVxd2ZQaGIvZU8w?=
 =?utf-8?B?aFFEMjJNZlU3ZXhRN0R1aUZFMmdKS2hTQnlWdS9xSU92MXhYZTZMVnRPMUxn?=
 =?utf-8?B?TGJ2bFNtNXl6cGtLc0F5Um5JbG5wRXZrTGZBMTFRWFo3R1Q2bTdXYjkvR0RF?=
 =?utf-8?B?aTg5RlJNWTM3alBxK2daRVRwbm5Pb1k1M25CYURNa1BKbE92YUpCM2gxSVlx?=
 =?utf-8?B?VDY1ZFJXVGRoZWE2TTB6MXVsOUMzS2xqdDhLYm54ZlFvTDkvbENsSFRsOG1n?=
 =?utf-8?B?cnN5Ym54QnZpNnQ0Yk0wTks1NU5oWU5XTlRJUmwvWm5ZMVRSQ0NHZnErUDVT?=
 =?utf-8?B?SWl6azg2SkVJVXhLUTZJVE9FVHpMazhSVkdOS29iODFuWG9VbDl3MjUvMWlq?=
 =?utf-8?B?MlhuZHM2NjNXMHZTN3ZuRDFhUFJWY3pNTmppa01vSURXekFWNVhQR2FrMCs0?=
 =?utf-8?B?Q3RWWStIR2xhbElrV3NqRnlnMjRuR01PYnkwdm5WalFTbEZ2MDVkOXlKRjhy?=
 =?utf-8?B?REhtN2p3SzVxcjVKM0hxUUZXM0IxbVI1RkF4VmtpZkFFQUZIZGg1SVl5b2Mv?=
 =?utf-8?B?ckQ1VVZwc25jbXNPUmtEa2owWE9rdkpnZytGYytYY0t3dlR2a0tTRk5MTk1S?=
 =?utf-8?B?MWFpc1lXL0tJWXBRTHFVTWJKS1d0dU91SllXdnd5NVQ3N0xJT20zRHE0MHNZ?=
 =?utf-8?B?MTdJVnNVaVJGNVdWZm9LMnBPd0haSWhqUU4rQkVqTHRQcDFCVFpxck54bllx?=
 =?utf-8?B?NnBXdnF1RkEwa3F6ODQyZTJCejR0bW81dUNwajFRN3I5ejJodVBqZVhHakIv?=
 =?utf-8?B?VlJqbDhGQXZHckEzYTBHRUlneXdZTkN5YlRWTFBoc1VHVVFvVS9NblU3enc5?=
 =?utf-8?B?M255UXhiYnpoOFJSTmlXNFMyZnB3eE96c2dRS1RNQm1LVmdXOUNqem9Oakhk?=
 =?utf-8?B?eGNFTVpmZ3VNUTAycDZ5T2VCd2xIV0tMSG94NkJrSDJ5UlBxNmlidklVVjZU?=
 =?utf-8?B?ZnBzRERYVTAyWWQyWW9OL0ExdDh2VzJtRFovOVVMQlFzaytoczhqOVNzbWNR?=
 =?utf-8?B?bGo4RWo2U0Vod3VxY1J0QnlIeElGRyt1bktudUlIRU5Eb1ZYazk0ZVpnSitI?=
 =?utf-8?B?Y0pVSWpKQno4L0FNdXRjdDl6YlR2bXRzUnJ5aGNDSjJoU1lBSVFyc0NvSnFj?=
 =?utf-8?B?VEhZaktQT3lyaW9aOHJFdlZUUm9HVUovM25tZmg1Wk1YaEpoeUpMTWdlSnlY?=
 =?utf-8?B?MmlXMzNzMjdjeUJWTURhYVV5dzBVZUlZUkt1dTdJTVExWklxcFd1eldJREhZ?=
 =?utf-8?B?cGhFVC9WV3A1NGV1WHZ5Qi9ZZGpIM3hHcmE0YW5ZZFhQcWNKTHBZSWFOMzI3?=
 =?utf-8?B?M2REeUZhaXo2SGtJMnZLRUtvS0JranlBdkVrUjdKREdzOHJlMmFsWXVPeHV1?=
 =?utf-8?B?MnYrL0RGSW5jSnZ6SGJ2a3VuaWI2MVFiOERReEM4Y0lQczVaYnR0UVZGcE0x?=
 =?utf-8?B?TDRwVnRRVmFYbjhwOXhWNDdoSEZ0U05wY3RuNzFVTjMzZG9FenBTYnQrd3Vj?=
 =?utf-8?B?amx1TUlzTlJ2UHVwRnQ2V2dNMEF6NXh2alJkODlXQXR4bDY0Uk8xQ0FKR2VU?=
 =?utf-8?B?QXl4QmlwRVhUenRjT2N0aUpTcEFrMElGZWZ1cWJkOFJmR1JHOGZ1dmV3YzdM?=
 =?utf-8?B?SFpCQXg3RkZ2ZlBvcHdBSEo5WUFCQmlmSjNPeDBacmorQ1E4NTJiYWxoODJD?=
 =?utf-8?B?OXU3SWpEeHh1dUsrN05sWEg4VGp3PT0=?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	5RMGdtgILsnhlNu4Fe+DM6M3sh2WyWr9bptoe/QvVyBW5p5DgTwTe+6wglU6AQ4hNHn0cZqjjFezQF8CQshcd3AJ+J5Ef0X+lqFuLX3cgbR5WmO7pYkNDrngBNGaGFIWR9LEQAgqET1nQhvj5vcJdnq1DHtT5mnwE3kZlfvnRX4KUJqeTvOuQT95yzEKx1ZPKCJCvJiAD1McDaZ3Txu1xfuvLxbCEBqsPmcCquii69rKCaO2an2taXQlLaCNaalPuR4LVZ342HzJW09yKtfGqkqXDWTpSaR7GvG897Qa9o3zAEN2v2uz3k71J+OSYHvuGSsBG8KAHvVZv4xD8m4//bIXfobqVym7/MrlC0AjL7uTIA6NAHiSQW1nR7+LU97joKs5Zv8iYP+EbLgY3yRPCF7EWLivbM31gLE2MUt3IcUmB627IU+1JZtMxJqVKe2bJ8oD9AeASiiOLaYP52tGyboJwHCI8r0+yIh6LKvS00CUblFQ5uMgoQcxJlnViHJItqbcqA+RZxcL3F4eb+LqBnxCHxcrM2k9m64GN88sdzP0kcYTuC7S2hd5276vTMXTtmvTVZdmLwz8y5SNKNef7huCLt2npofWH0uuS9VLczw=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d03ae0d1-6974-408e-fba6-08dc752c71ed
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6597.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 22:14:53.1769
 (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: FUVDWVFiML3QRz7daYTjKdxxjDSSHjcdQfhZjYllwNw0NL+C6SV7O/W2JHC+AG653eotmSmv/nbjs27ICiMVlZY6PIiPEzNKwq9ClzBEkws=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6640
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-15_14,2024-05-15_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 mlxscore=0
 spamscore=0 phishscore=0 suspectscore=0 mlxlogscore=989 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2405150159
X-Proofpoint-GUID: FogaO9gPDwMO7wUF07ivRXTURDj1Azzd
X-Proofpoint-ORIG-GUID: FogaO9gPDwMO7wUF07ivRXTURDj1Azzd

Jan Kara <jack@suse.cz> writes:
> On Mon 13-05-24 17:04:12, Stephen Brennan wrote:
>> Amir Goldstein <amir73il@gmail.com> writes:
>>=20
>> > 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=
 aimed to
>> >> make __fsnotify_update_child_dentry_flags() a sleepable function. Tha=
t work got
>> >> to a point that it was close to ready, but there were some locking is=
sues which
>> >> Jan found, and the kernel test robot reported, and I didn't find myse=
lf 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 th=
e
>> >> bathwater. While I may not have resolved the locking issues associate=
d with the
>> >> larger change, there was one patch which Amir shared, that probably r=
esolves
>> >> more than 90% of the issues that people may see. I'm sending that her=
e, since it
>> >> still applies to the latest master branch, and I think it's a very go=
od idea.
>> >>
>> >> To refresh you, the underlying issue I was trying to resolve was when
>> >> directories have many dentries (frequently, a ton of negative dentrie=
s), the
>> >> __fsnotify_update_child_dentry_flags() operation can take a while, an=
d it
>> >> happens under spinlock.
>> >>
>> >> Case #1 - if the directory has tens of millions of dentries, then you=
 could get
>> >> a soft lockup from a single call to this function. I have seen some c=
ases 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 directo=
ry. Suppose
>> >> the directory watch is removed. The caller will begin executing
>> >> __fsnotify_update_child_dentry_flags() to clear the PARENT_WATCHED fl=
ag, but in
>> >> parallel, many other CPUs could wind up in __fsnotify_parent() and de=
cide 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 r=
e-do the
>> >> long (and likely, useless) call. Even if the original call only took =
a second or
>> >> two, if you have a dozen or so CPUs that end up in that call, some CP=
Us will
>> >> spin a long time.
>> >>
>> >> Amir's patch to clear PARENT_WATCHED flags lazily resolves that easil=
y. In
>> >> __fsnotify_parent(), if callers notice that the parent is no longer w=
atching,
>> >> they merely update the flags for the current dentry (not all the othe=
r
>> >> children). The __fsnotify_recalc_mask() function further avoids exces=
s calls by
>> >> only updating children if the parent started watching. This easily ha=
ndles case
>> >> #2 above. Perhaps case #1 could still cause issues, for the cases of =
truly huge
>> >> dentry counts, but we shouldn't let "perfect" get in the way of "good=
 enough" :)
>> >>
>> >
>> > 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 obse=
rved
>> > and measurable.
>>=20
>> 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).
>>=20
>> 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.
>
> Thanks for the patch and for testing! I've added your patch to my tree (n=
ot
> for this merge window though) with a cosmetic tweak that instead of
> fsnotify_update_child_dentry_flags() we just have
> fsnotify_clear_child_dentry_flag() and fsnotify_set_children_dentry_flags=
()
> functions to make naming somewhat clearer.

Thank you Jan! I agree that change will make it clearer when reading
code and stack traces :)

-Stephen