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 <mi...@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;
                }

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to