Message-Id: <52EF383A-7411-4DE8-90F7-8E943ABE26E6@dilger.ca>
Date: Thu, 7 Nov 2024 14:00:34 -0700
From: Andreas Dilger <adilger@...ger.ca>
To: Theodore Ts'o <tytso@....edu>
Cc: Li Zetao <lizetao1@...wei.com>,
 Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH -next 0/3] ext4: Using scope-based resource management
 function

On Nov 6, 2024, at 9:16 PM, Theodore Ts'o <tytso@....edu> 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?

Personally I don't see much improvement between the new code vs.
the existing code.  Essentially it looks like some macros that wrap
the following block of code with the lock/unlock.

Could it avoid some classes of bugs?  Maybe, for very simple cases
where the code block is very short, but I don't think few-line bodies
are the cases where "forgot to unlock" happens in practice.  That is
more likely to happen with huge sprawling functions with multiple
intermediate error cases that need to be unwound, and it isn't clear
if these constructs will help in the real cases where they are needed.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists