On 11.10.19 г. 12:31 ч., Qu Wenruo wrote:
>
>
> On 2019/10/10 下午11:06, Nikolay Borisov wrote:
>> The code responsible for reading and initilizing tree roots is
>> scattered in open_ctree among 2 labels, emulating a loop. This is rather
>> confusing to reason about. Instead, factor the code in a new function,
>> init_tree_roots which implements the same logical flow.
>
>
> The refactor itself is great, but maybe we can make it better?
>
> Extra comment inlined below.
>
>>
>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
>> ---
>> fs/btrfs/disk-io.c | 136 ++++++++++++++++++++++++++-------------------
>> 1 file changed, 80 insertions(+), 56 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 2c3fa89702e7..b850988023aa 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2553,6 +2553,82 @@ static int btrfs_validate_write_super(struct
>> btrfs_fs_info *fs_info,
>> return ret;
>> }
>>
>> +int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
>> +{
>> +
>> + bool should_retry = btrfs_test_opt(fs_info, USEBACKUPROOT);
>> + struct btrfs_super_block *sb = fs_info->super_copy;
>> + struct btrfs_root *tree_root = fs_info->tree_root;
>> + u64 generation;
>> + int level;
>> + bool handle_error = false;
>> + int num_backups_tried = 0;
>> + int backup_index = 0;
>> + int ret = 0;
>> +
>> + while (true) {
>> + if (handle_error) {
>
> This two part doesn't look good enough to me.
>
> Can we do something like this?
>
> /*
> * change next_root_backup() to:
> * - Don't modify backup_index parameter
> * - Accept @index as 0, 1, 2, 3, 4.
> * 0 is regular sb tree roots, 1~4 is the backup roots, 1 is the best
> candiate
> * while 4 is the oldest candidate
> * - Check if we should try next backup (usebackuproot option)
> * - Return proper error value other than -1.
Patch 3 actually makes next_root_backup always return EINVAL.
> */
> for (backup_index = 0; next_root_backup(fs_info, backup_index) == 0;
> backup_index++) {
> /*
> * do the heavy lifting tree reads here
> */
> if (some_thing_went_wrong)
> goto next;
>
> /* Everything done correctly */
> break;
>
> next:
> /* Do the cleanup */
> }
I agree that the interface of next_root_backup could be cleaned further
in the spirit you suggested but I don't think the net result will be any
cleaner. We will still have a loop and a goto inside the loop which I
don't think is any cleaner. I'm not a big fan of the if (handle_error)
either but I think it's way more explicit.
>
> To me, that would look more sane other than strange error handling
> creeping around.
Let's see what other people think about the two approaches before
deciding which way to go .
>
> Thanks,
> Qu
<snip>
