Re: [PATCH 03/12] Btrfs-progs: Don't free the devices when close the ctree

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

  btrfs_close_devices() should reset fs_devices->latest_bdev
  and fs_devices->lowest_bdev as well they hold fd of the
  open dev in the list which is being closed.


On 07/03/2013 09:25 PM, Miao Xie wrote:
> Some commands(such as btrfs-convert) access the devices again after we close
> the ctree, so it is better that we don't free the devices objects when the ctree
> is closed, or we need re-allocate the memory for the devices.


> We needn't worry
> the memory leak problem, because all the memory will be freed after the taskes
> die.

  That will apply same for close as well right ?
  However from debugging and converting these functions
  as library-functions point of view its better if we
  have close/free called explicitly where possible.

  I am sending a patch to fix the the memory leak. So
  its fine if this patch just address the close issue.

Thanks, Anand


> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
>   btrfs-find-root.c | 21 +--------------------
>   disk-io.c         | 30 ++----------------------------
>   volumes.c         |  3 +++
>   3 files changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/btrfs-find-root.c b/btrfs-find-root.c
> index 3e1396d..da22c1d 100644
> --- a/btrfs-find-root.c
> +++ b/btrfs-find-root.c
> @@ -65,25 +65,6 @@ int csum_block(void *buf, u32 len)
>   	return ret;
>   }
>
> -static int close_all_devices(struct btrfs_fs_info *fs_info)
> -{
> -	struct list_head *list;
> -	struct list_head *next;
> -	struct btrfs_device *device;
> -
> -	return 0;
> -
> -	list = &fs_info->fs_devices->devices;
> -	list_for_each(next, list) {
> -		device = list_entry(next, struct btrfs_device, dev_list);
> -		if (device->fd != -1) {
> -			close(device->fd);
> -			device->fd = -1;
> -		}
> -	}
> -	return 0;
> -}
> -
>   static struct btrfs_root *open_ctree_broken(int fd, const char *device)
>   {
>   	u32 sectorsize;
> @@ -217,7 +198,7 @@ static struct btrfs_root *open_ctree_broken(int fd, const char *device)
>   out_chunk:
>   	free_extent_buffer(fs_info->chunk_root->node);
>   out_devices:
> -	close_all_devices(fs_info);
> +	btrfs_close_devices(fs_info->fs_devices);
>   out_cleanup:
>   	extent_io_tree_cleanup(&fs_info->extent_cache);
>   	extent_io_tree_cleanup(&fs_info->free_space_cache);
> diff --git a/disk-io.c b/disk-io.c
> index 4003636..a8176a5 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -35,8 +35,6 @@
>   #include "utils.h"
>   #include "print-tree.h"
>
> -static int close_all_devices(struct btrfs_fs_info *fs_info);
> -
>   static int check_tree_block(struct btrfs_root *root, struct extent_buffer *buf)
>   {
>
> @@ -1028,7 +1026,7 @@ out_chunk:
>   	if (fs_info->chunk_root)
>   		free_extent_buffer(fs_info->chunk_root->node);
>   out_devices:
> -	close_all_devices(fs_info);
> +	btrfs_close_devices(fs_info->fs_devices);
>   out_cleanup:
>   	extent_io_tree_cleanup(&fs_info->extent_cache);
>   	extent_io_tree_cleanup(&fs_info->free_space_cache);
> @@ -1261,30 +1259,6 @@ int write_ctree_super(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>
> -static int close_all_devices(struct btrfs_fs_info *fs_info)
> -{
> -	struct list_head *list;
> -	struct btrfs_device *device;
> -
> -	list = &fs_info->fs_devices->devices;
> -	while (!list_empty(list)) {
> -		device = list_entry(list->next, struct btrfs_device, dev_list);
> -		list_del_init(&device->dev_list);
> -		if (device->fd != -1) {
> -			fsync(device->fd);
> -			if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED))
> -				fprintf(stderr, "Warning, could not drop caches\n");
> -			close(device->fd);
> -			device->fd = -1;
> -		}
> -		kfree(device->name);
> -		kfree(device->label);
> -		kfree(device);
> -	}
> -	kfree(fs_info->fs_devices);
> -	return 0;
> -}
> -
>   static void free_mapping_cache(struct btrfs_fs_info *fs_info)
>   {
>   	struct cache_tree *cache_tree = &fs_info->mapping_tree.cache_tree;
> @@ -1337,7 +1311,7 @@ int close_ctree(struct btrfs_root *root)
>   		free(fs_info->log_root_tree);
>   	}
>
> -	close_all_devices(fs_info);
> +	btrfs_close_devices(fs_info->fs_devices);
>   	free_mapping_cache(fs_info);
>   	extent_io_tree_cleanup(&fs_info->extent_cache);
>   	extent_io_tree_cleanup(&fs_info->free_space_cache);
> diff --git a/volumes.c b/volumes.c
> index b88385b..0f6a35b 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -163,6 +163,9 @@ again:
>   	list_for_each(cur, &fs_devices->devices) {
>   		device = list_entry(cur, struct btrfs_device, dev_list);
>   		if (device->fd != -1) {
> +			fsync(device->fd);
> +			if (posix_fadvise(device->fd, 0, 0, POSIX_FADV_DONTNEED))
> +				fprintf(stderr, "Warning, could not drop caches\n");
>   			close(device->fd);
>   			device->fd = -1;
>   		}
>