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

[Anand Jain <anand.jain@oracle.com>]

On 2/8/24 02:35, Qu Wenruo wrote:
> 
> 
> 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.

Indeed, reusing fewer fd is a good cleanup.

> 
>>
>>
>> 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.

  It is a step in the right direction.

> 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.


It depends on when the parent's fd is closed. Sequential
is better imo (See Josef comments in my patch), keeping
only one fd open at any time.
David's concerns about excess udev events and calls to
btrfs dev scan should be fine for mkfs since
no good-sb written yet.

> This would make open_ctree_fs_info() to get rid of __open_ctree_fd()
> completely.

It's a step forward, but adding another member to fs_info
for a helper fd as in this patch isn't wise.

> That would be a large work and does not provide really much benefit > compared to the current workaround.

Improving open_ctree are more in line with my patch's goal.

Thanks, Anand


> 
> Thanks,
> Qu
> 
>>
>> Thx. Anand
>>
>>