On Wed, Apr 03, 2019 at 10:26:05PM +0800, Qu Wenruo wrote:
> On 2019/4/3 下午10:21, David Sterba wrote:
> > On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote:
> >> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
> >> zero item") introduced comprehensive root owner checker.
> >
> > That's 4.7, so we might want to do stable backport to 4.9+.
>
> It's just a performance optimization.
>
> I'm not sure if we should backport such things even if we can solve the
> conflicts.
That's for the consideration, if the performance improvement is
significant, but due to other changes in the codebases it might not be
safe.
> >> However it's pretty expensive tree search to locate the owner root,
> >> especially when it get reused by mandatory read and write time
> >> tree-checker.
> >>
> >> This patch will remove that check, and completely rely on owner based
> >> empty leaf check, which is much faster and still works fine for most
> >> case.
> >>
> >> And since we skip the old root owner check, now write time tree check
> >> can be merged with btrfs_check_leaf_full().
> >
> > And for that reason the change should be minimal, this patch depends on
> > current development queue so it's not suitable for the backport.
> >
> > I'm thinking what to do here, I want to avoid patches that effectively
> > revert changes that are still in the development branch and haven't been
> > released.
>
> OK, I'll resend write time tree checker patchset.
Wait, it's just one patch that needs update, "btrfs: Do mandatory tree
block check before submitting bio".
If the above patch is dropped, then the expensive checks in check_leaf
can be removed as you do in this patch, without the updates to
parameters.
> > This means to rework "btrfs: Do mandatory tree block check before
> > submitting bio", though it's deep in the branch, the scope of changes is
> > only 2 files so this should not cause any conflicts.
>
> BTW, I'm a little interesting in the workflow on your side.
>
> In this case, what's the correct way to handle them?
> Just remove those two related patches from misc-next branch and re-apply
> the newer version?
If you mean these two
btrfs: disk-io: Show the timing of corrupted tree block explicitly
btrfs: Do mandatory tree block check before submitting bio
we don't need to remove both, just the second one.
> Or remove all those write time tree checker and later enhancement
> patches and reapply them all?
No, why removing them? That's independent enhancement of the tree
checker. What needs to be removed is the call at write time.
> Either way, I hope to experience the same workflow so I could reduce
> your workload.
This is an infrequent case when a patch deep in the branch needs update
so the solution depends on the impact on the patches above it. Here it's
only minor conflict with a cleanup patch removing redundant fs_info from
check_leaf.
So that's trivial and we don't need to update the patches in place.
Further updates can be applied on top of misc-next (with the patch
removed).
The ideal workflow is:
1) patches are developed, tested by the developer
2) posted to mailinglist
3) after review added to a topic branch in for-next for wider testing
- patchset revisions are easy to update as it's in a topic branch
4) after some time the patches get merged to misc-next and no updates are
expected, beyond minor updates like coding style or changelog
5) patch goes to release branch and is effectively frozen, all fixes go
as separate patches
So if you want to reduce my workload, spend more time on 1-3 so we
don't have to be creative when fixing problems we discover at 4-5.
I know this cannot be followed 100% times, but so far we've been able to
resolve all the situations and the number of times is overall low. So
it's bearable.
