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

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

> 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