linux-kernel - Re: [RFC PATCH 0/8] use cleanup.h in btrfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <31b5e88c-0979-44cc-9e7a-1cb3320caf39@suse.com>
Date: Thu, 13 Nov 2025 07:25:33 +1030
From: Qu Wenruo <wqu@...e.com>
To: Gladyshev Ilya <foxido@...ido.dev>
Cc: Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/8] use cleanup.h in btrfs



在 2025/11/13 05:19, Gladyshev Ilya 写道:
> This series represents my experimentation with refactoring with
> cleanup guards. In my opinion, RAII-style locking improves readability
> in most cases and also improves code robustness for future code changes,
> so I tried to refactor simple cases that really benefits from lock guards.

Although I totally agree with the guard usages, it's not yet determined 
we will fully embrace guard usages.

> 
> However readability is a subjective concept, so you can freely disagree
> and reject any of those changes, I won't insist on any. Please note that
> patches 1-3 can be useful even without lock guards.
> 
> I didn't know how to split this series, mostly because it's just a lot of
> small changes... so I tried to split it by types of transformation:

And even if we're determined to go guard path, I doubt if it should be 
done in such a rushed way.

There are already some cases where scope based auto-cleanup conversion 
led to some regressions, no matter how trivial they seem.
Thankfully they are all caught early, but we have to ask one critical 
question:

   Have you run the full fstest test cases?

If not, please run it first. Such huge change is not really that easy to 
review.


Although I love the new scope based auto cleanup, I still tend to be 
more cautious doing the conversion.

Thus my recommendation on the conversion would be:

- Up to the author/expert on the involved field
   E.g. if Filipe wants to use guards for send, he is 100% fine to
   send out dedicated patches to do the conversion.

   This also ensures reviewablity, as such change will only involve one
   functionality.

- During other refactors of the code
   This is pretty much the same for any code-style fixups.
   We do not accept dedicated patches just fixing up whitespace/code-
   style errors.
   But if one is refactoring some code, it's recommended to fix any code-
   style related problems near the touched part.

So I'm afraid we're not yet at the stage to accept huge conversions yet.

Thanks,
Qu

> 
> 1. Patches 1-3 include some preparation work and simple fixes I noticed.
> 2. Patches 4-6  gradually increase the complexity of the refactored
>    situations, from simple lock/unlock pairs to scoped guards.
> 3. Patch 7 refactors functions which control flow can really benefit from
>    removed cleanups on exit. E.g. we can get rid of obscure if statements
>    in exit paths.
> 4. Patch 8 is kinda an example of overdone code refactoring and I predict
>    that it will be dropped anyway.
> 
> There is no TODOs for this series, but it's junk enough to be marked as
> RFC.
> 
> Gladyshev Ilya (8):
>    btrfs: remove redundant label in __del_qgroup_relation
>    btrfs: move kfree out of btrfs_create_qgroup's cleanup path
>    btrfs: simplify control flow in scrub_simple_mirror
>    btrfs: simplify function protections with guards
>    btrfs: use cleanup.h guard()s to simplify unlocks on return
>    btrfs: simplify cleanup via scoped_guard()
>    btrfs: simplify return path via cleanup.h
>    btrfs: simplify cleanup in btrfs_add_qgroup_relation
> 
>   fs/btrfs/block-group.c      |  24 ++----
>   fs/btrfs/compression.c      |  13 ++-
>   fs/btrfs/discard.c          |  20 ++---
>   fs/btrfs/disk-io.c          |   9 +-
>   fs/btrfs/extent-io-tree.c   |  72 ++++++----------
>   fs/btrfs/extent-tree.c      | 104 ++++++++++-------------
>   fs/btrfs/extent_io.c        |  33 ++++----
>   fs/btrfs/file-item.c        |   6 +-
>   fs/btrfs/free-space-cache.c |  87 +++++++------------
>   fs/btrfs/fs.c               |   9 +-
>   fs/btrfs/inode.c            |   3 +-
>   fs/btrfs/ordered-data.c     |  67 ++++++---------
>   fs/btrfs/qgroup.c           | 165 ++++++++++++++----------------------
>   fs/btrfs/raid56.c           |  20 ++---
>   fs/btrfs/scrub.c            |  19 ++---
>   fs/btrfs/send.c             |  40 ++++-----
>   fs/btrfs/space-info.c       |   4 +-
>   fs/btrfs/subpage.c          |  41 +++------
>   fs/btrfs/tree-log.c         |  28 +++---
>   fs/btrfs/volumes.c          |   3 +-
>   fs/btrfs/zoned.c            |  13 +--
>   fs/btrfs/zstd.c             |  13 +--
>   22 files changed, 299 insertions(+), 494 deletions(-)
> 
> 
> base-commit: 24172e0d79900908cf5ebf366600616d29c9b417