Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() to provide better chance on damaged btrfs.

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

-------- Original Message --------
Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree() 
to provide better chance on damaged btrfs.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年02月12日 21:16
> On Thu, Feb 12, 2015 at 09:36:01AM +0800, Qu Wenruo wrote:
>> Subject: Re: [PATCH v3 00/10] Enhance btrfs-find-root and open_ctree()
>> to provide better chance on damaged btrfs.
>> From: David Sterba <dsterba@suse.cz>
>> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> Date: 2015年02月12日 01:52
>>> On Wed, Feb 11, 2015 at 08:33:03AM +0800, Qu Wenruo wrote:
>>>>>> Also, since only 2 patches is modified(although other part is slightly
>>>>>> modified to match the change), to avoid mail bombing, I created the pull
>>>>>> request on github and only send the first 2 patches with cover-letter.
>>>>>> https://github.com/kdave/btrfs-progs/pull/5
>>>>> Sending the changed patches only is ok (if you point me at the rest of
>>>>> the patches), but it's not necessary to open the github pull request.
>>>>>
>>>>> The version to version changelogs are also stored in the commit
>>>>> changelogs, that's a bit unexpected for a branch to be pulled.
>>>> Oh, very sorry for this.
>>>> I was meant to save your time, but I forgot that pull branch won't emit
>>>> the changelog like patches.
>>> Pulled except the last patch, and I've cleaned up some bits so please
>>> have a look. It's basically what I'd tell you during a normal review but
>>> now it was easier to do myself.
>> Thanks for merging and modifying them.
>> It seems that my naming sense is not so good and the new naming looks
>> good for me, except some of them,
>> like OPEN_CTREE_SUPPRESS_CHECK_TREE_ERROR seems too long for me, but
>> that's all right and doesn't
>> do any harm.
> Yeah it's long and I'm not completely happy about that but I gave
> preference to descriptiveness as we may want to add other fine tuning
> flags to open_ctree. As the TODO you've added to the enum definition
> says, we may split the flags and then cleanup as needed.
>
>> And it seems that the patch I send it still out of date and some naming
>> changes in my v3 patch doesn't show in
>> it...
> I've used the v3 branch from github. Please send incremental fixes if I
> missed something.
The modification seems better than mine, so that's completely OK for me.
No need for other patches.
>>> My concern about the patch "btrfs-progs: Allow open_ctree use backup
>>> tree root or search it automatically if primary..." is the
>>> 'automatically' part. Falling to the backup roots should be IMO on
>>> request. The tools should have (and some of them already do have)
>>> commandline options to request a given backup root. That way the user
>>> can try the default action and then decide if the backup roots are fine
>>> for use.
>> What about ask user to do such fallback method?
>> IMHO, such way should take a balance for average user and advanced user
>> who can, for example, extract
>> the bad block and fix it manually without corruption.
> Agreed this is about finding a good balance. However, here a regular
> user could do more harm than good if the tool just "does something" and
> does not stop if there's not a safe way forward.
>
>> Current btrfsck has a problem that doesn't give enough info on possible
>> solutions.
> Right, this should be improved in the long term. This requires
> documentation of the internals and some level of knowledge even if the
> tool provides options how to proceed.
>
>> The case is much like --init-(csum/extent)-tree, we provide such
>> options, but when a tree block in extent
>> tree happens, the backref mismatch error messages won't really help to
>> guide user to use --init-extent-tree
>> option.
>>
>> How do you think about the ask-user method?
> The options are:
>
> * exit if user input/decision is required
> * pass command line options to checker to preset the behaviour
> * add a specialized subcommand to fix a particular class of errors
> * run checker in interactive mode that asks user if she/he wants to fix
>    the problem, possibly how if there are more options
Thanks for all the options.
These options are better than my original just ask_user() method.
>
> The superblock and root backup options are very handy in the analysis
> phase, so one can see the differences in the output. I think the same
> holds for the find-root command. We don't have a "cookbook" how to
> analyze a broken filesystem. Your and my experience is probably
> different in how to do the analysis but I believe we will find a common
> ground and implement that into the tools and documentation.
This makes sense, I was only focused on the btrfsck --repair work, 
forgot that we can use btrfsck and btrfs-find-root
to do analysis before we do repair.

Considering analysis, I'd like a add a prompt if we failed to read tree 
root, guiding user to use the new
option like --search-root(which will first use backup and then 
find-root) or btrfs-find-root to find the best tree root.

Thanks,
Qu