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

["Theodore Ts'o" <tytso@mit.edu>]

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?

						- Ted