Re: [PATCH -next 0/3] ext4: Using scope-based resource management function

[Jan Kara <jack@suse.cz>]

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@intel.com
> 
> 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@suse.com>
SUSE Labs, CR