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

Anand Jain
2024-02-07T16:36:21Z

> 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()?
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

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.

Thx. Anand