Re: [PATCH 3/3] btrfs-progs: fix the stray fd close in open_ctree_fs_info()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2024/2/8 03:06, Anand Jain wrote:
>
>> index c053319200cb..05323b2cd393 100644
>> --- a/kernel-shared/disk-io.c
>> +++ b/kernel-shared/disk-io.c
>> @@ -913,6 +913,7 @@ struct btrfs_fs_info *btrfs_new_fs_info(int
>> writable, u64 sb_bytenr)
>>       fs_info->metadata_alloc_profile = (u64)-1;
>>       fs_info->system_alloc_profile = fs_info->metadata_alloc_profile;
>>       fs_info->nr_global_roots = 1;
>> +    fs_info->initial_fd = -1;
>>       return fs_info;
>> @@ -1690,7 +1691,10 @@ struct btrfs_fs_info *open_ctree_fs_info(struct
>> open_ctree_args *oca)
>>           return NULL;
>>       }
>
>
>>       info = __open_ctree_fd(fp, oca);
>> -    close(fp);
>> +    if (info)
>
>
>> +        info->initial_fd = fp;
>
> Why not do this in __open_ctree_fd()?

Because we want to keep open()/close() in the same layer when possible.

> then in the parent function, __open_ctree_fd you could:
>
>      if (!info)
>          close(fp);
>
>
>> +    else
>> +        close(fp);
>
>>       return info;
>>   }
>> @@ -2297,6 +2301,8 @@ skip_commit:
>>       btrfs_release_all_roots(fs_info);
>>       ret = btrfs_close_devices(fs_info->fs_devices);
>> +    if (fs_info->initial_fd >= 0)
>> +        close(fs_info->initial_fd);
>>       btrfs_cleanup_all_caches(fs_info);
>>       btrfs_free_fs_info(fs_info);
>>       if (!err)
>
>
> Essentially this patch converts the following nested open close
>
>
> fd1 open
> fd1 write zero
> fd1 write temp-sb
> fd2 open
> fd2 read super
> fd3 open devices
> fd2 close
> fd1 write temp-sb-to-remaining-dev-if-any
> fd3 write good-sb
> fd3 close
> fd1 close
>
> to
>
> [1]
> fd1 open
> fd1 write zero
> fd1 write temp-sb
> fd2 open
> fd2 read super
> fd3 open devices
> fd1 write temp-sb-to-remaining-dev-if-any
> fd3 write good-sb
> fd3 close
> fd2 close
> fd1 close
>
>
> However the patch
>    [PATCH RFC] btrfs-progs: mkfs: optimize file descriptor usage in
> mkfs.btrfs
>
>
> achieved: (And reduced one less fd)
>
> fd1 open
> fd1 write zero
> fd1 write temp-sb
> fd1 read super
> fd2 open devices
> fd1 write temp-sb-to-remaining-dev-if-any
> fd2 write good-sb
> fd2 close
> fd1 close

The problem we are hitting don't care how many fds we opened or
whatever, as long as the final close is later than the final fs commit.

>
>
> This patch saves the temporary fd (which is a helper fd to read the sb.
> fd2 in [1]) into a new member fs_info::initial_fd, does not make much
> sense to me. This fix is a kind of a quick, patchy solution rather
> than a ground-up clean design, imo.

Nope, your version needs all caller of open_ctree_fs_info() to converted
to the new interface.

This one keeps the interface untouched and still solves the problem.

Yes, this can be enhanced, but not in the way you did.

The proper way is to make open_ctree_fs_info() to not to rely on any
external fd, but really make open_ctree_fs_info() to only open fds for
its devices.

This would make open_ctree_fs_info() to get rid of __open_ctree_fd()
completely.
That would be a large work and does not provide really much benefit
compared to the current workaround.

Thanks,
Qu

>
> Thx. Anand
>
>