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.

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:

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
-- 
2.51.1.dirty

在 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

On 11/12/25 23:55, Qu Wenruo wrote:
> 在 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.

No, because it's RFC only [however I tried to verify that no locking 
semantics/scopes changed and I tried not to touch any really complex 
scenarios.]
> 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.

Personally I don't like this approach for correctness-sensitive 
refactoring. When it's something dedicated and standalone, it's easier 
to focus and verify that all changes are valid

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

I would be surprised if such patchset would be accepted as one thing, 
honestly) For now, it's only purpose is to show how code _can_ be 
refactored in theory [not _should_]. And then, for example, if there is 
positive feedback on scoped guards, but not on full-function guards, I 
will send smaller, fully tested patch with only approved approaches.

Probably I should've been more clear on that in the cover letter, sorry...