On Wed 06-11-24 23:16:44, Theodore Ts'o wrote: > On Fri, Aug 23, 2024 at 02:18:21PM +0800, Li Zetao wrote: > > Hi all, > > > > This patch set is dedicated to using scope-based resource management > > functions to replace the direct use of lock/unlock methods, so that > > developers can focus more on using resources in a certain scope and > > avoid overly focusing on resource leakage issues. > > > > At the same time, some functions can remove the controversial goto > > label(eg: patch 3), which usually only releases resources and then > > exits the function. After replacement, these functions can exit > > directly without worrying about resources not being released. > > > > This patch set has been tested by fsstress for a long time and no > > problems were found. > > Hmm, I'm torn. I do like the simplification that these patches can > offer. > > The potential downsides/problems that are worrying me: > > 1) The zero day test bot has flagged a number of warnings[1] > > [1] https://lore.kernel.org/r/202408290407.XQuWf1oH-lkp@xxxxxxxxx > > 2) The documentation for guard() and scoped_guard() is pretty sparse, > and the comments in include/linux/cleanup.h are positively > confusing. There is a real need for a tutorial which explains how > they should be used in the Documentation directory, or maybe a > LWN.net article. Still, after staring that the implementation, I > was able to figure it out, but I'm bit worried that people who > aren't familiar with this construt which appears to have laned in > August 2023, might find the code less readable. > > 3) Once this this lands, I could see potential problems when bug fixes > are backported to stable kernels older than 6.6, since this changes > how lock and unlock calls in the ext4 code. So unless > include/linux/cleanup.h is backported to all of the LTS kernels, as > well as these ext4 patches, there is a ris that a future (possibly > security) bug fix will result in a missing unlock leading to > hilarity and/or sadness. > > I'm reminded of the story of XFS changing the error return > semantics from errno to -errno, and resulting bugs when patches > were automatically backported to the stable kernels leading to > real problems, which is why XFS opted out of LTS backports. This > patch series could have the same problem.... and I haven't been > able to recruit someone to be the ext4 stable kernel maintainers > who could monitor xfstests resullts with lockdep enabled to catch > potential problems. > > That being said, I do see the value of the change > > What do other ext4 developers think? So overall I consider guards a useful feature. That being said I find changes as: - rcu_read_lock(); + guard(rcu)(); rcu_dereference(sbi->s_group_desc)[i] = bh; - rcu_read_unlock(); actively harmful because they make you go "Eww, so when *does* the RCU get released now?" If this was modified to: - rcu_read_lock(); + guard(rcu)() { rcu_dereference(sbi->s_group_desc)[i] = bh; } - rcu_read_unlock(); then I wouldn't mind *that* much but I see such change mostly as a pointless churn. OTOH if we managed to successfully convert e.g. ext4_fill_super() into using guards, then I would absolutely love that and in principle I think that is possible. The unwinding code there is not pretty with various #ifdef blocks, __maybe_unused annotations to silence warnings etc. But it would take more than just a quick replacement of obvious blocks to do that conversion. So to sum it up, I'm not against using guards but as I've glanced over the posted patches I find them to do more harm than good. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
