Re: [PATCH 2/3] btrfs-progs: mkfs: rework how we traverse rootdir

[Qu Wenruo <wqu@suse.com>]

在 2024/8/1 09:12, Boris Burkov 写道:
> On Thu, Aug 01, 2024 at 08:49:39AM +0930, Qu Wenruo wrote:
[...]
>> # mkfs.btrfs -f --rootdir rootdir test.img
>> ERROR: item file1 already exists but has wrong st_nlink 1 <= 1
>> ERROR: unable to traverse directory rootdir/: 1
>> ERROR: error while filling filesystem: 1
>>
>> And it failed as expeceted.
>>
> 
> Perfect example, it did seem to be vulnerable to this with just
> "original st_ino + 256"... Thanks for the new test.
> 
>>>
>>> In that case, if this is a bugfix, I think it makes sense to write a
>>> regression test that highlights it. It would be nice to pull the fix
>>> out of the refactor, too, but I suppose that with such a deep refactor,
>>> that might not be possible/worth it.
>>
>> Unfortunately I didn't even notice how serious these problems are, until
>> I tried...
> 
> That's kind of my concern with the higher level testing question, that
> we don't have much coverage for such a big refactor. OTOH, if we already
> had such serious bugs, how bad could the refactor really be :)

David is also pushing for the coverage tests:
https://github.com/kdave/btrfs-progs/actions/runs/10167512547

And indeed the coverage for mkfs/rootdir.c is not that good (71.2 %)

Another thing is, even if we go fsstress (already included 
btrfs-progs/tests) there are still limits what we can test.

E.g. the cross-mount point bug, and the out of rootdir hard link, as 
even fsstress can not create new mount point (unless using btrfs 
snapshot/subvolume)

To be honest, all the exotic new bugs are only exposed if we find some 
behavior unreliable, then reserve engineering to create test cases.

So in short, we need both fsstress based functionality tests, but we 
also need all those weird and seemingly impossible hand-crafted corner 
cases.

I just hope the new cases can improve the coverage.

[...]
>> A quick grep shows:
>>
>> 004-rootdir-keeps-size
>> 009-special-files-for-rootdir
>> 011-rootdir-create-file
>> 012-rootdir-no-shrink
>> 014-rootdir-inline-extent
>> 016-rootdir-bad-symbolic-link
>> 021-rfeatures-quota-rootdir
>> 022-rootdir-size
>> 027-rootdir-inode
> 
> Thanks for collecting those. By scanning those, it's hard to tell if
> there is, for example, a test of an "interesting" directory structure
> like the one I drew above, to test the traversal, for example.
> 
> Out of curiousity, I applied your patches, ran the tests (pass) then
> put a bug in the traversal code (only pop on cur_lvl > ftw_level, not
> ftw_level - 1) and 004-rootdir-keeps-size did fail, as it ran on the
> Documentation/ dir, which does seem like a sufficiently interesting
> directory.
> 
> I feel a bit better now :)
> 
> Maybe something that ran after fsstress (or however we test send/recv?)
> in the long run would be a good test, as we nail down some of the bugs
> you have hit here.
> 
> This comment is not directed at you, but is more general for btrfs
> development: I think explicitly stating the testing conducted on a
> complex patch is really helpful for reviewers.

In that case, the coverage rate would be something to improve next.

> 
>>
>>
>>> Is it
>>> in fstests? Knowing that all of the new code has at least run correctly
>>> would go a long way to feeling confident in the details of the
>>> transformation.
>>
>> We have btrfs-progs github CI, runs the all the selftests on each PR.
>> And it's all green (except a typo caught by code spell).
>>
>> [...]
>>>> -	parent_inum = highest_inum + BTRFS_FIRST_FREE_OBJECTID;
>>>> -	dir_entry->inum = parent_inum;
>>>> -	list_add_tail(&dir_entry->list, &dir_head->list);
>>>> +	/*
>>>> +	 * If our path level is higher than this level - 1, this means
>>>> +	 * we have changed directory.
>>>> +	 * Poping out the unrelated inodes.
>>>
>>> Popping
>>
>> Exposed by the CI, and fixed immediately in github.
>>
> 
> Cool!
> 
>>>
>>> Also, this took me like 15 minutes of working examples to figure out why
>>> it worked. I think it could definitely do with some deeper explanation
>>> of the invariant, like:
>>>
>>> ftwbuf->level - 1 is the level of the parent of the current traversed
>>> inode. ftw will traverse all inodes in a directory before leaving it,
>>> and will never traverse an inode without first traversing its dir,
>>> so if we see a level less than or equal to the last directory we saw,
>>> we are finished with that directory and can pop it.
>>>
>>> Perhaps with an annotated drawing like:
>>>
>>> 0: /
>>> 1:         /foo/
>>> 2:                 /foo/bar/
>>> 3:                         /foo/bar/f
>>> 2:                 /foo/baz/            POP! 2 > (2 - 1); done with /foo/bar/
>>> 3:                         /foo/baz/g
>>> 1:         /h                           POP! 2 > (1 - 1); done with /foo/baz/
>>>
>>> To help make it clearer. I honestly even think just changing to >= makes
>>> it clearer? Not sure about that.
>>
>> I'll add comments with an example to explain the workflow.
>>
>> BTW, David and I are working with Github review system a lot recently:
>> https://github.com/kdave/btrfs-progs/pull/855
>>
>> We do not force anyone to use specific system to do anything, but you
>> may find it a little easier to comment, and feel free to fall back to
>> the mail based review workflow at any time.
> 
> Happy to do code review in PRs! It's a bit annoying that the history
> gets blown up when the author re-pushes the branch after incorporating
> feeback (never understood that design, Phabricator tracks the *branch*
> history too and it seems very helpful to me.)

In fact that auto `outdated` marking is pretty useful for guys with a 
short memory, just like me.

Sometimes I forgot to address some comments, then the review part still 
shows the previous comment without `outdated`, reminding me something is 
missing.

Thanks,
Qu

> 
> BTW, if this review style is going well, we should discuss more and get
> the workflow more documented/supported in the workflow docs.
> 
> Also, while we are still here in email-land, you can add
> Reviewed-by: Boris Burkov <boris@bur.io>
> 
> Thanks,
> Boris
> 
>>
>> Thanks a lot for the detailed review!
>> Qu