Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Jan Kara <jack@suse.cz>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Al Viro <viro@zeniv.linux.org.uk>,
        Amir Goldstein <amir73il@gmail.com>
Subject: Re: [PATCH v4 0/5] fsnotify: fix softlockups iterating over d_subdirs
In-Reply-To: <20221111220614.991928-1-stephen.s.brennan@oracle.com>
References: <20221028001016.332663-1-stephen.s.brennan@oracle.com>
 <20221111220614.991928-1-stephen.s.brennan@oracle.com>
Date:   Fri, 11 Nov 2022 14:08:45 -0800
Message-ID: <87bkpdf8k2.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?hePXaf73F3naFnVBBbducmwNMDpOO+lZ8EFTuslZdz04lAlExsFKcxh7SJrI?=
 =?us-ascii?Q?kv7wiEpnQHMLdXI+qmATfgE3oGFu8lsNVUIKK1e+XXIJWqD7uxu+vyRVqZdz?=
 =?us-ascii?Q?vEMozwQW/LRZjLQwDlkyzQyPF7zqm8vR0oRas73XSg9oLqtk0r21fzMFtO4g?=
 =?us-ascii?Q?9p4d00PTZxo68g4I69ZsYLY6MMZjLqo43CyCcNYT231yyP8LdiWxMlwR0MF2?=
 =?us-ascii?Q?cvtdBXq9+YLHwdjgBzwPGXnqHO8FtjFDaPp2XPsemamCV/O2WjvTEEWGss1h?=
 =?us-ascii?Q?O7eUV47ZZ50ymGVOWAKlD+M92KwDjGZqr834ajyd5q40rMebdegc2fMy2V7e?=
 =?us-ascii?Q?G9PoJNpDechVhsox4sVJAg3CyNiDlonFPe8L0s0hj9Lvrv5t6qMuxsppXbEP?=
 =?us-ascii?Q?EE0yw9qHP/5ShS86B4YaBBoGcOJDjrd3SRtF3Lf6BgqUt3tXVGZIu6obuYtG?=
 =?us-ascii?Q?yrIWQF280RZWzeLN1jdDVco8dQWO614LNC6kFQYOZ2JjHfpRbDhm9AEVFU6Y?=
 =?us-ascii?Q?DudVYvQFByoqvrWngVX46B0B7wiR3OA6jtHT+R11+aC5CTumZJuC2br/67F4?=
 =?us-ascii?Q?6ZkmWjBuiJ/Ta47MdfHr8dUqCaZIWDsmFx9xnzJ275cLeUzD22Q82mjcXVG0?=
 =?us-ascii?Q?v4gOeZ4LEnI5hi2JhiW3zEihYBZx9we1vyaCG0zxyUxp9PSB7s0UXNBtXZ3a?=
 =?us-ascii?Q?ip6bkGdJm4Bh3nyFojW6imedjKMEYMEMsKJFB+yxE9fZ5XtLkZyxhp+mnkYL?=
 =?us-ascii?Q?MzEPKDNBy9hVOpKVHeKV4fIbNIyLWD9AyH78yGaAkvOgIB5S81JHL49fijbS?=
 =?us-ascii?Q?04VYxWgssENGRPsG2IcDEV1IdjZHpg12L4qjP0temr3Tehs3wPElIoxc7DJF?=
 =?us-ascii?Q?pYxo3YxUmRQX5sNVdivQvCdaiaAFuN3JfRncw0etjfuLYmp5hUBnQAilg8Uf?=
 =?us-ascii?Q?YGkjqLWo5EAXDJdVa17zC/fjp8vMlx3JqGLsD5jFjbooaSnJZk13pSi+81T4?=
 =?us-ascii?Q?3CDVw+6txj+p1cnxxDTrphsr7ea1GSCFhyqQ9qoVTtzYBOayAIths7zwWSCH?=
 =?us-ascii?Q?BHZUKvMng06UkZ3ruDwmPyp7CIQIEKXGQEMpqRYqndKB+3lISJyhUoQexNlr?=
 =?us-ascii?Q?OpCyWhkFIBdAIRmpM/lHYkWawf0FjYJw0ftsyMIl6PZpC95gdC6hxtRV3NJg?=
 =?us-ascii?Q?1ESN+M6gQzD4w8acBU/GEUNjd+qpRqJC5USl+QAUP/5wFohO6x4ciHnCbLvZ?=
 =?us-ascii?Q?NDdpiQv2xTRiAlZKbNdhs1tzLSVOQQIOPyvG3OzZB+IpSAQB5HB56mgMaRcK?=
 =?us-ascii?Q?rUEDhAwyFAZoqYcR9aEx9ONlcSm/soKaFySqq+KBCJjI5zwV9mtRvN/jVNhd?=
 =?us-ascii?Q?r32L+qZ06fcMSBc7Ts2NFBuk4rTyMJpWztSXawFM9juhQaPwXCX1CS1PwevV?=
 =?us-ascii?Q?G0e/WAP9qeMtLfR0hmiFpz0+Ou4FHNBpKrdzrJucqcRjiFOzR9ynTOHGk9Hm?=
 =?us-ascii?Q?MJWUN1C3FhAZnbuL9Nsa5etwMBdMFacWJ0wi/sxVtbtGJMpdcmtFUj9zXt/u?=
 =?us-ascii?Q?pgAtBkF+E9tEt2oFnEcLkPIdL0GhryLc2dTHf6ONtztJGpqwNUuS70MSKO3H?=
 =?us-ascii?Q?ww=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: cBOhNCTHLbOs1CWLmt4W3nkwINHQsRSIVD5c72WSqOcivKwsnw3eDYPWN5ovHTSaLXDRDkzQrRJIODrKRn3dMqjmH2So/kFQSPnuRPpq3BKpwpRyOSHfrbYdyFFs5trSY5b3+QPFkSWbH0hvhyw3g4rIZqnmQx+Urwx9vsXsbwzUq3pli+uQAJvXoQ2gzFwUaYyKPZMFuo+bJJAal9VlCc8XJZLhLtBgUEFxrlenqaIAuxb8U8Ofq7E5VVpPpAtkQOQwlEY5o7VwGbudkueS5Yuyx+3joVDfqAVJ/tDWW3rpiDl2v4ePCRhTOvndRfYVAX9HQqIHYj/UrLlE1rnRvsb7yZoYNNQp37zWHr70xsQb19Or9i9bkBCAsvIu4gsfqZYsMcACo68T+a6l5AYaVBGCmN9CrW9SiZNAYzFRJs56obw6+PcuJdhu0JPpUIeQ1JEMqWPSzR2fmcIaiSX1ZTn7UownxnPqz52aNb6drzptTMtdp0uZOOuDJtOhlf76psmGQWhZIj3ReSzFNXihQUtLg79gt2ZO2aoL7HQFaJUL6BkXJWnlHYB5eaLqGij01IbdjD+FxZLVvPdQ0ErTxvLjoot9szmmg4Ory/aj3hay5jTc2p8eqVTLxhGT6U8O02AyIPu65H+xjH4bJhGsmqcouDG6W4Ppwsab1kiTiBL2a+HqeX11qsxNDoSkl0qWKu5ifRgzjo3CUEgxWHmu6siDtUHtfyzuLo4ZTW3wWWQMD8o2yuwTIbDBbHl0Ldg9NqVZ2Hv9uR4/rTiKeV0qSRRIMPeLjIH8DJvLj9013kGnQyYkkD5NNNpGHFWDYrSeq4OTbkjGT3fBMgoZPH4Vyo0Zvh5zAMkDNynyrHmISTi+2q+n5WJIGPCmIRF5WcyncP7pQyzjj4HW8zkDSjNNrA==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d9067642-052a-414b-326e-08dac4314df1
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2022 22:08:46.7221
 (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: EHDIe6OBMRYKSi3M7s2DRf1qGOdWnTiWZV7DA0z6Yvhsd4yQ+v2wAJ3o3WTl7mmNguhCX25KM9RIIlf2mqJaf7S8hZTrN2xf4YXK/2m9imc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR10MB6914
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-11_11,2022-11-11_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 spamscore=0
 adultscore=0 malwarescore=0 mlxlogscore=999 bulkscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211110150
X-Proofpoint-ORIG-GUID: FklGPhwafZ5XGglXTdsmfMwcVmxDVWeU
X-Proofpoint-GUID: FklGPhwafZ5XGglXTdsmfMwcVmxDVWeU
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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Stephen Brennan <stephen.s.brennan@oracle.com> writes:

> Hi Jan, Amir, Al,
>
> Here's my v4 patch series that aims to eliminate soft lockups when updating
> dentry flags in fsnotify. I've incorporated Jan's suggestion of simply
> allowing the flag to be lazily cleared in the fsnotify_parent() function,
> via Amir's patch. This allowed me to drop patch #2 from my previous series
> (fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem). I
> replaced it with "fsnotify: require inode lock held during child flag
> update", patch #5 in this series. I also added "dnotify: move
> fsnotify_recalc_mask() outside spinlock" to address the sleep-during-atomic
> issues with dnotify.
>
> Jan expressed concerns about lock ordering of the inode rwsem with the
> fsnotify group mutex. I built this with lockdep enabled (see below for the
> lock debugging .config section -- I'm not too familiar with lockdep so I
> wanted a sanity check). I ran all the fanotify, inotify, and dnotify tests
> I could find in LTP, with no lockdep splats to be found. I don't know that
> this can completely satisfy the concerns about lock ordering: I'm reading
> through the code to better understand the concern about "the removal of
> oneshot mark during modify event generation". But I'm encouraged by the
> LTP+lockdep results.

Of course, I forgot to append the .config section:

#
# Lock Debugging (spinlocks, mutexes, etc...)
#
CONFIG_LOCK_DEBUGGING_SUPPORT=y
CONFIG_PROVE_LOCKING=y
# CONFIG_PROVE_RAW_LOCK_NESTING is not set
CONFIG_LOCK_STAT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y
CONFIG_DEBUG_RWSEMS=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_LOCKDEP=y
CONFIG_LOCKDEP_BITS=15
CONFIG_LOCKDEP_CHAINS_BITS=16
CONFIG_LOCKDEP_STACK_TRACE_BITS=19
CONFIG_LOCKDEP_STACK_TRACE_HASH_BITS=14
CONFIG_LOCKDEP_CIRCULAR_QUEUE_BITS=12
CONFIG_DEBUG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
# CONFIG_DEBUG_LOCKING_API_SELFTESTS is not set
# CONFIG_LOCK_TORTURE_TEST is not set
# CONFIG_WW_MUTEX_SELFTEST is not set
# CONFIG_SCF_TORTURE_TEST is not set
CONFIG_CSD_LOCK_WAIT_DEBUG=y
# end of Lock Debugging (spinlocks, mutexes, etc...)

>
> I also went ahead and did my negative dentry oriented testing. Of course
> the fsnotify_parent() issue is fully resolved, and when I tested several
> processes all using inotifywait on the same directory full of negative
> dentries, I was able to use ftrace to confirm that
> fsnotify_update_children_dentry_flags() was called exactly once for all
> processes. No softlockups occurred!
>
> I originally wrote this series to make the last patch (#5) optional: if for
> some reason we didn't think it was necessary to hold the inode rwsem, then
> we could omit it -- the main penalty being the race condition described in
> the patch description. I tested without the last patch and LTP passed also
> with lockdep enabled, but of course when multiple tasks did an inotifywait
> on the same directory (with many negative dentries) only the first waited
> for the flag updates, the rest of the tasks immediately returned despite
> the flags not being ready.
>
> I agree with Amir that as long as the lock ordering is fine, we should keep
> patch #5. And if that's the case, I can reorder the series a bit to make it
> a bit more logical, and eliminate logic in
> fsnotify_update_children_dentry_flags() for handling d_move/cursor races,
> which I promptly delete later in the series.
>
> 1. fsnotify: clear PARENT_WATCHED flags lazily
> 2. fsnotify: Use d_find_any_alias to get dentry associated with inode
> 3. dnotify: move fsnotify_recalc_mask() outside spinlock
> 4. fsnotify: require inode lock held during child flag update
> 5. fsnotify: allow sleepable child flag update
>
> Thanks for continuing to read this series, I hope we're making progress
> toward a simpler way to fix these scaling issues!
>
> Stephen
>
> Amir Goldstein (1):
>   fsnotify: clear PARENT_WATCHED flags lazily
>
> Stephen Brennan (4):
>   fsnotify: Use d_find_any_alias to get dentry associated with inode
>   dnotify: move fsnotify_recalc_mask() outside spinlock
>   fsnotify: allow sleepable child flag update
>   fsnotify: require inode lock held during child flag update
>
>  fs/notify/dnotify/dnotify.c      |  28 ++++++---
>  fs/notify/fsnotify.c             | 101 ++++++++++++++++++++++---------
>  fs/notify/fsnotify.h             |   3 +-
>  fs/notify/mark.c                 |  40 +++++++++++-
>  include/linux/fsnotify_backend.h |   8 ++-
>  5 files changed, 136 insertions(+), 44 deletions(-)
>
> -- 
> 2.34.1