Re: [PATCH v8 00/12] btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror()

[David Sterba <dsterba@suse.cz>]

On Tue, Apr 04, 2023 at 09:08:31AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/4/1 00:17, David Sterba wrote:
> > On Fri, Mar 31, 2023 at 09:20:03AM +0800, Qu Wenruo wrote:
> >> This series can be found in my github repo:
> >>
> >> https://github.com/adam900710/linux/tree/scrub_stripe
> > 
> > This also includes the cleanup branch so I'll use this as topic branch
> > in for-next.
> 
> Thanks for that.
> 
> Just some questions inspired by the series.
> 
> [WAY TO CLEANUP]
> Just want to ask what's the proper way to do the cleanup.
> 
> Christoph mentioned in other subsystems they accept huge cleanup as long 
> as it's only deleting code, while in my series I did the split to try 
> keep each cleanup small.

I've just finished final pass through the scrub series so I can give you
a fresh answer. The split was great and thank you for taking the time to
do it. I don't treat deleting code differently than adding code.
Deleting also affects functionality, we've seen a recent example when
deleting code broke the thread_pool mount option. The review question is
"does it still work as expected when this code is gone?". As an extreme
example imagine deleting a mutex, it will make things faster but also
wrong.

If there are other subsystems where deleting code is not taken seriously
the same way new code is then I have my doubts about the review quality.

> But the split itself sometimes introduced dead code which is only going 
> to be removed later, and most of the time, such new code makes no sense 
> other than for patch split.

The reason I ask for split is to let me maintain focus on what's being
changed, and with too many deleted lines it's easy to overlook something
or sipmly fatigue.  Which means the review would be useless.
The removal patches are over 3000 lines in total, separately a few
hundreds each and logically grouped.

Adding a few lines to avoid warnings or to making it compile is a small
cost and as you did it with the comments it's quite clear where it is
and why.

> So I'm wondering what's the proper way to do huge cleanup in btrfs.

As you did it in the scrub series is a great example to follow for next
time.

> [FUTURE SCRUB UPDATE]
> There are still something I may want to do improving scrub.
> 
> One such objective is to enhance RAID56 scrubbing.
> 
> In that case, what branch should I base my code on?
> Normally I would go misc-next, but the new scrub is only in for-next.

I'll move it to misc-next so you can start there.