Sorry for multiple emails, however looking closely it appears this will make btrfs_close_devices should be the last thing in the thread, which means thread can not use the list after calling btrfs_close_devices(). That would confuse. Further not all threads using device_list_add() would call btrfs_open_devices() for eg cmd_show(), so there will still be memory leak since you can't call btrfs_close_devices() here. So since we have device_list_add() its better to have its undo part as a separate function and not something to do within close. Further, below patch which I submitted provided a way to delete a fsid+devices from the list. But just noticed that it missed the bug which you are addressing here and it should check if device is closed before releasing the list item. [PATCH 09/13] btrfs-progs: function to release a specific fsid from the list I can revamp this patch to the bug here, based feedback(s). (my new patch-set doesn't have to call device_list_fini() any more, so this patch is kind of void now). Thanks, Anand On 07/03/2013 01:01 PM, Anand Jain wrote:
further, you need to free device->label as well. ---- static int device_list_add(const char *path, struct btrfs_super_block *disk_super, u64 devid, struct btrfs_fs_devices **fs_devices_ret) { :: device->label = kstrdup(disk_super->label, GFP_NOFS); ---- disk_super->label is never null when disk_super is not null since its inline allocation. and kstrdup does len = strlen(s) + 1; which looks like device->label is never NULL, but I havn't traced down kmalloc_track_caller until to its end ----- 22 char *kstrdup(const char *s, gfp_t gfp) 23 { 24 size_t len; 25 char *buf; 26 27 if (!s) 28 return NULL; 29 30 len = strlen(s) + 1; 31 buf = kmalloc_track_caller(len, gfp); 32 if (buf) 33 memcpy(buf, s, len); 34 return buf; 35 } ---------- Thanks, Anand On 06/25/2013 09:02 PM, Wang Sheng-Hui wrote:Three kind of structures need to be freed on close: * All struct btrfs_device managed by fs_devices * The name field for each struct btrfs_device * The above items for seed_devices Signed-off-by: Wang Sheng-Hui <shhuiw@xxxxxxxxx> --- volumes.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/volumes.c b/volumes.c index d6f81f8..257b740 100644 --- a/volumes.c +++ b/volumes.c @@ -153,6 +153,16 @@ static int device_list_add(const char *path, return 0; } +static void btrfs_close_device(struct btrfs_device *device) +{ + close(device->fd); + device->fd = -1; + device->writeable = 0; + if (device->name) + kfree(device->name); + kfree(device); +} + int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) { struct btrfs_fs_devices *seed_devices; @@ -161,17 +171,17 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) again: list_for_each(cur, &fs_devices->devices) { device = list_entry(cur, struct btrfs_device, dev_list); - close(device->fd); - device->fd = -1; - device->writeable = 0; + btrfs_close_device(device); } seed_devices = fs_devices->seed; fs_devices->seed = NULL; if (seed_devices) { + kfree(fs_devices); fs_devices = seed_devices; goto again; } + kfree(fs_devices); return 0; }-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
