[PATCH 2/5] libbtrfsutil: fix memory leak in deleted_subvolumes()

2018-03-29 Thread Omar Sandoval
From: Omar Sandoval 

If we fail to reallocate the ID array, we still need to free it.

Signed-off-by: Omar Sandoval 
---
 libbtrfsutil/subvolume.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index d6c0ced8..867b3e10 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -1353,8 +1353,10 @@ PUBLIC enum btrfs_util_error 
btrfs_util_deleted_subvolumes_fd(int fd,
new_capacity = capacity ? capacity * 2 : 1;
new_ids = reallocarray(*ids, new_capacity,
   sizeof(**ids));
-   if (!new_ids)
-   return BTRFS_UTIL_ERROR_NO_MEMORY;
+   if (!new_ids) {
+   err = BTRFS_UTIL_ERROR_NO_MEMORY;
+   goto out;
+   }
 
*ids = new_ids;
capacity = new_capacity;
-- 
2.16.3

--
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


Re: [PATCH] btrfs-progs: mkfs/rootdir: Fix memory leak in traverse_directory()

2018-02-14 Thread David Sterba
On Wed, Feb 14, 2018 at 03:50:06PM +0800, Qu Wenruo wrote:
> The bug is exposed by mkfs test case 009, with D=asan.
> 
> We are leaking memory of parent_dir_entry->path() which ,except the
> rootdir, is allocated by strdup().
> 
> Before fixing it, unifiy the allocation of parent_dir_entry() to heap
> allocation.
> 
> Then fix it by adding "free(parent_dir_entry->path);" in
> traverse_directory() and error handler.
> 
> Signed-off-by: Qu Wenruo 

Applied, thanks.
--
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


Re: [PATCH] btrfs-progs: convert/ext2: Fix memory leak caused by handled ext2_filsys

2018-02-14 Thread David Sterba
On Wed, Feb 14, 2018 at 04:09:56PM +0800, Qu Wenruo wrote:
> Exposed convert-test with D=asan.
> 
> Unlike btrfs, ext2fs_close() still leaves its ext2_filsys parameter
> filled with allocated pointers.
> 
> It needs ext2fs_free() to free those pointers.
> 
> Signed-off-by: Qu Wenruo 

Applied, thanks.
--
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


[PATCH] btrfs-progs: convert/ext2: Fix memory leak caused by handled ext2_filsys

2018-02-14 Thread Qu Wenruo
Exposed convert-test with D=asan.

Unlike btrfs, ext2fs_close() still leaves its ext2_filsys parameter
filled with allocated pointers.

It needs ext2fs_free() to free those pointers.

Signed-off-by: Qu Wenruo 
---
 convert/source-ext2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index e9277213b77a..67e97cf87185 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -95,6 +95,7 @@ static int ext2_open_fs(struct btrfs_convert_context *cctx, 
const char *name)
return 0;
 fail:
ext2fs_close(ext2_fs);
+   ext2fs_free(ext2_fs);
return -1;
 }
 
@@ -179,6 +180,7 @@ static void ext2_close_fs(struct btrfs_convert_context 
*cctx)
cctx->volume_name = NULL;
}
ext2fs_close(cctx->fs_data);
+   ext2fs_free(cctx->fs_data);
 }
 
 static u8 ext2_filetype_conversion_table[EXT2_FT_MAX] = {
-- 
2.16.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


[PATCH] btrfs-progs: mkfs/rootdir: Fix memory leak in traverse_directory()

2018-02-13 Thread Qu Wenruo
The bug is exposed by mkfs test case 009, with D=asan.

We are leaking memory of parent_dir_entry->path() which ,except the
rootdir, is allocated by strdup().

Before fixing it, unifiy the allocation of parent_dir_entry() to heap
allocation.

Then fix it by adding "free(parent_dir_entry->path);" in
traverse_directory() and error handler.

Signed-off-by: Qu Wenruo 
---
 mkfs/rootdir.c | 5 +++--
 mkfs/rootdir.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index 9be4bcbfd0b5..f9472e126674 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -453,7 +453,6 @@ static int traverse_directory(struct btrfs_trans_handle 
*trans,
ino_t parent_inum, cur_inum;
ino_t highest_inum = 0;
const char *parent_dir_name;
-   char real_path[PATH_MAX];
struct btrfs_path path;
struct extent_buffer *leaf;
struct btrfs_key root_dir_key;
@@ -464,7 +463,7 @@ static int traverse_directory(struct btrfs_trans_handle 
*trans,
if (!dir_entry)
return -ENOMEM;
dir_entry->dir_name = dir_name;
-   dir_entry->path = realpath(dir_name, real_path);
+   dir_entry->path = realpath(dir_name, NULL);
if (!dir_entry->path) {
error("realpath  failed for %s: %s", dir_name, strerror(errno));
ret = -1;
@@ -616,6 +615,7 @@ static int traverse_directory(struct btrfs_trans_handle 
*trans,
}
 
free_namelist(files, count);
+   free(parent_dir_entry->path);
free(parent_dir_entry);
 
index_cnt = 2;
@@ -686,6 +686,7 @@ fail:
dir_entry = list_entry(dir_head.list.next,
   struct directory_name_entry, list);
list_del(_entry->list);
+   free(dir_entry->path);
free(dir_entry);
}
 out:
diff --git a/mkfs/rootdir.h b/mkfs/rootdir.h
index d0fc2eb58563..f06c7dd16e3c 100644
--- a/mkfs/rootdir.h
+++ b/mkfs/rootdir.h
@@ -23,7 +23,7 @@
 
 struct directory_name_entry {
const char *dir_name;
-   const char *path;
+   char *path;
ino_t inum;
struct list_head list;
 };
-- 
2.16.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


Re: [PATCH] btrfs: Fix memory leak on multiple mounts

2017-12-18 Thread Nikolay Borisov


On 19.12.2017 08:05, Misono, Tomohiro wrote:
> Hello, 
> 
> On 2017/12/18 19:06, Nikolay Borisov wrote:
>>
>>
>> On 18.12.2017 12:03, Nikolay Borisov wrote:
>>> Currently if a mounted-btrfs instance is mounted for the 2nd time
>>> without first unmounting the first instance then we hit a memory leak
>>> in btrfs_mount_root due to the fs_info of the acquired superblock is
>>> different than the newly allocated fs info. Fix this by specifically
>>> checking if the fs_info instance of the newly acquired superblock is
>>> the same as ours and free it if not.
>>>
>>> Reproducer:
>>>
>>> mount /dev/vdc /media/scratch
>>> mount /dev/vdc /media/scratch2 <- memory leak hit
>>>
>>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
>>
>> This one fixes:
>>
>> 4da82a9e60a1 btrfs: add btrfs_mount_root() and new file_system_type
>>
>> Which seems to be in for-next only.
> 
> The patch doesn't change the logic here (same as current btrfs_mount()).
> 
>>
>>> ---
>>>  fs/btrfs/super.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index e57eb6e70278..ea3bca85be44 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -1612,7 +1612,11 @@ static struct dentry *btrfs_mount_root(struct 
>>> file_system_type *fs_type,
>>> goto error_sec_opts;
>>> }
>>>  
>>> -   fs_info = btrfs_sb(s);
>>> +   if (btrfs_sb(s) != fs_info) {
>>> +   free_fs_info(fs_info);
>>> +   fs_info = btrfs_sb(s);
>>> +   }
>>> +
>>> error = setup_security_options(fs_info, s, _sec_opts);
>>> if (error) {
>>> deactivate_locked_super(s);
>>>
> 
> I'm not sure how the memory leak happens.
> Just above this line is:
> 
> ---
>  if (s->s_root) {
>btrfs_close_devices(fs_devices);
>free_fs_info(fs_info);
>    if ((flags ^ s->s_flags) & SB_RDONLY)
>  error = -EBUSY;
>  } else {
>snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
>btrfs_sb(s)->bdev_holder = fs_type;
>error = btrfs_fill_super(s, fs_devices, data);
>  }
> ---
> 
> In the first mount, s->s_root is null and btrfs_fill_super() is called.
> On the other hand, in the second mount, s->s_root is not null and fs_info is 
> freed there.
> 
> So, I think there is no memory leak in the current code, or am I missing 
> something? 

Hm, you are right, I've missed that, so disregard this patch.
> 
> Regards,
> Tomohiro
> 
>> --
>> 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
>>
> 
> 
--
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


Re: [PATCH] btrfs: Fix memory leak on multiple mounts

2017-12-18 Thread Misono, Tomohiro
Hello, 

On 2017/12/18 19:06, Nikolay Borisov wrote:
> 
> 
> On 18.12.2017 12:03, Nikolay Borisov wrote:
>> Currently if a mounted-btrfs instance is mounted for the 2nd time
>> without first unmounting the first instance then we hit a memory leak
>> in btrfs_mount_root due to the fs_info of the acquired superblock is
>> different than the newly allocated fs info. Fix this by specifically
>> checking if the fs_info instance of the newly acquired superblock is
>> the same as ours and free it if not.
>>
>> Reproducer:
>>
>> mount /dev/vdc /media/scratch
>> mount /dev/vdc /media/scratch2 <- memory leak hit
>>
>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
> 
> This one fixes:
> 
> 4da82a9e60a1 btrfs: add btrfs_mount_root() and new file_system_type
> 
> Which seems to be in for-next only.

The patch doesn't change the logic here (same as current btrfs_mount()).

> 
>> ---
>>  fs/btrfs/super.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index e57eb6e70278..ea3bca85be44 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -1612,7 +1612,11 @@ static struct dentry *btrfs_mount_root(struct 
>> file_system_type *fs_type,
>>  goto error_sec_opts;
>>  }
>>  
>> -fs_info = btrfs_sb(s);
>> +if (btrfs_sb(s) != fs_info) {
>> +free_fs_info(fs_info);
>> +fs_info = btrfs_sb(s);
>> +}
>> +
>>  error = setup_security_options(fs_info, s, _sec_opts);
>>  if (error) {
>>  deactivate_locked_super(s);
>>

I'm not sure how the memory leak happens.
Just above this line is:

---
 if (s->s_root) {
   btrfs_close_devices(fs_devices);
   free_fs_info(fs_info);
   if ((flags ^ s->s_flags) & SB_RDONLY)
 error = -EBUSY;
 } else {
   snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
   btrfs_sb(s)->bdev_holder = fs_type;
   error = btrfs_fill_super(s, fs_devices, data);
 }
---

In the first mount, s->s_root is null and btrfs_fill_super() is called.
On the other hand, in the second mount, s->s_root is not null and fs_info is 
freed there.

So, I think there is no memory leak in the current code, or am I missing 
something? 

Regards,
Tomohiro

> --
> 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
> 

--
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


Re: [PATCH] btrfs: Fix memory leak on multiple mounts

2017-12-18 Thread Nikolay Borisov


On 18.12.2017 12:03, Nikolay Borisov wrote:
> Currently if a mounted-btrfs instance is mounted for the 2nd time
> without first unmounting the first instance then we hit a memory leak
> in btrfs_mount_root due to the fs_info of the acquired superblock is
> different than the newly allocated fs info. Fix this by specifically
> checking if the fs_info instance of the newly acquired superblock is
> the same as ours and free it if not.
> 
> Reproducer:
> 
> mount /dev/vdc /media/scratch
> mount /dev/vdc /media/scratch2 <- memory leak hit
> 
> Signed-off-by: Nikolay Borisov <nbori...@suse.com>

This one fixes:

4da82a9e60a1 btrfs: add btrfs_mount_root() and new file_system_type

Which seems to be in for-next only.

> ---
>  fs/btrfs/super.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index e57eb6e70278..ea3bca85be44 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1612,7 +1612,11 @@ static struct dentry *btrfs_mount_root(struct 
> file_system_type *fs_type,
>   goto error_sec_opts;
>   }
>  
> - fs_info = btrfs_sb(s);
> + if (btrfs_sb(s) != fs_info) {
> + free_fs_info(fs_info);
> + fs_info = btrfs_sb(s);
> + }
> +
>   error = setup_security_options(fs_info, s, _sec_opts);
>   if (error) {
>   deactivate_locked_super(s);
> 
--
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


[PATCH] btrfs: Fix memory leak on multiple mounts

2017-12-18 Thread Nikolay Borisov
Currently if a mounted-btrfs instance is mounted for the 2nd time
without first unmounting the first instance then we hit a memory leak
in btrfs_mount_root due to the fs_info of the acquired superblock is
different than the newly allocated fs info. Fix this by specifically
checking if the fs_info instance of the newly acquired superblock is
the same as ours and free it if not.

Reproducer:

mount /dev/vdc /media/scratch
mount /dev/vdc /media/scratch2 <- memory leak hit

Signed-off-by: Nikolay Borisov <nbori...@suse.com>
---
 fs/btrfs/super.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index e57eb6e70278..ea3bca85be44 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1612,7 +1612,11 @@ static struct dentry *btrfs_mount_root(struct 
file_system_type *fs_type,
goto error_sec_opts;
}
 
-   fs_info = btrfs_sb(s);
+   if (btrfs_sb(s) != fs_info) {
+   free_fs_info(fs_info);
+   fs_info = btrfs_sb(s);
+   }
+
error = setup_security_options(fs_info, s, _sec_opts);
if (error) {
deactivate_locked_super(s);
-- 
2.7.4

--
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


RE: [PATCH] btrfs-progs: Resolve memory-leak in btrfs qgroup show when quota disabled

2017-09-28 Thread Gu, Jinxiang
Hi,
I am so sorry for that this patch brings a new problem of double free in normal 
scenes.
So I send a new patch .
Please revert this one and use the new patch.


> -Original Message-
> From: David Sterba [mailto:dste...@suse.cz]
> Sent: Tuesday, September 26, 2017 7:28 PM
> To: Gu, Jinxiang/顾 金香 <g...@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH] btrfs-progs: Resolve memory-leak in btrfs qgroup show 
> when quota disabled
> 
> On Tue, Sep 26, 2017 at 05:28:31PM +0800, Gu Jinxiang wrote:
> > When quota disabled, btrfs qgroup show exit with a error message, but
> > the allocated memory is not freed.
> >
> > By the way, this bug marked as issue#20 in github.
> >
> > Signed-off-by: Gu Jinxiang <g...@cn.fujitsu.com>
> 
> Applied, thanks.
> 



N�Р骒r��yb�X�肚�v�^�)藓{.n�+�伐�{�n谶�)��骅w*jg�报�茛j/�赇z罐���2���ㄨ��&�)摺�a囤���G���h��j:+v���w��佶

[PATCH] btrfs-progs: Resolve memory-leak in btrfs qgroup show when quota disabled

2017-09-28 Thread Gu Jinxiang
From: Gu JinXiang 

When quota disabled, btrfs qgroup show exit with a error message,
but the allocated memory is not freed.
By the way, this bug marked as issue#20 in github.

Signed-off-by: Gu JinXiang 
---
 cmds-qgroup.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 38382ea9..51d4f3a1 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -369,9 +369,8 @@ static int cmd_qgroup_show(int argc, char **argv)
path = argv[optind];
fd = btrfs_open_dir(path, , 1);
if (fd < 0) {
-   free(filter_set);
-   free(comparer_set);
-   return 1;
+   ret = 1;
+   goto out;
}
 
if (sync) {
@@ -399,13 +398,21 @@ static int cmd_qgroup_show(int argc, char **argv)
qgroupid);
}
ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
-   if (ret == -ENOENT)
+   if (ret == -ENOENT) {
error("can't list qgroups: quotas not enabled");
-   else if (ret < 0)
+   goto out;
+   } else if (ret < 0) {
error("can't list qgroups: %s", strerror(-ret));
+   goto out;
+   }
close_file_or_dir(fd, dirstream);
 
+   return !!ret;
+
 out:
+   close_file_or_dir(fd, dirstream);
+   free(filter_set);
+   free(comparer_set);
return !!ret;
 }
 
-- 
2.14.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


Re: [PATCH] btrfs-progs: Resolve memory-leak in btrfs qgroup show when quota disabled

2017-09-26 Thread David Sterba
On Tue, Sep 26, 2017 at 05:28:31PM +0800, Gu Jinxiang wrote:
> When quota disabled, btrfs qgroup show exit with a error message,
> but the allocated memory is not freed.
> 
> By the way, this bug marked as issue#20 in github.
> 
> Signed-off-by: Gu Jinxiang 

Applied, thanks.
--
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


[PATCH] btrfs-progs: Resolve memory-leak in btrfs qgroup show when quota disabled

2017-09-26 Thread Gu Jinxiang
When quota disabled, btrfs qgroup show exit with a error message,
but the allocated memory is not freed.

By the way, this bug marked as issue#20 in github.

Signed-off-by: Gu Jinxiang 
---
 cmds-qgroup.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 38382ea9..5fbfaa17 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -369,9 +369,8 @@ static int cmd_qgroup_show(int argc, char **argv)
path = argv[optind];
fd = btrfs_open_dir(path, , 1);
if (fd < 0) {
-   free(filter_set);
-   free(comparer_set);
-   return 1;
+   ret = 1;
+   goto out;
}
 
if (sync) {
@@ -406,6 +405,10 @@ static int cmd_qgroup_show(int argc, char **argv)
close_file_or_dir(fd, dirstream);
 
 out:
+   if (filter_set)
+   free(filter_set);
+   if (comparer_set)
+   free(comparer_set);
return !!ret;
 }
 
-- 
2.14.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


Re: [PATCH] Btrfs: fix memory leak in raid56

2017-09-24 Thread David Sterba
On Fri, Sep 22, 2017 at 12:11:18PM -0600, Liu Bo wrote:
> The local bio_list may have pending bios when doing cleanup, it can
> end up with memory leak if they don't get free'd.

I was wondering if we could make a common helper that would call
rbio_orig_end_io and while (..) put_bio(), but __raid56_parity_recover
does not fit the pattern. Still might be worth, but the patch is ok as
is.

Reviewed-by: David Sterba <dste...@suse.com>
--
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


[PATCH] Btrfs: fix memory leak in raid56

2017-09-22 Thread Liu Bo
The local bio_list may have pending bios when doing cleanup, it can
end up with memory leak if they don't get free'd.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/raid56.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 2cf6ba4..063a2a0 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1325,6 +1325,9 @@ static noinline void finish_rmw(struct btrfs_raid_bio 
*rbio)
 
 cleanup:
rbio_orig_end_io(rbio, BLK_STS_IOERR);
+
+   while ((bio = bio_list_pop(_list)))
+   bio_put(bio);
 }
 
 /*
@@ -1580,6 +1583,10 @@ static int raid56_rmw_stripe(struct btrfs_raid_bio *rbio)
 
 cleanup:
rbio_orig_end_io(rbio, BLK_STS_IOERR);
+
+   while ((bio = bio_list_pop(_list)))
+   bio_put(bio);
+
return -EIO;
 
 finish:
@@ -2105,6 +2112,10 @@ static int __raid56_parity_recover(struct btrfs_raid_bio 
*rbio)
if (rbio->operation == BTRFS_RBIO_READ_REBUILD ||
rbio->operation == BTRFS_RBIO_REBUILD_MISSING)
rbio_orig_end_io(rbio, BLK_STS_IOERR);
+
+   while ((bio = bio_list_pop(_list)))
+   bio_put(bio);
+
return -EIO;
 }
 
@@ -2452,6 +2463,9 @@ static noinline void finish_parity_scrub(struct 
btrfs_raid_bio *rbio,
 
 cleanup:
rbio_orig_end_io(rbio, BLK_STS_IOERR);
+
+   while ((bio = bio_list_pop(_list)))
+   bio_put(bio);
 }
 
 static inline int is_data_stripe(struct btrfs_raid_bio *rbio, int stripe)
@@ -2561,12 +2575,12 @@ static void raid56_parity_scrub_stripe(struct 
btrfs_raid_bio *rbio)
int stripe;
struct bio *bio;
 
+   bio_list_init(_list);
+
ret = alloc_rbio_essential_pages(rbio);
if (ret)
goto cleanup;
 
-   bio_list_init(_list);
-
atomic_set(>error, 0);
/*
 * build a list of bios to read all the missing parts of this
@@ -2634,6 +2648,10 @@ static void raid56_parity_scrub_stripe(struct 
btrfs_raid_bio *rbio)
 
 cleanup:
rbio_orig_end_io(rbio, BLK_STS_IOERR);
+
+   while ((bio = bio_list_pop(_list)))
+   bio_put(bio);
+
return;
 
 finish:
-- 
2.9.4

--
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


Re: [PATCH] btrfs: tests: Fix a memory leak in error handling path in 'run_test()'

2017-09-10 Thread Qu Wenruo



On 2017年09月10日 19:19, Christophe JAILLET wrote:

If 'btrfs_alloc_path()' fails, we must free the resourses already
allocated, as done in the other error handling paths in this function.

Signed-off-by: Christophe JAILLET 


Reviewed-by: Qu Wenruo 

BTW, I also checked all btrfs_alloc_path() in self tests, not such leak 
remaining.


Thanks,
Qu

---
  fs/btrfs/tests/free-space-tree-tests.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tests/free-space-tree-tests.c 
b/fs/btrfs/tests/free-space-tree-tests.c
index 1458bb0ea124..8444a018cca2 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -500,7 +500,8 @@ static int run_test(test_func_t test_func, int bitmaps, u32 
sectorsize,
path = btrfs_alloc_path();
if (!path) {
test_msg("Couldn't allocate path\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out;
}
  
  	ret = add_block_group_free_space(, root->fs_info, cache);



--
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


[PATCH] btrfs: tests: Fix a memory leak in error handling path in 'run_test()'

2017-09-10 Thread Christophe JAILLET
If 'btrfs_alloc_path()' fails, we must free the resourses already
allocated, as done in the other error handling paths in this function.

Signed-off-by: Christophe JAILLET 
---
 fs/btrfs/tests/free-space-tree-tests.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tests/free-space-tree-tests.c 
b/fs/btrfs/tests/free-space-tree-tests.c
index 1458bb0ea124..8444a018cca2 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -500,7 +500,8 @@ static int run_test(test_func_t test_func, int bitmaps, u32 
sectorsize,
path = btrfs_alloc_path();
if (!path) {
test_msg("Couldn't allocate path\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out;
}
 
ret = add_block_group_free_space(, root->fs_info, cache);
-- 
2.11.0

--
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


Re: [PATCH] btrfs: fix memory leak in update_space_info failure path

2017-05-18 Thread Liu Bo
On Wed, May 17, 2017 at 09:49:37AM -0400, je...@suse.com wrote:
> From: Jeff Mahoney 
> 
> If we fail to add the space_info kobject, we'll leak the memory
> for the percpu counter.
> 
> Fixes: 6ab0a2029c (btrfs: publish allocation data in sysfs)
> Cc:  # v3.14+
> Signed-off-by: Jeff Mahoney 

Looks good.

Reviewed-by: Liu Bo 

Thanks,

-liubo
--
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


[PATCH] btrfs: fix memory leak in update_space_info failure path

2017-05-17 Thread jeffm
From: Jeff Mahoney 

If we fail to add the space_info kobject, we'll leak the memory
for the percpu counter.

Fixes: 6ab0a2029c (btrfs: publish allocation data in sysfs)
Cc:  # v3.14+
Signed-off-by: Jeff Mahoney 
---
 fs/btrfs/extent-tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index be5477676cc8..2f23fd4a08f1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3983,6 +3983,7 @@ static int update_space_info(struct btrfs_fs_info *info, 
u64 flags,
info->space_info_kobj, "%s",
alloc_name(found->flags));
if (ret) {
+   percpu_counter_destroy(>total_bytes_pinned);
kfree(found);
return ret;
}
-- 
2.11.0

--
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


[PATCH] btrfs-progs: record_orphan_data_extent: Fix memory leak

2017-05-10 Thread Praveen K Pandey
The nodes of the list at btrfs_root->orphan_data_extents
are never freed causing memory to be leaked. This commit
fixes the bug by freeing the nodes on all the
btrfs_root->orphan_data_extents list.

Signed-off-by: Praveen K Pandey 
---
 cmds-check.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 0014bc2..cac94a8 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -9286,6 +9286,9 @@ static int check_extent_refs(struct btrfs_root *root,
 {
struct extent_record *rec;
struct cache_extent *cache;
+   struct btrfs_fs_info *fs_info;
+   struct btrfs_root *root_orphan_extent;
+   struct rb_node *rb_node;
int ret = 0;
int had_dups = 0;
 
@@ -9439,6 +9442,26 @@ static int check_extent_refs(struct btrfs_root *root,
   rec->start + rec->max_size - 1);
free(rec);
}
+   /*
+* Release memory of oprhan_data_extent
+* which allocated  while traversing in orphan_data_extents
+*/
+
+   fs_info = root->fs_info;
+   rb_node = rb_first(_info->fs_root_tree);
+   while (rb_node) {
+   root_orphan_extent = container_of(rb_node,
+   struct btrfs_root, rb_node);
+   
free_orphan_data_extents(_orphan_extent->orphan_data_extents);
+   rb_node  = rb_next(rb_node);
+   }
+   free_orphan_data_extents(_info->extent_root->orphan_data_extents);
+   free_orphan_data_extents(_info->tree_root->orphan_data_extents);
+   free_orphan_data_extents(_info->chunk_root->orphan_data_extents);
+   free_orphan_data_extents(_info->dev_root->orphan_data_extents);
+   free_orphan_data_extents(_info->csum_root->orphan_data_extents);
+   if (fs_info->quota_enabled)
+   
free_orphan_data_extents(_info->quota_root->orphan_data_extents);
 repair_abort:
if (repair) {
if (ret && ret != -EAGAIN) {
-- 
2.9.3

--
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


Re: [PATCH] btrfs-progs: check: Fix memory leak in check_chunks_and_extents

2017-05-02 Thread David Sterba
On Tue, May 02, 2017 at 11:17:25AM +0800, Qu Wenruo wrote:
> fsck/003-shift-offsets makes valgrinds complaining about memory leaks.
> ==5910==
> ==5910== HEAP SUMMARY:
> ==5910== in use at exit: 1,112 bytes in 11 blocks
> ==5910==   total heap usage: 161 allocs, 150 frees, 164,800 bytes allocated
> ==5910==
> ==5910== 216 (72 direct, 144 indirect) bytes in 1 blocks are definitely lost 
> in loss record 3 of 5
> ==5910==at 0x4C2AF1F: malloc (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5910==by 0x4815A3: add_root_item_to_list (cmds-check.c:9683)
> ==5910==by 0x481CE2: check_chunks_and_extents (cmds-check.c:9886)
> ==5910==by 0x4B: cmd_check (cmds-check.c:12977)
> ==5910==by 0x40A8C5: main (btrfs.c:246)
> ==5910==
> 
> The check_chunks_and_extents() memory leaks are caused by not freeing
> added root items of normal_trees and dropping_trees.
> 
> Signed-off-by: Qu Wenruo 

Applied, thanks.
--
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


Re: [PATCH] btrfs-progs: tests: fssum, fix memory leak

2017-05-02 Thread David Sterba
On Wed, Apr 26, 2017 at 09:22:41AM +0800, Lu Fengqi wrote:
> Free the alloced memory and close dir before exit.
> 
> Signed-off-by: Lu Fengqi 

Applied, thanks.
--
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


Re: [PATCH] btrfs-progs: Fix memory leak when 0 sized block group item is found

2017-05-02 Thread David Sterba
On Tue, Apr 25, 2017 at 04:01:17PM +0800, Qu Wenruo wrote:
> When a 0 sized block group item is found, set_extent_bits() will not
> really set any bits.
> While set_state_private() still inserts allocated block group cache into
> block group extent_io_tree.
> 
> So at close_ctree() time, we won't free the private block group cache
> stored since we can't find any bit set for the 0 sized block group.
> 
> To fix it, at btrfs_read_block_groups() we skip any 0 sized block group,
> so such leak won't happen.
> 
> Signed-off-by: Qu Wenruo 

Applied, thanks.
--
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


[PATCH] btrfs-progs: check: Fix memory leak in check_chunks_and_extents

2017-05-01 Thread Qu Wenruo
fsck/003-shift-offsets makes valgrinds complaining about memory leaks.
==5910==
==5910== HEAP SUMMARY:
==5910== in use at exit: 1,112 bytes in 11 blocks
==5910==   total heap usage: 161 allocs, 150 frees, 164,800 bytes allocated
==5910==
==5910== 216 (72 direct, 144 indirect) bytes in 1 blocks are definitely lost in 
loss record 3 of 5
==5910==at 0x4C2AF1F: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5910==by 0x4815A3: add_root_item_to_list (cmds-check.c:9683)
==5910==by 0x481CE2: check_chunks_and_extents (cmds-check.c:9886)
==5910==by 0x4B: cmd_check (cmds-check.c:12977)
==5910==by 0x40A8C5: main (btrfs.c:246)
==5910==

The check_chunks_and_extents() memory leaks are caused by not freeing
added root items of normal_trees and dropping_trees.

Signed-off-by: Qu Wenruo 
---
 cmds-check.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 17b7efbf..897b1587 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -9972,6 +9972,8 @@ out:
free_extent_cache_tree();
free_extent_cache_tree();
free_extent_cache_tree();
+   free_root_item_list(_trees);
+   free_root_item_list(_trees);
return ret;
 loop:
free_corrupt_blocks_tree(root->fs_info->corrupt_blocks);
-- 
2.12.2



--
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


[PATCH] btrfs-progs: tests: fssum, fix memory leak

2017-04-25 Thread Lu Fengqi
Free the alloced memory and close dir before exit.

Signed-off-by: Lu Fengqi 
---
 tests/fssum.c | 35 ++-
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/tests/fssum.c b/tests/fssum.c
index 83bd4106..8be44547 100644
--- a/tests/fssum.c
+++ b/tests/fssum.c
@@ -420,6 +420,9 @@ check_manifest(char *fn, char *m, char *c, int last_call)
while ((l = getln(line, sizeof(line), in_fp))) {
rem_c = strrchr(l, ' ');
if (!rem_c) {
+   if (checksum)
+   free(checksum);
+
/* final cs */
checksum = strdup(l);
break;
@@ -503,6 +506,7 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, 
char *path_in)
}
++entries;
}
+
qsort(namelist, entries, sizeof(*namelist), namecmp);
for (i = 0; i < entries; ++i) {
struct stat64 st;
@@ -624,7 +628,11 @@ sum(int dirfd, int level, sum_t *dircs, char *path_prefix, 
char *path_in)
sum_add_sum(dircs, );
 next:
free(path);
+   free(namelist[i]);
}
+
+   free(namelist);
+   closedir(d);
 }
 
 int
@@ -636,7 +644,9 @@ main(int argc, char *argv[])
char *path;
int fd;
sum_t cs;
+   char *sumstring;
char flagstring[sizeof(flchar)];
+   int ret = 0;
int i;
int plen;
int elen;
@@ -736,6 +746,9 @@ main(int argc, char *argv[])
} else if ((p = strchr(l, ':'))) {
*p++ = 0;
parse_flags(l);
+
+   if (checksum)
+   free(checksum);
checksum = strdup(p);
} else {
fprintf(stderr, "invalid input file format\n");
@@ -798,16 +811,28 @@ main(int argc, char *argv[])
if (!gen_manifest)
fprintf(out_fp, "%s:", flagstring);
 
-   fprintf(out_fp, "%s\n", sum_to_string());
+   sumstring = sum_to_string();
+   fprintf(out_fp, "%s\n", sumstring);
+   free(sumstring);
} else {
-   if (strcmp(checksum, sum_to_string()) == 0) {
+   sumstring = sum_to_string();
+   if (strcmp(checksum, sumstring) == 0) {
printf("OK\n");
-   exit(0);
+   ret = 0;
} else {
printf("FAIL\n");
-   exit(1);
+   ret = 1;
}
+
+   free(checksum);
+   free(sumstring);
}
 
-   exit(0);
+   if (in_fp)
+   fclose(in_fp);
+
+   if (out_fp != stdout)
+   fclose(out_fp);
+
+   exit(ret);
 }
-- 
2.12.2



--
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


[PATCH] btrfs-progs: Fix memory leak when 0 sized block group item is found

2017-04-25 Thread Qu Wenruo
When a 0 sized block group item is found, set_extent_bits() will not
really set any bits.
While set_state_private() still inserts allocated block group cache into
block group extent_io_tree.

So at close_ctree() time, we won't free the private block group cache
stored since we can't find any bit set for the 0 sized block group.

To fix it, at btrfs_read_block_groups() we skip any 0 sized block group,
so such leak won't happen.

Signed-off-by: Qu Wenruo 
---
 extent-tree.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/extent-tree.c b/extent-tree.c
index 3e32e43b..b12ee290 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -3250,6 +3250,7 @@ int btrfs_read_block_groups(struct btrfs_root *root)
}
leaf = path->nodes[0];
btrfs_item_key_to_cpu(leaf, _key, path->slots[0]);
+
cache = kzalloc(sizeof(*cache), GFP_NOFS);
if (!cache) {
ret = -ENOMEM;
@@ -3266,6 +3267,17 @@ int btrfs_read_block_groups(struct btrfs_root *root)
if (found_key.offset == 0)
key.objectid++;
btrfs_release_path(path);
+
+   /*
+* Skip 0 sized block group, don't insert them into block
+* group cache tree, as its length is 0, it won't get
+* freed at close_ctree() time.
+*/
+   if (found_key.offset == 0) {
+   free(cache);
+   continue;
+   }
+
cache->flags = btrfs_block_group_flags(>item);
bit = 0;
if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
-- 
2.12.2



--
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


Re: [PATCH 1/4] btrfs-progs: Fix memory leak in write_raid56_with_parity

2016-10-25 Thread David Sterba
On Mon, Oct 24, 2016 at 10:43:32AM +0800, Qu Wenruo wrote:
> Ebs and pointers are allocated, but if any of the allocation failed, we
> should free the allocated memory.
> 
> Reported-by: David Sterba 
> Resolves-Coverity-CID: 1296749
> Signed-off-by: Qu Wenruo 

1-4 applied, thanks. Please don't put my reported-by there.
--
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


Re: [PATCH 1/4] btrfs-progs: Fix memory leak in write_raid56_with_parity

2016-10-23 Thread Qu Wenruo



At 10/24/2016 10:43 AM, Qu Wenruo wrote:

Ebs and pointers are allocated, but if any of the allocation failed, we
should free the allocated memory.

Reported-by: David Sterba 
Resolves-Coverity-CID: 1296749


Sorry, wrong CID here,
Correct ones are:
Resolves-Coverity-CID: 1374101
Resolves-Coverity-CID: 1374100



Signed-off-by: Qu Wenruo 
---
 volumes.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index a7abd92..f687b0d 100644
--- a/volumes.c
+++ b/volumes.c
@@ -2120,8 +2120,11 @@ int write_raid56_with_parity(struct btrfs_fs_info *info,

ebs = malloc(sizeof(*ebs) * multi->num_stripes);
pointers = malloc(sizeof(*pointers) * multi->num_stripes);
-   if (!ebs || !pointers)
+   if (!ebs || !pointers) {
+   free(ebs);
+   free(pointers);
return -ENOMEM;
+   }

if (stripe_len > alloc_size)
alloc_size = stripe_len;




--
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


[PATCH 1/4] btrfs-progs: Fix memory leak in write_raid56_with_parity

2016-10-23 Thread Qu Wenruo
Ebs and pointers are allocated, but if any of the allocation failed, we
should free the allocated memory.

Reported-by: David Sterba 
Resolves-Coverity-CID: 1296749
Signed-off-by: Qu Wenruo 
---
 volumes.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index a7abd92..f687b0d 100644
--- a/volumes.c
+++ b/volumes.c
@@ -2120,8 +2120,11 @@ int write_raid56_with_parity(struct btrfs_fs_info *info,
 
ebs = malloc(sizeof(*ebs) * multi->num_stripes);
pointers = malloc(sizeof(*pointers) * multi->num_stripes);
-   if (!ebs || !pointers)
+   if (!ebs || !pointers) {
+   free(ebs);
+   free(pointers);
return -ENOMEM;
+   }
 
if (stripe_len > alloc_size)
alloc_size = stripe_len;
-- 
2.10.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


Re: [PATCH] Btrfs: fix memory leak in do_walk_down

2016-09-14 Thread Holger Hoffstätte
On 09/14/16 04:02, Liu Bo wrote:
> The extent buffer 'next' needs to be free'd conditionally.
> 
> Signed-off-by: Liu Bo 
> ---
>  fs/btrfs/extent-tree.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 5a940ab..779fd72 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -8882,6 +8882,7 @@ static noinline int do_walk_down(struct 
> btrfs_trans_handle *trans,
>  >flags[level - 1]);
>   if (ret < 0) {
>   btrfs_tree_unlock(next);
> + free_extent_buffer(next);
>   return ret;
>   }
>  
> 

This was fixed a long time ago by the following patch by Josef:

"Btrfs: don't BUG() during drop snapshot"
https://patchwork.kernel.org/patch/7002791/

..which never got merged. I've been using it since 4.1.x until today
without problems.

Note that apparently Patchwork got confused here: downloading only the
"patch" fragment won't work; you'll need the full "mbox" attachment.
It also seems to have been sorted into the wrong list bucket for some
reason.

-h

--
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


[PATCH] Btrfs: fix memory leak in do_walk_down

2016-09-13 Thread Liu Bo
The extent buffer 'next' needs to be free'd conditionally.

Signed-off-by: Liu Bo 
---
 fs/btrfs/extent-tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 5a940ab..779fd72 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -8882,6 +8882,7 @@ static noinline int do_walk_down(struct 
btrfs_trans_handle *trans,
   >flags[level - 1]);
if (ret < 0) {
btrfs_tree_unlock(next);
+   free_extent_buffer(next);
return ret;
}
 
-- 
2.5.5

--
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


Re: [PATCH 7/7] Btrfs: fix memory leak due to invalid btree height

2016-09-06 Thread Liu Bo
On Tue, Sep 06, 2016 at 06:50:19PM +0200, David Sterba wrote:
> On Fri, May 13, 2016 at 05:07:02PM -0700, Liu Bo wrote:
> > Thanks to fuzz testing, we can have invalid btree root node height.
> 
> Shouldn't we do this kind of sanity checks earlier? Not at the search
> slot time but when it's read from disk. The check that you're adding can
> stay, but without the early check we could hit it very often thus making
> it very noisy.

We do have such an early check when it's read from disk
(btree_readpage_end_io_hook) and this can protect us from 99.9% cases,
the only corner case is that the fuzz image changes our chunk root node
to superblock bytenr, so we firstly reads superblock into a dummy eb, and when
we get to read chunk root, we firstly search eb tree and find one eb
matching the bytenr, then we take this invalid eb to do
btrfs_search_slot() and we come cross this surprise.

Anyway, this patch was made before I found we could actually free
superblock's eb immediately after use.  Now with freeing that eb I don't
think we can have the above problem.

Thanks,

-liubo

> 
> > Btrfs limits btree height to 7 and if the given height is 9, then btrfs
> > will have problems in both releasing root node's lock and freeing the node.
> 
> 
> > 
> > Signed-off-by: Liu Bo 
> > ---
> >  fs/btrfs/ctree.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index ec7928a..3fccbcc 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -2756,6 +2756,13 @@ again:
> > }
> > }
> > }
> > +   if (level > BTRFS_MAX_LEVEL - 1 || level < 0) {
> > +   WARN_ONCE(1, KERN_WARNING "Invalid btree height %d\n", level);
> > +   if (!p->skip_locking)
> > +   btrfs_tree_unlock_rw(b, root_lock);
> > +   free_extent_buffer(b);
> > +   return -EINVAL;
> > +   }
> > p->nodes[level] = b;
> > if (!p->skip_locking)
> > p->locks[level] = root_lock;
> > -- 
> > 2.5.5
> > 
> > --
> > 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
--
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


Re: [PATCH 7/7] Btrfs: fix memory leak due to invalid btree height

2016-09-06 Thread David Sterba
On Fri, May 13, 2016 at 05:07:02PM -0700, Liu Bo wrote:
> Thanks to fuzz testing, we can have invalid btree root node height.

Shouldn't we do this kind of sanity checks earlier? Not at the search
slot time but when it's read from disk. The check that you're adding can
stay, but without the early check we could hit it very often thus making
it very noisy.

> Btrfs limits btree height to 7 and if the given height is 9, then btrfs
> will have problems in both releasing root node's lock and freeing the node.


> 
> Signed-off-by: Liu Bo 
> ---
>  fs/btrfs/ctree.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index ec7928a..3fccbcc 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2756,6 +2756,13 @@ again:
>   }
>   }
>   }
> + if (level > BTRFS_MAX_LEVEL - 1 || level < 0) {
> + WARN_ONCE(1, KERN_WARNING "Invalid btree height %d\n", level);
> + if (!p->skip_locking)
> + btrfs_tree_unlock_rw(b, root_lock);
> + free_extent_buffer(b);
> + return -EINVAL;
> + }
>   p->nodes[level] = b;
>   if (!p->skip_locking)
>   p->locks[level] = root_lock;
> -- 
> 2.5.5
> 
> --
> 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
--
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


Re: [PATCH] Btrfs: fix memory leak in reading btree blocks

2016-08-24 Thread Liu Bo
Hi David,

On Wed, Aug 03, 2016 at 12:33:01PM -0700, Liu Bo wrote:
> So we can read a btree block via readahead or intentional read,
> and we can end up with a memory leak when something happens as
> follows,
> 1) readahead starts to read block A but does not wait for read
>completion,
> 2) btree_readpage_end_io_hook finds that block A is corrupted,
>and it needs to clear all block A's pages' uptodate bit.
> 3) meanwhile an intentional read kicks in and checks block A's
>pages' uptodate to decide which page needs to be read.
> 4) when some pages have the uptodate bit during 3)'s check so
>3) doesn't count them for eb->io_pages, but they are later
>cleared by 2) so we has to readpage on the page, we get
>the wrong eb->io_pages which results in a memory leak of
>this block.
> 
> This fixes the problem by firstly getting all pages's locking and
> then checking pages' uptodate bit.

   t1(readahead)  t2(readahead endio)   
t3(the following read)
read_extent_buffer_pagesend_bio_extent_readpage 
 
  for pg in eb:for page 0,1,2 in eb:
  if pg is uptodate:   
btree_readpage_end_io_hook(pg)
  num_reads++  if uptodate: 
   
  eb->io_pages = num_reads SetPageUptodate(pg)  
___
  for pg in eb:for page 3 in eb:
 read_extent_buffer_pages   
   if pg is NOT uptodate:  
btree_readpage_end_io_hook(pg)   for pg in eb:
   __extent_read_full_page(pg) sanity check reports 
something wrong if pg is uptodate:
   
clear_extent_buffer_uptodate(eb) num_reads++
   for pg in eb:
eb->io_pages = num_reads
   
ClearPageUptodate(page)  ___

for pg in eb:

if pg is NOT uptodate:

__extent_read_full_page(pg)

So t3's eb->io_pages is not consistent with the number of pages it's reading, 
and during endio(), atomic_dec_and_test(>io_pages) will get a negative 
number so that we're not able to free the eb.

Thanks,

-liubo

> 
> Signed-off-by: Liu Bo <bo.li@oracle.com>
> ---
>  fs/btrfs/extent_io.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index bd29b9b..a77050e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5215,11 +5215,20 @@ int read_extent_buffer_pages(struct extent_io_tree 
> *tree,
>   lock_page(page);
>   }
>   locked_pages++;
> + }
> + /*
> +  * We need to firstly lock all pages to make sure that
> +  * the uptodate bit of our pages won't be affected by
> +  * clear_extent_buffer_uptodate().
> +  */
> + for (i = start_i; i < num_pages; i++) {
> + page = eb->pages[i];
>   if (!PageUptodate(page)) {
>   num_reads++;
>   all_uptodate = 0;
>   }
>   }
> +
>   if (all_uptodate) {
>   if (start_i == 0)
>   set_bit(EXTENT_BUFFER_UPTODATE, >bflags);
> -- 
> 2.5.5
> 
> --
> 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
--
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


[PATCH v3] Btrfs: fix memory leak of block group cache

2016-08-23 Thread Liu Bo
While processing delayed refs, we may update block group's statistics
and attach it to cur_trans->dirty_bgs, and later writing dirty block
groups will process the list, which happens during
btrfs_commit_transaction().

For whatever reason, the transaction is aborted and dirty_bgs
is not processed in cleanup_transaction(), we end up with memory leak
of these dirty block group cache.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
v2: - remove the 'return' when dirty_bgs is empty because there
  might be block group in list io_bgs.
- remove unnecessary '\n' in btrfs_err().
v3: - remove cleanup in btrfs_start_dirty_block_groups() because a later
  cleanup_transaction() can do the cleanup work, and this also
  addresses a problem when others holding refs for bg's cache inode.

 fs/btrfs/disk-io.c | 71 ++
 1 file changed, 71 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9d7d78e..bb2ad14 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4461,9 +4461,80 @@ again:
return 0;
 }
 
+static void btrfs_cleanup_bg_io(struct btrfs_block_group_cache *cache)
+{
+   struct inode *inode;
+
+   inode = cache->io_ctl.inode;
+   if (inode) {
+   invalidate_inode_pages2(inode->i_mapping);
+   BTRFS_I(inode)->generation = 0;
+   cache->io_ctl.inode = NULL;
+   iput(inode);
+   }
+   btrfs_put_block_group(cache);
+}
+
+static void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
+   struct btrfs_root *root)
+{
+   struct btrfs_block_group_cache *cache;
+
+   spin_lock(_trans->dirty_bgs_lock);
+   while (!list_empty(_trans->dirty_bgs)) {
+   cache = list_first_entry(_trans->dirty_bgs,
+struct btrfs_block_group_cache,
+dirty_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group dirty_bgs list");
+   spin_unlock(_trans->dirty_bgs_lock);
+   return;
+   }
+
+   if (!list_empty(>io_list)) {
+   spin_unlock(_trans->dirty_bgs_lock);
+   list_del_init(>io_list);
+   btrfs_cleanup_bg_io(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+
+   list_del_init(>dirty_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+
+   spin_unlock(_trans->dirty_bgs_lock);
+   btrfs_put_block_group(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+   spin_unlock(_trans->dirty_bgs_lock);
+
+   while (!list_empty(_trans->io_bgs)) {
+   cache = list_first_entry(_trans->io_bgs,
+struct btrfs_block_group_cache,
+io_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group on io_bgs list");
+   return;
+   }
+
+   list_del_init(>io_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+   btrfs_cleanup_bg_io(cache);
+   }
+}
+
 void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
   struct btrfs_root *root)
 {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
+   ASSERT(list_empty(_trans->dirty_bgs));
+   ASSERT(list_empty(_trans->io_bgs));
+
btrfs_destroy_delayed_refs(cur_trans, root);
 
cur_trans->state = TRANS_STATE_COMMIT_START;
-- 
2.5.5

--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-08-23 Thread Liu Bo
Hi David,

On Fri, Aug 19, 2016 at 01:28:23PM +0200, David Sterba wrote:
> On Thu, Jul 21, 2016 at 02:33:19PM -0700, Liu Bo wrote:
> > > > update_block_group() is the only producer to add block group cache to
> > > > dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
> > > > is aborted, so seems that there won't be anyone manipulating dirty_bgs
> > > > list, am I missing?
> > > > 
> > > 
> > > No, the dirty_bgs processing is safe I think.  My concern is with the 
> > > cache
> > > inode which we iput()
> > 
> > I think iput() is OK, we're doing iput() on block group cache on the io_bgs
> > list, where all block groups's inodes has been igrab()'d.  If others are
> > messing around with our cache inode, they should have their own igrab,
> > too.
> > 
> > > 
> > > > Another point is that when we fail on btrfs_start_dirty_block_groups(),
> > > > btrfs_commit_transaction() won't get to cleanup_transaction error
> > > > handling,
> > > 
> > > Right, because we don't actually finish the commit.  Someone will 
> > > eventually
> > > though ;)
> > 
> > Hmm yes, it's possible that there's a concurrent commit transaction
> > running.  If that's not true, we may still resort to
> > btrfs_error_commit_super(), other than that, I don't see who could
> > commit/cleanup the transaction after entering into BTRFS_FS_STATE_ERROR
> > state.
> 
> What's the resume of this patch? I don't see a followup patch or a (to
> me) clear yes/no whether to merge it. Please let me know, thanks.

I'm going to update the patch to remove btrfs_start_dirty_block_groups()
part.

Thanks,

-liubo
--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-08-19 Thread David Sterba
On Thu, Jul 21, 2016 at 02:33:19PM -0700, Liu Bo wrote:
> > > update_block_group() is the only producer to add block group cache to
> > > dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
> > > is aborted, so seems that there won't be anyone manipulating dirty_bgs
> > > list, am I missing?
> > > 
> > 
> > No, the dirty_bgs processing is safe I think.  My concern is with the cache
> > inode which we iput()
> 
> I think iput() is OK, we're doing iput() on block group cache on the io_bgs
> list, where all block groups's inodes has been igrab()'d.  If others are
> messing around with our cache inode, they should have their own igrab,
> too.
> 
> > 
> > > Another point is that when we fail on btrfs_start_dirty_block_groups(),
> > > btrfs_commit_transaction() won't get to cleanup_transaction error
> > > handling,
> > 
> > Right, because we don't actually finish the commit.  Someone will eventually
> > though ;)
> 
> Hmm yes, it's possible that there's a concurrent commit transaction
> running.  If that's not true, we may still resort to
> btrfs_error_commit_super(), other than that, I don't see who could
> commit/cleanup the transaction after entering into BTRFS_FS_STATE_ERROR
> state.

What's the resume of this patch? I don't see a followup patch or a (to
me) clear yes/no whether to merge it. Please let me know, thanks.
--
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


[PATCH] Btrfs: fix memory leak in reading btree blocks

2016-08-03 Thread Liu Bo
So we can read a btree block via readahead or intentional read,
and we can end up with a memory leak when something happens as
follows,
1) readahead starts to read block A but does not wait for read
   completion,
2) btree_readpage_end_io_hook finds that block A is corrupted,
   and it needs to clear all block A's pages' uptodate bit.
3) meanwhile an intentional read kicks in and checks block A's
   pages' uptodate to decide which page needs to be read.
4) when some pages have the uptodate bit during 3)'s check so
   3) doesn't count them for eb->io_pages, but they are later
   cleared by 2) so we has to readpage on the page, we get
   the wrong eb->io_pages which results in a memory leak of
   this block.

This fixes the problem by firstly getting all pages's locking and
then checking pages' uptodate bit.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/extent_io.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bd29b9b..a77050e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5215,11 +5215,20 @@ int read_extent_buffer_pages(struct extent_io_tree 
*tree,
lock_page(page);
}
locked_pages++;
+   }
+   /*
+* We need to firstly lock all pages to make sure that
+* the uptodate bit of our pages won't be affected by
+* clear_extent_buffer_uptodate().
+*/
+   for (i = start_i; i < num_pages; i++) {
+   page = eb->pages[i];
if (!PageUptodate(page)) {
num_reads++;
all_uptodate = 0;
}
}
+
if (all_uptodate) {
if (start_i == 0)
set_bit(EXTENT_BUFFER_UPTODATE, >bflags);
-- 
2.5.5

--
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


Re: [PATCH] btrfs-progs: fix memory leak with missing device

2016-07-29 Thread David Sterba
On Thu, Jul 28, 2016 at 10:44:11AM -0700, Justin Maggard wrote:
> In read_one_chunk(), we may add an empty entry for a missing device.
> However, this entry wasn't being added to the dev_list, and so it never
> got freed.
> 
> Signed-off-by: Justin Maggard 

Applied (to 4.7), thanks.
--
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


[PATCH] btrfs-progs: fix memory leak with missing device

2016-07-28 Thread Justin Maggard
In read_one_chunk(), we may add an empty entry for a missing device.
However, this entry wasn't being added to the dev_list, and so it never
got freed.

Signed-off-by: Justin Maggard 
---
 volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/volumes.c b/volumes.c
index ccfa732..b5066b9 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1759,6 +1759,8 @@ static int read_one_chunk(struct btrfs_root *root, struct 
btrfs_key *key,
map->stripes[i].dev = fill_missing_device(devid);
printf("warning, device %llu is missing\n",
   (unsigned long long)devid);
+   list_add(>stripes[i].dev->dev_list,
+>fs_info->fs_devices->devices);
}
 
}
-- 
2.9.2

--
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


Re: [PATCH] Btrfs: add ASSERT for block group's memory leak

2016-07-26 Thread David Sterba
On Wed, Jul 20, 2016 at 05:33:44PM -0700, Liu Bo wrote:
> This adds several ASSERT()' s to report memory leak of block group cache.
> 
> Signed-off-by: Liu Bo <bo.li@oracle.com>

Reviewed-by: David Sterba <dste...@suse.com>
--
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


Re: [PATCH] Btrfs: fix memory leak of reloc_root

2016-07-26 Thread David Sterba
On Tue, Jul 19, 2016 at 03:36:05PM -0700, Liu Bo wrote:
> When some critical errors occur and FS would be flipped into RO,
> if we have an on-going balance, we can end up with a memory leak
> of root->reloc_root since btrfs_drop_snapshots() bails out
> without freeing reloc_root at the very early start.
> 
> However, we're not able to free reloc_root in btrfs_drop_snapshots()
> because its caller, merge_reloc_roots(), still needs to access it to
> cleanup reloc_root's rbtree.
> 
> This makes us free reloc_root when we're going to free fs/file roots.
> 
> Signed-off-by: Liu Bo <bo.li@oracle.com>

Reviewed-by: David Sterba <dste...@suse.com>
--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-07-21 Thread Liu Bo
On Thu, Jul 21, 2016 at 03:17:41PM -0400, Chris Mason wrote:
> 
> 
> On 07/21/2016 03:03 PM, Liu Bo wrote:
> > On Thu, Jul 21, 2016 at 11:32:26AM -0400, Chris Mason wrote:
> > > 
> > > 
> > > On 07/20/2016 08:44 PM, Liu Bo wrote:
> > > > While processing delayed refs, we may update block group's statistics
> > > > and attach it to cur_trans->dirty_bgs, and later writing dirty block
> > > > groups will process the list, which happens during
> > > > btrfs_commit_transaction().
> > > > 
> > > > For whatever reason, the transaction is aborted and dirty_bgs
> > > > is not processed in cleanup_transaction(), we end up with memory leak
> > > > of these dirty block group cache.
> > > > 
> > > > Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
> > > > critical section, this also adds the cleanup work inside it.
> > > 
> > > It's the start_drity_block_groups() hunt that worries me a bit:
> > > 
> > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > > index 50bd683..7a35c9d 100644
> > > > --- a/fs/btrfs/extent-tree.c
> > > > +++ b/fs/btrfs/extent-tree.c
> > > > @@ -3698,6 +3698,8 @@ again:
> > > > goto again;
> > > > }
> > > > spin_unlock(_trans->dirty_bgs_lock);
> > > > +   } else if (ret < 0) {
> > > > +   btrfs_cleanup_dirty_bgs(cur_trans, root);
> > > > }
> > > > 
> > > > btrfs_free_path(path);
> > > > 
> > > 
> > > We have checks in here to make sure only one process runs
> > > btrfs_start_dirty_block_groups() but that doesn't mean that only one 
> > > process
> > > its messing around with the cache inode.  Is there any reason we can't let
> > > this cleanup wait for the cleanup_transaction code?
> > > 
> > > btrfs_run_delayed_refs() already aborts when it fails.
> > 
> > update_block_group() is the only producer to add block group cache to
> > dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
> > is aborted, so seems that there won't be anyone manipulating dirty_bgs
> > list, am I missing?
> > 
> 
> No, the dirty_bgs processing is safe I think.  My concern is with the cache
> inode which we iput()

I think iput() is OK, we're doing iput() on block group cache on the io_bgs
list, where all block groups's inodes has been igrab()'d.  If others are
messing around with our cache inode, they should have their own igrab,
too.

> 
> > Another point is that when we fail on btrfs_start_dirty_block_groups(),
> > btrfs_commit_transaction() won't get to cleanup_transaction error
> > handling,
> 
> Right, because we don't actually finish the commit.  Someone will eventually
> though ;)

Hmm yes, it's possible that there's a concurrent commit transaction
running.  If that's not true, we may still resort to
btrfs_error_commit_super(), other than that, I don't see who could
commit/cleanup the transaction after entering into BTRFS_FS_STATE_ERROR
state.

Thanks,

-liubo

> 
> > 
> > btrfs_commit_transaction() {
> >     ...
> > if (!test_bit(BTRFS_TRANS_DIRTY_BG_RUN, _trans->flags)) {
> > ...
> > ret = btrfs_start_dirty_block_groups(trans, root);
> > }
> > if (ret) {
> > btrfs_end_transaction(trans, root);
> > return ret;
> > }
> > ...
> > cleanup_transaction();
> > }
> > 
> > 
> > But yes, if we delay the cleanup, we still have a chance to do cleanup
> > in btrfs_error_commit_super(), and I have sent another patch to add
> > several ASSERT()s to check block group related memory leak, with which
> > we'll be warned if anything wrong.
> > 
> > I'm OK to remove the part that causes concerns.
> 
> Thanks.
> 
> -chris
> 
--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-07-21 Thread Chris Mason



On 07/21/2016 03:03 PM, Liu Bo wrote:

On Thu, Jul 21, 2016 at 11:32:26AM -0400, Chris Mason wrote:



On 07/20/2016 08:44 PM, Liu Bo wrote:

While processing delayed refs, we may update block group's statistics
and attach it to cur_trans->dirty_bgs, and later writing dirty block
groups will process the list, which happens during
btrfs_commit_transaction().

For whatever reason, the transaction is aborted and dirty_bgs
is not processed in cleanup_transaction(), we end up with memory leak
of these dirty block group cache.

Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
critical section, this also adds the cleanup work inside it.


It's the start_drity_block_groups() hunt that worries me a bit:


diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 50bd683..7a35c9d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3698,6 +3698,8 @@ again:
goto again;
}
spin_unlock(_trans->dirty_bgs_lock);
+   } else if (ret < 0) {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
}

btrfs_free_path(path);



We have checks in here to make sure only one process runs
btrfs_start_dirty_block_groups() but that doesn't mean that only one process
its messing around with the cache inode.  Is there any reason we can't let
this cleanup wait for the cleanup_transaction code?

btrfs_run_delayed_refs() already aborts when it fails.


update_block_group() is the only producer to add block group cache to
dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
is aborted, so seems that there won't be anyone manipulating dirty_bgs
list, am I missing?



No, the dirty_bgs processing is safe I think.  My concern is with the 
cache inode which we iput()



Another point is that when we fail on btrfs_start_dirty_block_groups(),
btrfs_commit_transaction() won't get to cleanup_transaction error
handling,


Right, because we don't actually finish the commit.  Someone will 
eventually though ;)




btrfs_commit_transaction() {
...
if (!test_bit(BTRFS_TRANS_DIRTY_BG_RUN, _trans->flags)) {
...
ret = btrfs_start_dirty_block_groups(trans, root);
}
if (ret) {
btrfs_end_transaction(trans, root);
return ret;
}
...
cleanup_transaction();
}


But yes, if we delay the cleanup, we still have a chance to do cleanup
in btrfs_error_commit_super(), and I have sent another patch to add
several ASSERT()s to check block group related memory leak, with which
we'll be warned if anything wrong.

I'm OK to remove the part that causes concerns.


Thanks.

-chris

--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-07-21 Thread Liu Bo
On Thu, Jul 21, 2016 at 11:32:26AM -0400, Chris Mason wrote:
> 
> 
> On 07/20/2016 08:44 PM, Liu Bo wrote:
> > While processing delayed refs, we may update block group's statistics
> > and attach it to cur_trans->dirty_bgs, and later writing dirty block
> > groups will process the list, which happens during
> > btrfs_commit_transaction().
> > 
> > For whatever reason, the transaction is aborted and dirty_bgs
> > is not processed in cleanup_transaction(), we end up with memory leak
> > of these dirty block group cache.
> > 
> > Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
> > critical section, this also adds the cleanup work inside it.
> 
> It's the start_drity_block_groups() hunt that worries me a bit:
> 
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 50bd683..7a35c9d 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -3698,6 +3698,8 @@ again:
> > goto again;
> > }
> > spin_unlock(_trans->dirty_bgs_lock);
> > +   } else if (ret < 0) {
> > +   btrfs_cleanup_dirty_bgs(cur_trans, root);
> > }
> > 
> > btrfs_free_path(path);
> > 
> 
> We have checks in here to make sure only one process runs
> btrfs_start_dirty_block_groups() but that doesn't mean that only one process
> its messing around with the cache inode.  Is there any reason we can't let
> this cleanup wait for the cleanup_transaction code?
> 
> btrfs_run_delayed_refs() already aborts when it fails.

update_block_group() is the only producer to add block group cache to
dirty_bgs list, and if btrfs_run_delayed_refs() aborts, the transaction
is aborted, so seems that there won't be anyone manipulating dirty_bgs
list, am I missing?

Another point is that when we fail on btrfs_start_dirty_block_groups(),
btrfs_commit_transaction() won't get to cleanup_transaction error
handling,

btrfs_commit_transaction() {
...
if (!test_bit(BTRFS_TRANS_DIRTY_BG_RUN, _trans->flags)) {
...
ret = btrfs_start_dirty_block_groups(trans, root);
}
if (ret) {
btrfs_end_transaction(trans, root);
return ret;
}
...
cleanup_transaction();
}


But yes, if we delay the cleanup, we still have a chance to do cleanup
in btrfs_error_commit_super(), and I have sent another patch to add
several ASSERT()s to check block group related memory leak, with which
we'll be warned if anything wrong.

I'm OK to remove the part that causes concerns.

Thanks,

-liubo
> 
> -chris
> 
> 
--
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


Re: [PATCH v2] Btrfs: fix memory leak of block group cache

2016-07-21 Thread Chris Mason



On 07/20/2016 08:44 PM, Liu Bo wrote:

While processing delayed refs, we may update block group's statistics
and attach it to cur_trans->dirty_bgs, and later writing dirty block
groups will process the list, which happens during
btrfs_commit_transaction().

For whatever reason, the transaction is aborted and dirty_bgs
is not processed in cleanup_transaction(), we end up with memory leak
of these dirty block group cache.

Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
critical section, this also adds the cleanup work inside it.


It's the start_drity_block_groups() hunt that worries me a bit:


diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 50bd683..7a35c9d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3698,6 +3698,8 @@ again:
goto again;
}
spin_unlock(_trans->dirty_bgs_lock);
+   } else if (ret < 0) {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
}

btrfs_free_path(path);



We have checks in here to make sure only one process runs 
btrfs_start_dirty_block_groups() but that doesn't mean that only one 
process its messing around with the cache inode.  Is there any reason we 
can't let this cleanup wait for the cleanup_transaction code?


btrfs_run_delayed_refs() already aborts when it fails.

-chris


--
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


[PATCH v2] Btrfs: fix memory leak of block group cache

2016-07-20 Thread Liu Bo
While processing delayed refs, we may update block group's statistics
and attach it to cur_trans->dirty_bgs, and later writing dirty block
groups will process the list, which happens during
btrfs_commit_transaction().

For whatever reason, the transaction is aborted and dirty_bgs
is not processed in cleanup_transaction(), we end up with memory leak
of these dirty block group cache.

Since btrfs_start_dirty_block_groups() doesn't make it go to the commit
critical section, this also adds the cleanup work inside it.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
v2: - remove the 'return' when dirty_bgs is empty because there
  might be block group in list io_bgs.
- remove unnecessary '\n' in btrfs_err().
- more commit log.

 fs/btrfs/disk-io.c | 71 ++
 fs/btrfs/disk-io.h |  2 ++
 fs/btrfs/extent-tree.c |  2 ++
 3 files changed, 75 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index db53eb8..4c110de 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4434,9 +4434,80 @@ again:
return 0;
 }
 
+static void btrfs_cleanup_bg_io(struct btrfs_block_group_cache *cache)
+{
+   struct inode *inode;
+
+   inode = cache->io_ctl.inode;
+   if (inode) {
+   invalidate_inode_pages2(inode->i_mapping);
+   BTRFS_I(inode)->generation = 0;
+   cache->io_ctl.inode = NULL;
+   iput(inode);
+   }
+   btrfs_put_block_group(cache);
+}
+
+void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
+struct btrfs_root *root)
+{
+   struct btrfs_block_group_cache *cache;
+
+   spin_lock(_trans->dirty_bgs_lock);
+   while (!list_empty(_trans->dirty_bgs)) {
+   cache = list_first_entry(_trans->dirty_bgs,
+struct btrfs_block_group_cache,
+dirty_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group dirty_bgs list");
+   spin_unlock(_trans->dirty_bgs_lock);
+   return;
+   }
+
+   if (!list_empty(>io_list)) {
+   spin_unlock(_trans->dirty_bgs_lock);
+   list_del_init(>io_list);
+   btrfs_cleanup_bg_io(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+
+   list_del_init(>dirty_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+
+   spin_unlock(_trans->dirty_bgs_lock);
+   btrfs_put_block_group(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+   spin_unlock(_trans->dirty_bgs_lock);
+
+   while (!list_empty(_trans->io_bgs)) {
+   cache = list_first_entry(_trans->io_bgs,
+struct btrfs_block_group_cache,
+io_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group on io_bgs list");
+   return;
+   }
+
+   list_del_init(>io_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+   btrfs_cleanup_bg_io(cache);
+   }
+}
+
 void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
   struct btrfs_root *root)
 {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
+   ASSERT(list_empty(_trans->dirty_bgs));
+   ASSERT(list_empty(_trans->io_bgs));
+
btrfs_destroy_delayed_refs(cur_trans, root);
 
cur_trans->state = TRANS_STATE_COMMIT_START;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index acba821..6201663 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -133,6 +133,8 @@ int btrfs_init_log_root_tree(struct btrfs_trans_handle 
*trans,
 struct btrfs_fs_info *fs_info);
 int btrfs_add_log_tree(struct btrfs_trans_handle *trans,
   struct btrfs_root *root);
+void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *trans,
+struct btrfs_root *root);
 void btrfs_cleanup_one_transaction(struct btrfs_transaction *trans,
  struct btrfs_root *root);
 struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 50bd683..7a35c9d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3698,6 +3698,8 @@ again:
goto again;
}
   

[PATCH] Btrfs: fix memory leak of block group cache

2016-07-20 Thread Liu Bo
While processing delayed refs, we may update block group's statistics
and attach it to cur_trans->dirty_bgs, and later writing dirty block
groups will process the list, which happens during
btrfs_commit_transaction().

For whatever reason, the transaction is aborted and dirty_bgs
is not processed in cleanup_transaction(), we end up with memory leak
of these dirty block group cache.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/disk-io.c | 76 ++
 fs/btrfs/disk-io.h |  2 ++
 fs/btrfs/extent-tree.c |  2 ++
 3 files changed, 80 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index db53eb8..6a7998f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4434,9 +4434,85 @@ again:
return 0;
 }
 
+static void btrfs_cleanup_bg_io(struct btrfs_block_group_cache *cache)
+{
+   struct inode *inode;
+
+   inode = cache->io_ctl.inode;
+   if (inode) {
+   invalidate_inode_pages2(inode->i_mapping);
+   BTRFS_I(inode)->generation = 0;
+   cache->io_ctl.inode = NULL;
+   iput(inode);
+   }
+   btrfs_put_block_group(cache);
+}
+
+void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
+struct btrfs_root *root)
+{
+   struct btrfs_block_group_cache *cache;
+
+   spin_lock(_trans->dirty_bgs_lock);
+   if (list_empty(_trans->dirty_bgs)) {
+   spin_unlock(_trans->dirty_bgs_lock);
+   return;
+   }
+
+   while (!list_empty(_trans->dirty_bgs)) {
+   cache = list_first_entry(_trans->dirty_bgs,
+struct btrfs_block_group_cache,
+dirty_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group dirty_bgs list\n");
+   spin_unlock(_trans->dirty_bgs_lock);
+   return;
+   }
+
+   if (!list_empty(>io_list)) {
+   spin_unlock(_trans->dirty_bgs_lock);
+   list_del_init(>io_list);
+   btrfs_cleanup_bg_io(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+
+   list_del_init(>dirty_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+
+   spin_unlock(_trans->dirty_bgs_lock);
+   btrfs_put_block_group(cache);
+   spin_lock(_trans->dirty_bgs_lock);
+   }
+   spin_unlock(_trans->dirty_bgs_lock);
+
+   while (!list_empty(_trans->io_bgs)) {
+   cache = list_first_entry(_trans->io_bgs,
+struct btrfs_block_group_cache,
+io_list);
+   if (!cache) {
+   btrfs_err(root->fs_info,
+ "orphan block group on io_bgs list\n");
+   return;
+   }
+
+   list_del_init(>io_list);
+   spin_lock(>lock);
+   cache->disk_cache_state = BTRFS_DC_ERROR;
+   spin_unlock(>lock);
+   btrfs_cleanup_bg_io(cache);
+   }
+}
+
 void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
   struct btrfs_root *root)
 {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
+   ASSERT(list_empty(_trans->dirty_bgs));
+   ASSERT(list_empty(_trans->io_bgs));
+
btrfs_destroy_delayed_refs(cur_trans, root);
 
cur_trans->state = TRANS_STATE_COMMIT_START;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index acba821..6201663 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -133,6 +133,8 @@ int btrfs_init_log_root_tree(struct btrfs_trans_handle 
*trans,
 struct btrfs_fs_info *fs_info);
 int btrfs_add_log_tree(struct btrfs_trans_handle *trans,
   struct btrfs_root *root);
+void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *trans,
+struct btrfs_root *root);
 void btrfs_cleanup_one_transaction(struct btrfs_transaction *trans,
  struct btrfs_root *root);
 struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 82b912a..a6d2726 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3698,6 +3698,8 @@ again:
goto again;
}
spin_unlock(_trans->dirty_bgs_lock);
+   } else if (ret < 0) {
+   btrfs_cleanup_dirty_bgs(cur_trans, root);
}
 
b

[PATCH] Btrfs: add ASSERT for block group's memory leak

2016-07-20 Thread Liu Bo
This adds several ASSERT()' s to report memory leak of block group cache.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/extent-tree.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 82b912a..50bd683 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -9712,6 +9712,7 @@ void btrfs_put_block_group_cache(struct btrfs_fs_info 
*info)
block_group->iref = 0;
block_group->inode = NULL;
spin_unlock(_group->lock);
+   ASSERT(block_group->io_ctl.inode == NULL);
iput(inode);
last = block_group->key.objectid + block_group->key.offset;
btrfs_put_block_group(block_group);
@@ -9769,6 +9770,10 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
free_excluded_extents(info->extent_root, block_group);
 
btrfs_remove_free_space_cache(block_group);
+   ASSERT(list_empty(_group->dirty_list));
+   ASSERT(list_empty(_group->io_list));
+   ASSERT(list_empty(_group->bg_list));
+   ASSERT(atomic_read(_group->count) == 1);
btrfs_put_block_group(block_group);
 
spin_lock(>block_group_cache_lock);
-- 
2.5.5

--
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


[PATCH] Btrfs: fix memory leak of reloc_root

2016-07-19 Thread Liu Bo
When some critical errors occur and FS would be flipped into RO,
if we have an on-going balance, we can end up with a memory leak
of root->reloc_root since btrfs_drop_snapshots() bails out
without freeing reloc_root at the very early start.

However, we're not able to free reloc_root in btrfs_drop_snapshots()
because its caller, merge_reloc_roots(), still needs to access it to
cleanup reloc_root's rbtree.

This makes us free reloc_root when we're going to free fs/file roots.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/disk-io.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 60ce119..db53eb8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3737,8 +3737,15 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info 
*fs_info,
if (btrfs_root_refs(>root_item) == 0)
synchronize_srcu(_info->subvol_srcu);
 
-   if (test_bit(BTRFS_FS_STATE_ERROR, _info->fs_state))
+   if (test_bit(BTRFS_FS_STATE_ERROR, _info->fs_state)) {
btrfs_free_log(NULL, root);
+   if (root->reloc_root) {
+   free_extent_buffer(root->reloc_root->node);
+   free_extent_buffer(root->reloc_root->commit_root);
+   btrfs_put_fs_root(root->reloc_root);
+   root->reloc_root = NULL;
+   }
+   }
 
if (root->free_ino_pinned)
__btrfs_remove_free_space_cache(root->free_ino_pinned);
-- 
2.5.5

--
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


Re: [PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-12 Thread David Sterba
On Mon, Jul 11, 2016 at 10:39:07AM -0700, Liu Bo wrote:
> eb->io_pages is set in read_extent_buffer_pages().
> 
> In case of readpage failure, for pages that have been added to bio,
> it calls bio_endio and later readpage_io_failed_hook() does the work.
> 
> When this eb's page (couldn't be the 1st page) fails to add itself to bio
> due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
>  and ends up with a memory leak eventually.
> 
> This lets __do_readpage propagate errors to callers and adds the
>  'atomic_dec(>io_pages)'.
> 
> Signed-off-by: Liu Bo <bo.li@oracle.com>

Reviewed-by: David Sterba <dste...@suse.com>

>   if (!PageUptodate(page)) {
> + if (ret) {
> + atomic_dec(>io_pages);
> + unlock_page(page);
> + continue;
> + }

This changes the behaviour to "fail early", which could be positive as a
sequence of unreadable blocks will not try to reread all of them with
the timeouts and retries.
--
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


Re: [PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-11 Thread Liu Bo
On Mon, Jul 11, 2016 at 06:54:02PM -0400, Chris Mason wrote:
> On Mon, Jul 11, 2016 at 03:48:38PM -0700, Liu Bo wrote:
> > On Mon, Jul 11, 2016 at 02:27:39PM -0400, Chris Mason wrote:
> > > 
> > > 
> > > On 07/11/2016 01:39 PM, Liu Bo wrote:
> > > > eb->io_pages is set in read_extent_buffer_pages().
> > > >
> > > > In case of readpage failure, for pages that have been added to bio,
> > > > it calls bio_endio and later readpage_io_failed_hook() does the work.
> > > >
> > > > When this eb's page (couldn't be the 1st page) fails to add itself to 
> > > > bio
> > > > due to failure in merge_bio(), it cannot decrease eb->io_pages via 
> > > > bio_endio,
> > > >  and ends up with a memory leak eventually.
> > > >
> > > > This lets __do_readpage propagate errors to callers and adds the
> > > >  'atomic_dec(>io_pages)'.
> > > 
> > > Thanks for looking at this Liu, how is it currently being tested?
> > 
> > I have a btrfs disk image which was corrupted by btrfs-corrupt-block
> > tool, in that image, the chunk tree's content has been removed while the
> > chunk node can be read from read successfully, so we'd get -EIO when
> > trying to read tree root's node since __btrfs_map_block() would fail to
> > find the right item in chunk mapping_tree.  Thus, we can test our error
> > handling path in read_extent_buffer_pages().
> 
> Fantastic.  Can you please make this an xfstest, maybe along with a dm-flakey?
> as the second phase?

Sure, this depends on a btrfs-corrupt-block patch, which I've not sent
out, I'll try to work out a xfstests case :)

Btw, I'm also planning to add this into our fuzz images of btrfs-progs.

Thanks,

-liubo
--
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


Re: [PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-11 Thread Chris Mason

On Mon, Jul 11, 2016 at 03:48:38PM -0700, Liu Bo wrote:

On Mon, Jul 11, 2016 at 02:27:39PM -0400, Chris Mason wrote:



On 07/11/2016 01:39 PM, Liu Bo wrote:
> eb->io_pages is set in read_extent_buffer_pages().
>
> In case of readpage failure, for pages that have been added to bio,
> it calls bio_endio and later readpage_io_failed_hook() does the work.
>
> When this eb's page (couldn't be the 1st page) fails to add itself to bio
> due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
>  and ends up with a memory leak eventually.
>
> This lets __do_readpage propagate errors to callers and adds the
>  'atomic_dec(>io_pages)'.

Thanks for looking at this Liu, how is it currently being tested?


I have a btrfs disk image which was corrupted by btrfs-corrupt-block
tool, in that image, the chunk tree's content has been removed while the
chunk node can be read from read successfully, so we'd get -EIO when
trying to read tree root's node since __btrfs_map_block() would fail to
find the right item in chunk mapping_tree.  Thus, we can test our error
handling path in read_extent_buffer_pages().


Fantastic.  Can you please make this an xfstest, maybe along with a dm-flakey?
as the second phase?

-chris

--
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


Re: [PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-11 Thread Liu Bo
On Mon, Jul 11, 2016 at 02:27:39PM -0400, Chris Mason wrote:
> 
> 
> On 07/11/2016 01:39 PM, Liu Bo wrote:
> > eb->io_pages is set in read_extent_buffer_pages().
> > 
> > In case of readpage failure, for pages that have been added to bio,
> > it calls bio_endio and later readpage_io_failed_hook() does the work.
> > 
> > When this eb's page (couldn't be the 1st page) fails to add itself to bio
> > due to failure in merge_bio(), it cannot decrease eb->io_pages via 
> > bio_endio,
> >  and ends up with a memory leak eventually.
> > 
> > This lets __do_readpage propagate errors to callers and adds the
> >  'atomic_dec(>io_pages)'.
> 
> Thanks for looking at this Liu, how is it currently being tested?

I have a btrfs disk image which was corrupted by btrfs-corrupt-block
tool, in that image, the chunk tree's content has been removed while the
chunk node can be read from read successfully, so we'd get -EIO when
trying to read tree root's node since __btrfs_map_block() would fail to
find the right item in chunk mapping_tree.  Thus, we can test our error
handling path in read_extent_buffer_pages().

Thanks,

-liubo

> 
> -chris
--
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


Re: [PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-11 Thread Chris Mason



On 07/11/2016 01:39 PM, Liu Bo wrote:

eb->io_pages is set in read_extent_buffer_pages().

In case of readpage failure, for pages that have been added to bio,
it calls bio_endio and later readpage_io_failed_hook() does the work.

When this eb's page (couldn't be the 1st page) fails to add itself to bio
due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
 and ends up with a memory leak eventually.

This lets __do_readpage propagate errors to callers and adds the
 'atomic_dec(>io_pages)'.


Thanks for looking at this Liu, how is it currently being tested?

-chris
--
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


[PATCH v3] Btrfs: fix eb memory leak due to readpage failure

2016-07-11 Thread Liu Bo
eb->io_pages is set in read_extent_buffer_pages().

In case of readpage failure, for pages that have been added to bio,
it calls bio_endio and later readpage_io_failed_hook() does the work.

When this eb's page (couldn't be the 1st page) fails to add itself to bio
due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
 and ends up with a memory leak eventually.

This lets __do_readpage propagate errors to callers and adds the
 'atomic_dec(>io_pages)'.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
v2: - Move 'dec io_pages' to the caller so that we're consistent with
  write_one_eb()
v3: - Bail out once we fail to read a page and do the cleanup work
  for eb->io_pages

 fs/btrfs/extent_io.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ac1a696..7303e5a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2878,6 +2878,7 @@ __get_extent_map(struct inode *inode, struct page *page, 
size_t pg_offset,
  * into the tree that are removed when the IO is done (by the end_io
  * handlers)
  * XXX JDM: This needs looking at to ensure proper page locking
+ * return 0 on success, otherwise return error
  */
 static int __do_readpage(struct extent_io_tree *tree,
 struct page *page,
@@ -2899,7 +2900,7 @@ static int __do_readpage(struct extent_io_tree *tree,
sector_t sector;
struct extent_map *em;
struct block_device *bdev;
-   int ret;
+   int ret = 0;
int nr = 0;
size_t pg_offset = 0;
size_t iosize;
@@ -3080,6 +3081,7 @@ static int __do_readpage(struct extent_io_tree *tree,
} else {
SetPageError(page);
unlock_extent(tree, cur, cur + iosize - 1);
+   goto out;
}
cur = cur + iosize;
pg_offset += iosize;
@@ -3090,7 +3092,7 @@ out:
SetPageUptodate(page);
unlock_page(page);
}
-   return 0;
+   return ret;
 }
 
 static inline void __do_contiguous_readpages(struct extent_io_tree *tree,
@@ -5230,14 +5232,31 @@ int read_extent_buffer_pages(struct extent_io_tree 
*tree,
atomic_set(>io_pages, num_reads);
for (i = start_i; i < num_pages; i++) {
page = eb->pages[i];
+
if (!PageUptodate(page)) {
+   if (ret) {
+   atomic_dec(>io_pages);
+   unlock_page(page);
+   continue;
+   }
+
ClearPageError(page);
err = __extent_read_full_page(tree, page,
  get_extent, ,
  mirror_num, _flags,
  READ | REQ_META);
-   if (err)
+   if (err) {
ret = err;
+   /*
+* We use  in above __extent_read_full_page,
+* so we ensure that if it returns error, the
+* current page fails to add itself to bio and
+* it's been unlocked.
+*
+* We must dec io_pages by ourselves.
+*/
+   atomic_dec(>io_pages);
+   }
} else {
unlock_page(page);
}
-- 
2.5.5

--
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


Re: [PATCH v2] Btrfs: fix eb memory leak due to readpage failure

2016-07-08 Thread Liu Bo
On Fri, Jul 08, 2016 at 06:01:49PM +0200, David Sterba wrote:
> On Fri, Jun 03, 2016 at 12:08:38PM -0700, Liu Bo wrote:
> > eb->io_pages is set in read_extent_buffer_pages().
> > 
> > In case of readpage failure, for pages that have been added to bio,
> > it calls bio_endio and later readpage_io_failed_hook() does the work.
> > 
> > When this eb's page (couldn't be the 1st page) fails to add itself to bio
> > due to failure in merge_bio(), it cannot decrease eb->io_pages via 
> > bio_endio,
> >  and ends up with a memory leak eventually.
> > 
> > This lets __do_readpage propagate errors to callers and adds the
> >  'atomic_dec(>io_pages)'.
> 
> I'm not sure, but could we lose some error values from __do_readpage?
> Ie. return 0 even if there was an error in a page that's in the middle
> (not the first, not the last).
> 
> The loop in __do_readpage iterates while (cur <= end), and ret is only
> set by submit_extent_page, but the loop does not exit immediatelly. So
> we can detect error, set page error state bit, but next loop will
> overwrite ret with 0 (if the page submission was ok).
> 
> Then we still don't decrement the io_pages as needed.

Right, it still has that problem, then the possible way I can see is to break
the while (cur <= end) loop when we fail on submit_extent_page() and
pass an error up to its caller and we can do the rest eb->io_pages cleanup work 
in
read_extent_buffer_pages(), just like how we did in write_one_eb()
(this was already suggested by Josef, but seems I was off the right track).

This also assumes that if one page fails on submit_extent_page(), it's
likely for the rest pages to fail as well.

What do you think?

Thanks,

-liubo
--
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


Re: [PATCH v2] Btrfs: fix eb memory leak due to readpage failure

2016-07-08 Thread David Sterba
On Fri, Jun 03, 2016 at 12:08:38PM -0700, Liu Bo wrote:
> eb->io_pages is set in read_extent_buffer_pages().
> 
> In case of readpage failure, for pages that have been added to bio,
> it calls bio_endio and later readpage_io_failed_hook() does the work.
> 
> When this eb's page (couldn't be the 1st page) fails to add itself to bio
> due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
>  and ends up with a memory leak eventually.
> 
> This lets __do_readpage propagate errors to callers and adds the
>  'atomic_dec(>io_pages)'.

I'm not sure, but could we lose some error values from __do_readpage?
Ie. return 0 even if there was an error in a page that's in the middle
(not the first, not the last).

The loop in __do_readpage iterates while (cur <= end), and ret is only
set by submit_extent_page, but the loop does not exit immediatelly. So
we can detect error, set page error state bit, but next loop will
overwrite ret with 0 (if the page submission was ok).

Then we still don't decrement the io_pages as needed.
--
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


Re: [PATCH v2] Btrfs: fix eb memory leak due to readpage failure

2016-06-13 Thread David Sterba
On Fri, Jun 03, 2016 at 12:08:38PM -0700, Liu Bo wrote:
> eb->io_pages is set in read_extent_buffer_pages().
> 
> In case of readpage failure, for pages that have been added to bio,
> it calls bio_endio and later readpage_io_failed_hook() does the work.
> 
> When this eb's page (couldn't be the 1st page) fails to add itself to bio
> due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
>  and ends up with a memory leak eventually.
> 
> This lets __do_readpage propagate errors to callers and adds the
>  'atomic_dec(>io_pages)'.
> 
> Signed-off-by: Liu Bo <bo.li@oracle.com>

I'm adding this to for-next, but a review is needed if this is supposed
to go to 4.7.
--
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


[PATCH v2] Btrfs: fix eb memory leak due to readpage failure

2016-06-03 Thread Liu Bo
eb->io_pages is set in read_extent_buffer_pages().

In case of readpage failure, for pages that have been added to bio,
it calls bio_endio and later readpage_io_failed_hook() does the work.

When this eb's page (couldn't be the 1st page) fails to add itself to bio
due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
 and ends up with a memory leak eventually.

This lets __do_readpage propagate errors to callers and adds the
 'atomic_dec(>io_pages)'.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
v2:
  - Move 'dec io_pages' to the caller so that we're consistent with
write_one_eb()

 fs/btrfs/extent_io.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d247fc0..0309388 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2869,6 +2869,7 @@ __get_extent_map(struct inode *inode, struct page *page, 
size_t pg_offset,
  * into the tree that are removed when the IO is done (by the end_io
  * handlers)
  * XXX JDM: This needs looking at to ensure proper page locking
+ * return 0 on success, otherwise return error
  */
 static int __do_readpage(struct extent_io_tree *tree,
 struct page *page,
@@ -2890,7 +2891,7 @@ static int __do_readpage(struct extent_io_tree *tree,
sector_t sector;
struct extent_map *em;
struct block_device *bdev;
-   int ret;
+   int ret = 0;
int nr = 0;
size_t pg_offset = 0;
size_t iosize;
@@ -3081,7 +3082,7 @@ out:
SetPageUptodate(page);
unlock_page(page);
}
-   return 0;
+   return ret;
 }
 
 static inline void __do_contiguous_readpages(struct extent_io_tree *tree,
@@ -5204,8 +5205,17 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
  get_extent, ,
  mirror_num, _flags,
  READ | REQ_META);
-   if (err)
+   if (err) {
ret = err;
+   /*
+* We use  in above __extent_read_full_page,
+* so we ensure that if it returns error, the
+* current page fails to add itself to bio.
+*
+* We must dec io_pages by ourselves.
+*/
+   atomic_dec(>io_pages);
+   }
} else {
unlock_page(page);
}
-- 
2.5.5

--
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


Re: [PATCH 6/7] Btrfs: fix eb memory leak due to readpage failure

2016-05-18 Thread Josef Bacik

On 05/13/2016 08:07 PM, Liu Bo wrote:

eb->io_pages is set in read_extent_buffer_pages().

In case of readpage failure, for pages that have been added to bio,
it calls bio_endio and later readpage_io_failed_hook() does the work.

When this eb's page (couldn't be the 1st page) fails to add itself to bio
due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
 and ends up with a memory leak eventually.

This adds the 'atomic_dec(>io_pages)' to the readpage error handling.


Wait why can't this be done in


Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/extent_io.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 99286d1..2327200 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3069,6 +3069,30 @@ static int __do_readpage(struct extent_io_tree *tree,
*bio_flags = this_bio_flag;
} else {
SetPageError(page);
+   /*
+* Only metadata io request has this issue, for data it
+* just unlocks extent and releases page's lock.
+*
+* eb->io_pages is set in read_extent_buffer_pages().
+*
+* When this eb's page fails to add itself to bio,
+* it cannot decrease eb->io_pages via bio_endio, and
+* ends up with extent_buffer_under_io() always being
+* true, because of that, eb won't be freed and we have
+        * a memory leak eventually.
+*
+* Here we still hold this page's lock, and other tasks
+* who're also reading this eb are blocked.
+*/
+   if (rw & REQ_META) {
+   struct extent_buffer *eb;
+
+   WARN_ON_ONCE(!PagePrivate(page));
+   eb = (struct extent_buffer *)page->private;
+
+   WARN_ON_ONCE(atomic_read(>io_pages) < 1);
+   atomic_dec(>io_pages);
+   }
unlock_extent(tree, cur, cur + iosize - 1);
}
cur = cur + iosize;



This isn't the right way to do this.  It looks like we don't propagate 
up errors from __do_readpage, which we need to in order to clean up 
properly.  So do that and then change the error stuff to decrement the 
io_pages for the remaining, you can see write_one_eb for how to deal 
with that properly.  Thanks,


Josef
--
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


RE: [PATCH] btrfs: fix memory leak during RAID 5/6 device replacement

2016-05-16 Thread Zhao Lei
Hi, Scott Talbert

> From: Zhao Lei [mailto:zhao...@cn.fujitsu.com]
> Sent: Friday, May 13, 2016 6:31 PM
> To: 'dste...@suse.cz' <dste...@suse.cz>; 'Scott Talbert'
> <scott.talb...@hgst.com>
> Cc: 'linux-btrfs@vger.kernel.org' <linux-btrfs@vger.kernel.org>
> Subject: RE: [PATCH] btrfs: fix memory leak during RAID 5/6 device replacement
> 
> Hi, Scott Talbert
> 
> * From: David Sterba [mailto:dste...@suse.cz]
> > Sent: Tuesday, May 10, 2016 1:32 AM
> > To: Scott Talbert <scott.talb...@hgst.com>
> > Cc: linux-btrfs@vger.kernel.org; Zhao Lei <zhao...@cn.fujitsu.com>
> > Subject: Re: [PATCH] btrfs: fix memory leak during RAID 5/6 device
> replacement
> >
> > CC Zhao Lei <zhao...@cn.fujitsu.com>
> >
> > On Mon, May 09, 2016 at 09:14:28AM -0400, Scott Talbert wrote:
> > > A 'struct bio' is allocated in scrub_missing_raid56_pages(), but it was 
> > > never
> > > freed anywhere.
> > >
> > > Signed-off-by: Scott Talbert <scott.talb...@hgst.com>
> > > ---
> > >  fs/btrfs/scrub.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > > index 82bedf9..607cc6e 100644
> > > --- a/fs/btrfs/scrub.c
> > > +++ b/fs/btrfs/scrub.c
> > > @@ -2130,6 +2130,8 @@ static void scrub_missing_raid56_end_io(struct
> > bio *bio)
> > >   if (bio->bi_error)
> > >   sblock->no_io_error_seen = 0;
> > >
> > > + bio_put(bio);
> > > +
> Seems good by reviewing.
> I'll add some debug code to test it more detailedly.
> 
Signed-off-by: Zhao Lei <zhao...@cn.fujitsu.com>
Test-by: Zhao Lei <zhao...@cn.fujitsu.com>

Thanks
Zhaolei

> Thanks
> Zhaolei
> 
> > >   btrfs_queue_work(fs_info->scrub_workers, >work);
> > >  }
> > >
> > > --
> > > 1.9.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




--
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


[PATCH 6/7] Btrfs: fix eb memory leak due to readpage failure

2016-05-13 Thread Liu Bo
eb->io_pages is set in read_extent_buffer_pages().

In case of readpage failure, for pages that have been added to bio,
it calls bio_endio and later readpage_io_failed_hook() does the work.

When this eb's page (couldn't be the 1st page) fails to add itself to bio
due to failure in merge_bio(), it cannot decrease eb->io_pages via bio_endio,
 and ends up with a memory leak eventually.

This adds the 'atomic_dec(>io_pages)' to the readpage error handling.

Signed-off-by: Liu Bo <bo.li@oracle.com>
---
 fs/btrfs/extent_io.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 99286d1..2327200 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3069,6 +3069,30 @@ static int __do_readpage(struct extent_io_tree *tree,
*bio_flags = this_bio_flag;
} else {
SetPageError(page);
+   /*
+* Only metadata io request has this issue, for data it
+* just unlocks extent and releases page's lock.
+*
+* eb->io_pages is set in read_extent_buffer_pages().
+*
+* When this eb's page fails to add itself to bio,
+* it cannot decrease eb->io_pages via bio_endio, and
+* ends up with extent_buffer_under_io() always being
+* true, because of that, eb won't be freed and we have
+        * a memory leak eventually.
+*
+* Here we still hold this page's lock, and other tasks
+* who're also reading this eb are blocked.
+*/
+   if (rw & REQ_META) {
+   struct extent_buffer *eb;
+
+   WARN_ON_ONCE(!PagePrivate(page));
+   eb = (struct extent_buffer *)page->private;
+
+   WARN_ON_ONCE(atomic_read(>io_pages) < 1);
+   atomic_dec(>io_pages);
+   }
unlock_extent(tree, cur, cur + iosize - 1);
}
cur = cur + iosize;
-- 
2.5.5

--
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


[PATCH 7/7] Btrfs: fix memory leak due to invalid btree height

2016-05-13 Thread Liu Bo
Thanks to fuzz testing, we can have invalid btree root node height.
Btrfs limits btree height to 7 and if the given height is 9, then btrfs
will have problems in both releasing root node's lock and freeing the node.

Signed-off-by: Liu Bo 
---
 fs/btrfs/ctree.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index ec7928a..3fccbcc 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2756,6 +2756,13 @@ again:
}
}
}
+   if (level > BTRFS_MAX_LEVEL - 1 || level < 0) {
+   WARN_ONCE(1, KERN_WARNING "Invalid btree height %d\n", level);
+   if (!p->skip_locking)
+   btrfs_tree_unlock_rw(b, root_lock);
+   free_extent_buffer(b);
+   return -EINVAL;
+   }
p->nodes[level] = b;
if (!p->skip_locking)
p->locks[level] = root_lock;
-- 
2.5.5

--
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


RE: [PATCH] btrfs: fix memory leak during RAID 5/6 device replacement

2016-05-13 Thread Zhao Lei
Hi, Scott Talbert

* From: David Sterba [mailto:dste...@suse.cz]
> Sent: Tuesday, May 10, 2016 1:32 AM
> To: Scott Talbert <scott.talb...@hgst.com>
> Cc: linux-btrfs@vger.kernel.org; Zhao Lei <zhao...@cn.fujitsu.com>
> Subject: Re: [PATCH] btrfs: fix memory leak during RAID 5/6 device replacement
> 
> CC Zhao Lei <zhao...@cn.fujitsu.com>
> 
> On Mon, May 09, 2016 at 09:14:28AM -0400, Scott Talbert wrote:
> > A 'struct bio' is allocated in scrub_missing_raid56_pages(), but it was 
> > never
> > freed anywhere.
> >
> > Signed-off-by: Scott Talbert <scott.talb...@hgst.com>
> > ---
> >  fs/btrfs/scrub.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > index 82bedf9..607cc6e 100644
> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -2130,6 +2130,8 @@ static void scrub_missing_raid56_end_io(struct
> bio *bio)
> > if (bio->bi_error)
> > sblock->no_io_error_seen = 0;
> >
> > +   bio_put(bio);
> > +
Seems good by reviewing.
I'll add some debug code to test it more detailedly.

Thanks
Zhaolei

> > btrfs_queue_work(fs_info->scrub_workers, >work);
> >  }
> >
> > --
> > 1.9.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




--
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


Re: [PATCH] btrfs: fix memory leak during RAID 5/6 device replacement

2016-05-09 Thread David Sterba
CC Zhao Lei 

On Mon, May 09, 2016 at 09:14:28AM -0400, Scott Talbert wrote:
> A 'struct bio' is allocated in scrub_missing_raid56_pages(), but it was never
> freed anywhere.
> 
> Signed-off-by: Scott Talbert 
> ---
>  fs/btrfs/scrub.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 82bedf9..607cc6e 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2130,6 +2130,8 @@ static void scrub_missing_raid56_end_io(struct bio *bio)
>   if (bio->bi_error)
>   sblock->no_io_error_seen = 0;
>  
> + bio_put(bio);
> +
>   btrfs_queue_work(fs_info->scrub_workers, >work);
>  }
>  
> -- 
> 1.9.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
--
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


[PATCH] btrfs: fix memory leak during RAID 5/6 device replacement

2016-05-09 Thread Scott Talbert
A 'struct bio' is allocated in scrub_missing_raid56_pages(), but it was never
freed anywhere.

Signed-off-by: Scott Talbert 
---
 fs/btrfs/scrub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 82bedf9..607cc6e 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2130,6 +2130,8 @@ static void scrub_missing_raid56_end_io(struct bio *bio)
if (bio->bi_error)
sblock->no_io_error_seen = 0;
 
+   bio_put(bio);
+
btrfs_queue_work(fs_info->scrub_workers, >work);
 }
 
-- 
1.9.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


[PATCH v8 21/27] btrfs: Fix a memory leak in inband dedupe hash

2016-03-21 Thread Qu Wenruo
We allocate a dedupe hash into async_extent, but forget to free it.
Fix it by freeing the hash before freeing async_extent.

Reported-by: Wang Xiaoguang 
Signed-off-by: Qu Wenruo 
---
 fs/btrfs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 979811c..81b19193 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -751,6 +751,7 @@ retry:
  WB_SYNC_ALL);
else if (ret)
unlock_page(async_cow->locked_page);
+   kfree(hash);
kfree(async_extent);
cond_resched();
continue;
@@ -876,6 +877,7 @@ retry:
free_async_extent_pages(async_extent);
}
alloc_hint = ins.objectid + ins.offset;
+   kfree(hash);
kfree(async_extent);
cond_resched();
}
@@ -892,6 +894,7 @@ out_free:
 PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK |
 PAGE_SET_ERROR);
free_async_extent_pages(async_extent);
+   kfree(hash);
kfree(async_extent);
goto again;
 }
-- 
2.7.3



--
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


[PATCH 09/10] btrfs-progs: fix memory leak in cmd_qgroup_show()

2015-10-19 Thread Eryu Guan
filter_set and comparer_set should be freed on return.

Signed-off-by: Eryu Guan 
---
 cmds-qgroup.c | 2 ++
 qgroup.c  | 1 +
 2 files changed, 3 insertions(+)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 48c1733..82bd2e2 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -354,6 +354,8 @@ static int cmd_qgroup_show(int argc, char **argv)
fd = open_file_or_dir(path, );
if (fd < 0) {
fprintf(stderr, "ERROR: can't access '%s'\n", path);
+   btrfs_qgroup_free_filter_set(filter_set);
+   btrfs_qgroup_free_comparer_set(comparer_set);
return 1;
}
 
diff --git a/qgroup.c b/qgroup.c
index ec9a3ac..0272aa8 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -1211,6 +1211,7 @@ int btrfs_show_qgroups(int fd,
 
__free_all_qgroups(_lookup);
btrfs_qgroup_free_filter_set(filter_set);
+   btrfs_qgroup_free_comparer_set(comp_set);
return ret;
 }
 
-- 
2.4.3

--
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


[PATCH 07/10] btrfs-progs: fix memory leak on error path

2015-10-19 Thread Eryu Guan
dev_scans and t_scans should be freed on malloc error.

Signed-off-by: Eryu Guan 
---
 chunk-recover.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index 1fb04f7..c727f0f 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -847,11 +847,16 @@ static int scan_devices(struct recover_control *rc)
if (!dev_scans)
return -ENOMEM;
t_scans = (pthread_t *)malloc(sizeof(pthread_t) * devnr);
-   if (!t_scans)
+   if (!t_scans) {
+   free(dev_scans);
return -ENOMEM;
+   }
t_rets = (long *)malloc(sizeof(long) * devnr);
-   if (!t_rets)
+   if (!t_rets) {
+   free(dev_scans);
+   free(t_scans);
return -ENOMEM;
+   }
 
list_for_each_entry(dev, >fs_devices->devices, dev_list) {
fd = open(dev->name, O_RDONLY);
-- 
2.4.3

--
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


Re: [PATCH] Btrfs: open_ctree: Fix possible memory leak

2015-10-07 Thread David Sterba
On Mon, Oct 05, 2015 at 10:14:25PM +0530, Chandan Rajendra wrote:
> After reading one of chunk or tree root tree's root node from disk, if the
> root node does not have EXTENT_BUFFER_UPTODATE flag set, we fail to release
> the memory used by the root node. Fix this.
> 
> Signed-off-by: Chandan Rajendra 

Good catch.

Reviewed-by: David Sterba 
--
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


Re: [PATCH 1/3] btrfs-progs: fix memory leak in btrfs-convert main()

2015-08-31 Thread David Sterba
On Fri, Aug 28, 2015 at 12:38:16AM +0900, Byongho Lee wrote:
> In btrfs-convert main(), strdup() allocates memory to fslabel but that
> memory is not freed. We could fix it by adding free() calls to every
> return point, but that would make the code messy because there are
> several return paths.
> So I fix it by changing the code using strdup() with local array and
> strncpy().
> 
> And btrfs-convert main() guarantees that string length of fslabel is not
> to exceed 'BTRFS_LABEL_SIZE', so it's enough to use strcpy() instead of
> strncpy() to copy fslabel in do_convert().
> 
> Signed-off-by: Byongho Lee 

Applied, thanks.
--
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


Re: [PATCH 2/3] btrfs-progs: fix memory leak in btrfs-map-logical main()

2015-08-28 Thread David Sterba
On Fri, Aug 28, 2015 at 12:38:17AM +0900, Byongho Lee wrote:
 In btrfs-map-logical main(), strdup() allocates memory to output_file,
 but that memory is not freed.
 So add missing free() calls before return.
 
 Signed-off-by: Byongho Lee bhlee.ker...@gmail.com

Applied, thanks.
--
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


[PATCH 2/3] btrfs-progs: fix memory leak in btrfs-map-logical main()

2015-08-27 Thread Byongho Lee
In btrfs-map-logical main(), strdup() allocates memory to output_file,
but that memory is not freed.
So add missing free() calls before return.

Signed-off-by: Byongho Lee bhlee.ker...@gmail.com
---
 btrfs-map-logical.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/btrfs-map-logical.c b/btrfs-map-logical.c
index a88e56e39dbb..d9fa6b29d3c9 100644
--- a/btrfs-map-logical.c
+++ b/btrfs-map-logical.c
@@ -262,6 +262,7 @@ int main(int ac, char **av)
root = open_ctree(dev, 0, 0);
if (!root) {
fprintf(stderr, Open ctree failed\n);
+   free(output_file);
exit(1);
}
 
@@ -354,6 +355,7 @@ out_close_fd:
if (output_file  out_fd != 1)
close(out_fd);
 close:
+   free(output_file);
close_ctree(root);
if (ret  0)
ret = 1;
-- 
2.5.0

--
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


[PATCH 1/3] btrfs-progs: fix memory leak in btrfs-convert main()

2015-08-27 Thread Byongho Lee
In btrfs-convert main(), strdup() allocates memory to fslabel but that
memory is not freed. We could fix it by adding free() calls to every
return point, but that would make the code messy because there are
several return paths.
So I fix it by changing the code using strdup() with local array and
strncpy().

And btrfs-convert main() guarantees that string length of fslabel is not
to exceed 'BTRFS_LABEL_SIZE', so it's enough to use strcpy() instead of
strncpy() to copy fslabel in do_convert().

Signed-off-by: Byongho Lee bhlee.ker...@gmail.com
---
 btrfs-convert.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 917bbc1b74d2..25ae424ea73b 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2428,7 +2428,7 @@ static int do_convert(const char *devname, int datacsum, 
int packing, int noxatt
fprintf(stderr, copy label '%s'\n,
root-fs_info-super_copy-label);
} else if (copylabel == -1) {
-   strncpy(root-fs_info-super_copy-label, fslabel, 
BTRFS_LABEL_SIZE);
+   strcpy(root-fs_info-super_copy-label, fslabel);
fprintf(stderr, set label to '%s'\n, fslabel);
}
 
@@ -2868,7 +2868,7 @@ int main(int argc, char *argv[])
int usage_error = 0;
int progress = 1;
char *file;
-   char *fslabel = NULL;
+   char fslabel[BTRFS_LABEL_SIZE+1];
u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
 
while(1) {
@@ -2910,8 +2910,9 @@ int main(int argc, char *argv[])
break;
case 'l':
copylabel = -1;
-   fslabel = strdup(optarg);
-   if (strlen(fslabel)  BTRFS_LABEL_SIZE) {
+   fslabel[BTRFS_LABEL_SIZE] = 0;
+   strncpy(fslabel, optarg, sizeof(fslabel));
+   if (fslabel[BTRFS_LABEL_SIZE]) {
fprintf(stderr,
warning: label too long, 
trimmed to %d bytes\n,
BTRFS_LABEL_SIZE);
-- 
2.5.0

--
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


Re: [PATCH v2] Btrfs: fix memory leak in the extent_same ioctl

2015-07-06 Thread Mark Fasheh
Thanks for this Filipe,

On Fri, Jul 03, 2015 at 11:36:49AM +0100, fdman...@kernel.org wrote:
 From: Filipe Manana fdman...@suse.com
 
 We were allocating memory with memdup_user() but we were never releasing
 that memory. This affected pretty much every call to the ioctl, whether
 it deduplicated extents or not.
 
 This issue was reported on IRC by Julian Taylor and on the mailing list
 by Marcel Ritter, credit goes to them for finding the issue.
 
 Reported-by: Julian Taylor jtaylor.deb...@googlemail.com
 Reported-by: Marcel Ritter ritter.mar...@gmail.com
 Cc: sta...@vger.kernel.org
 Signed-off-by: Filipe Manana fdman...@suse.com

Reviewed-by: Mark Fasheh mfas...@suse.de
--Mark

--
Mark Fasheh
--
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


Re: [PATCH] Btrfs: fix memory leak in the extent_same ioctl

2015-07-03 Thread Mark Fasheh
That looks great, thanks for fixing this Filipe.

On Fri, Jul 03, 2015 at 08:48:22AM +0100, fdman...@kernel.org wrote:
 From: Filipe Manana fdman...@suse.com
 
 We were allocating memory with memdup_user() but we were never releasing
 that memory. This affected pretty much every call to the ioctl, whether
 it deduplicated extents or not.
 
 This issue was reported on IRC by Julian Taylor and on the mailing list
 by Marcel Ritter, credit goes to them for finding the issue.
 
 Reported-by: Julian Taylor (jtaylor@IRC)
 Reported-by: Marcel Ritter ritter.mar...@gmail.com
 Signed-off-by: Filipe Manana fdman...@suse.com

Reviewed-by: Mark Fasheh mfas...@suse.de
--Mark

--
Mark Fasheh
--
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


[PATCH] Btrfs: fix memory leak in the extent_same ioctl

2015-07-03 Thread fdmanana
From: Filipe Manana fdman...@suse.com

We were allocating memory with memdup_user() but we were never releasing
that memory. This affected pretty much every call to the ioctl, whether
it deduplicated extents or not.

This issue was reported on IRC by Julian Taylor and on the mailing list
by Marcel Ritter, credit goes to them for finding the issue.

Reported-by: Julian Taylor (jtaylor@IRC)
Reported-by: Marcel Ritter ritter.mar...@gmail.com
Signed-off-by: Filipe Manana fdman...@suse.com
---
 fs/btrfs/ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index c86b835..78e6a28 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2958,7 +2958,7 @@ out_unlock:
 static long btrfs_ioctl_file_extent_same(struct file *file,
struct btrfs_ioctl_same_args __user *argp)
 {
-   struct btrfs_ioctl_same_args *same;
+   struct btrfs_ioctl_same_args *same = NULL;
struct btrfs_ioctl_same_extent_info *info;
struct inode *src = file_inode(file);
u64 off;
@@ -2988,6 +2988,7 @@ static long btrfs_ioctl_file_extent_same(struct file 
*file,
 
if (IS_ERR(same)) {
ret = PTR_ERR(same);
+   same = NULL;
goto out;
}
 
@@ -3058,6 +3059,7 @@ static long btrfs_ioctl_file_extent_same(struct file 
*file,
 
 out:
mnt_drop_write_file(file);
+   kfree(same);
return ret;
 }
 
-- 
2.1.3

--
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


Re: linux 4.1 - memory leak (possibly dedup related)

2015-07-03 Thread Filipe David Manana
On Fri, Jul 3, 2015 at 7:58 AM, Marcel Ritter ritter.mar...@gmail.com wrote:
 Hi,

 I've been running some btrfs tests (mainly duperemove related) with
 linux kernel 4.1 for the last few days.

 Now I noticed by accident (dying processes), that all my memory (128
 GB!) is gone.
 Gone meaning, there's no user space process allocating this memory.

 Digging deeper I found the missing memory using slabtop (see output of
 /proc/slabinfo is attached): Looks like I got a lot of kernel memory
 allocated by kmalloc-1024 (memory leak?).
 Given the fact that the test machine does little more than btrfs
 testing I think this may be btrfs related.

 I was running duperemove on a 1.5 TB volume around the time the first
 Out of memory error were logged, so maybe the memory leak can be
 found somewhere in this code path.

 I'm still waiting for a scrub run to finish, after that I'll reboot
 the machine and try to reproduce this behaviour with a fresh btrfs
 filesystem.

 Have there been any fixes concerning memory leaks since 4.1 release I could 
 try?
 Any other ideas how to track down this potential memory leak?

Hi,

We had Julian Taylor reporting the same on IRC a couple days ago (he
also found what was being leaked). I just sent a fix and cc'ed you
(https://patchwork.kernel.org/patch/6713301/).

thanks


 Bye,
Marcel



-- 
Filipe David Manana,

Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men.
--
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


linux 4.1 - memory leak (possibly dedup related)

2015-07-03 Thread Marcel Ritter
Hi,

I've been running some btrfs tests (mainly duperemove related) with
linux kernel 4.1 for the last few days.

Now I noticed by accident (dying processes), that all my memory (128
GB!) is gone.
Gone meaning, there's no user space process allocating this memory.

Digging deeper I found the missing memory using slabtop (see output of
/proc/slabinfo is attached): Looks like I got a lot of kernel memory
allocated by kmalloc-1024 (memory leak?).
Given the fact that the test machine does little more than btrfs
testing I think this may be btrfs related.

I was running duperemove on a 1.5 TB volume around the time the first
Out of memory error were logged, so maybe the memory leak can be
found somewhere in this code path.

I'm still waiting for a scrub run to finish, after that I'll reboot
the machine and try to reproduce this behaviour with a fresh btrfs
filesystem.

Have there been any fixes concerning memory leaks since 4.1 release I could try?
Any other ideas how to track down this potential memory leak?

Bye,
   Marcel
slabinfo - version: 2.1
# nameactive_objs num_objs objsize objperslab pagesperslab : tunables limit batchcount sharedfactor : slabdata active_slabs num_slabs sharedavail
btrfs_delayed_data_ref   2394   2982 96   421 : tunables000 : slabdata 71 71  0
btrfs_delayed_tree_ref   3726   4600 88   461 : tunables000 : slabdata100100  0
btrfs_delayed_ref_head   1900   2100160   251 : tunables000 : slabdata 84 84  0
btrfs_delayed_node   1561   1794304   262 : tunables000 : slabdata 69 69  0
btrfs_ordered_extent   1140   1558424   384 : tunables000 : slabdata 41 41  0
bio-2   1600   1625320   252 : tunables000 : slabdata 65 65  0
btrfs_extent_buffer688   1276280   292 : tunables000 : slabdata 44 44  0
btrfs_extent_state   4539   4590 80   511 : tunables000 : slabdata 90 90  0
btrfs_delalloc_work  0  0152   261 : tunables000 : slabdata  0  0  0
btrfs_transaction176176360   222 : tunables000 : slabdata  8  8  0
btrfs_trans_handle184184176   231 : tunables000 : slabdata  8  8  0
btrfs_inode 1636   6388984   338 : tunables000 : slabdata205205  0
nfs4_layout_stateid  0  0240   342 : tunables000 : slabdata  0  0  0
nfsd4_delegations  0  0224   362 : tunables000 : slabdata  0  0  0
nfsd4_files0  0288   282 : tunables000 : slabdata  0  0  0
nfsd4_openowners   0  0440   374 : tunables000 : slabdata  0  0  0
nfs_direct_cache   0  0352   232 : tunables000 : slabdata  0  0  0
nfs_commit_data   23 23704   234 : tunables000 : slabdata  1  1  0
nfs_inode_cache0  0   1000   328 : tunables000 : slabdata  0  0  0
rpc_inode_cache   50 50640   254 : tunables000 : slabdata  2  2  0
fscache_cookie_jar 46 46 88   461 : tunables000 : slabdata  1  1  0
ext3_inode_cache 160160808   204 : tunables000 : slabdata  8  8  0
journal_handle  1360   1360 24  1701 : tunables000 : slabdata  8  8  0
ext4_groupinfo_4k   3887   6636144   281 : tunables000 : slabdata237237  0
ip6-frags  0  0216   372 : tunables000 : slabdata  0  0  0
UDPLITEv6  0  0   1088   308 : tunables000 : slabdata  0  0  0
UDPv6240240   1088   308 : tunables000 : slabdata  8  8  0
tw_sock_TCPv6 58 58280   292 : tunables000 : slabdata  2  2  0
TCPv6112112   2240   148 : tunables000 : slabdata  8  8  0
kcopyd_job 0  0   331298 : tunables000 : slabdata  0  0  0
dm_uevent  0  0   2632   128 : tunables000 : slabdata  0  0  0
cfq_queue  0  0232   352 : tunables000 : slabdata  0  0  0
bsg_cmd0  0312   262 : tunables000 : slabdata  0  0  0
mqueue_inode_cache 36 36896   368 : tunables000 : slabdata  1  1  0
fuse_request   0  0400   202 : tunables000 : slabdata  0  0  0

[PATCH v2] Btrfs: fix memory leak in the extent_same ioctl

2015-07-03 Thread fdmanana
From: Filipe Manana fdman...@suse.com

We were allocating memory with memdup_user() but we were never releasing
that memory. This affected pretty much every call to the ioctl, whether
it deduplicated extents or not.

This issue was reported on IRC by Julian Taylor and on the mailing list
by Marcel Ritter, credit goes to them for finding the issue.

Reported-by: Julian Taylor jtaylor.deb...@googlemail.com
Reported-by: Marcel Ritter ritter.mar...@gmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Filipe Manana fdman...@suse.com
---

V2: Added cc to stable, forgotten in v1, and Julian's mail address.

 fs/btrfs/ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index c86b835..78e6a28 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2958,7 +2958,7 @@ out_unlock:
 static long btrfs_ioctl_file_extent_same(struct file *file,
struct btrfs_ioctl_same_args __user *argp)
 {
-   struct btrfs_ioctl_same_args *same;
+   struct btrfs_ioctl_same_args *same = NULL;
struct btrfs_ioctl_same_extent_info *info;
struct inode *src = file_inode(file);
u64 off;
@@ -2988,6 +2988,7 @@ static long btrfs_ioctl_file_extent_same(struct file 
*file,
 
if (IS_ERR(same)) {
ret = PTR_ERR(same);
+   same = NULL;
goto out;
}
 
@@ -3058,6 +3059,7 @@ static long btrfs_ioctl_file_extent_same(struct file 
*file,
 
 out:
mnt_drop_write_file(file);
+   kfree(same);
return ret;
 }
 
-- 
2.1.3

--
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


[PATCH] Btrfs: fix memory leak after block remove + trimming

2014-12-01 Thread Filipe Manana
There was a free space entry structure memeory leak if a block
group is remove while a free space entry is being trimmed, which
the following diagram explains:

   CPU 1  CPU 2

  btrfs_trim_block_group()
  trim_no_bitmap()
  remove free space entry from
  block group cache's rbtree
  do_trimming()

btrfs_remove_block_group()

btrfs_remove_free_space_cache()

  add back free space entry to
  block group's cache rbtree
  btrfs_put_block_group()

(...)
btrfs_put_block_group()

kfree(bg-free_space_ctl)
kfree(bg)

The free space entry added after doing the discard of its respective
range ends up never being freed.
Detected after doing an rmmod btrfs after running the stress test
recently submitted for fstests:

[ 8234.642212] kmem_cache_destroy btrfs_free_space: Slab cache still has objects
[ 8234.642657] CPU: 1 PID: 32276 Comm: rmmod Tainted: GWL 
3.17.0-rc5-btrfs-next-2+ #1
[ 8234.642660] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
[ 8234.642664]   8801af1b3eb8 8140c7b6 
8801dbedd0c0
[ 8234.642670]  8801af1b3ed0 811149ce  
8801af1b3ee0
[ 8234.642676]  a042dbe7 8801af1b3ef0 a0487422 
8801af1b3f78
[ 8234.642682] Call Trace:
[ 8234.642692]  [8140c7b6] dump_stack+0x4d/0x66
[ 8234.642699]  [811149ce] kmem_cache_destroy+0x4d/0x92
[ 8234.642731]  [a042dbe7] btrfs_destroy_cachep+0x63/0x76 [btrfs]
[ 8234.642757]  [a0487422] exit_btrfs_fs+0x9/0xbe7 [btrfs]
[ 8234.642762]  [810a76a5] SyS_delete_module+0x155/0x1c6
[ 8234.642768]  [8122a7eb] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 8234.642773]  [814122d2] system_call_fastpath+0x16/0x1b

This applies on top (depends on) of my previous patch titled:
Btrfs: fix race between fs trimming and block group remove/allocation

Signed-off-by: Filipe Manana fdman...@suse.com
---
 fs/btrfs/free-space-cache.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 2ee73c2..030847b 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -3200,6 +3200,12 @@ out:
/* once for us and once for the tree */
free_extent_map(em);
free_extent_map(em);
+
+   /*
+* We've left one free space entry and other tasks trimming
+* this block group have left 1 entry each one. Free them.
+*/
+   __btrfs_remove_free_space_cache(block_group-free_space_ctl);
} else {
spin_unlock(block_group-lock);
}
-- 
2.1.3

--
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


[PATCH 5/8] btrfs: fix memory leak when there is no more seed device

2014-08-13 Thread Anand Jain
When we replace all the seed device in the system there is
no point in just keeping the btrfs_fs_devices with out
any device

Signed-off-by: Anand Jain anand.j...@oracle.com
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6716658..314bbbf 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2013,6 +2013,8 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info 
*fs_info,
tmp_fs_devices = tmp_fs_devices-seed;
}
fs_devices-seed = NULL;
+   __btrfs_close_devices(fs_devices);
+   free_fs_devices(fs_devices);
}
 }
 
-- 
2.0.0.153.g79d

--
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


Re: [PATCH v6] Btrfs: fix memory leak of orphan block rsv

2014-06-18 Thread Alex Lyakas
Hi Filipe,
I finally got to debug this deeper. As it turns out, this happens only
if both nospace_cache and clear_cache are specified. You need to
unmount and mount again to cause this. After mounting, due to
clear_cache, all the block-groups are marked as BTRFS_DC_CLEAR, and
then cache_save_setup() is called on them (this function is called
only in case of BTRFS_DC_CLEAR). So cache_save_setup() goes ahead and
creates the free-space inode. But then it realizes that it was mounted
with nospace_cache, so it does not put any content in the inode. But
the inode itself gets created. The patch that fixes this for me:


alex@ubuntu-alex:/mnt/work/alex/linux-stable/source$ git diff -U10
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d170412..06f876e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2941,20 +2941,26 @@ again:
goto out;
}

if (IS_ERR(inode)) {
BUG_ON(retries);
retries++;

if (block_group-ro)
goto out_free;

+   /* with nospace_cache avoid creating the free-space inode */
+   if (!btrfs_test_opt(root, SPACE_CACHE)) {
+   dcs = BTRFS_DC_WRITTEN;
+   goto out_free;
+   }
+
ret = create_free_space_inode(root, trans, block_group, path);
if (ret)
goto out_free;
goto again;
}

/* We've already setup this transaction, go ahead and exit */
if (block_group-cache_generation == trans-transid 
i_size_read(inode)) {
dcs = BTRFS_DC_SETUP;



Thanks,
Alex.


On Wed, Nov 6, 2013 at 3:19 PM, Filipe David Manana fdman...@gmail.com wrote:
 On Mon, Nov 4, 2013 at 12:16 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hi Filipe,
 any luck with this patch?:)

 Hey Alex,

 I haven't digged further, but I remember I couldn't reproduce your
 issue (with latest btrfs-next of that day) of getting the free space
 inodes created even when mount option nospace_cache is given.

 What kernel were you using?


 Alex.

 On Wed, Oct 23, 2013 at 5:26 PM, Filipe David Manana fdman...@gmail.com 
 wrote:
 On Wed, Oct 23, 2013 at 3:14 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hello,

 On Wed, Oct 23, 2013 at 4:35 PM, Filipe David Manana fdman...@gmail.com 
 wrote:
 On Wed, Oct 23, 2013 at 2:33 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hi Filipe,


 On Tue, Aug 20, 2013 at 2:52 AM, Filipe David Borba Manana
 fdman...@gmail.com wrote:

 This issue is simple to reproduce and observe if kmemleak is enabled.
 Two simple ways to reproduce it:

 ** 1

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ btrfs balance start /mnt/btrfs
 $ umount /mnt/btrfs

 So here it seems that the leak can only happen in case the block-group
 has a free-space inode. This is what the orphan item is added for.
 Yes, here kmemleak reports.
 But: if space_cache option is disabled (and nospace_cache) enabled, it
 seems that btrfs still creates the FREE_SPACE inodes, although they
 are empty because in cache_save_setup:

 inode = lookup_free_space_inode(root, block_group, path);
 if (IS_ERR(inode)  PTR_ERR(inode) != -ENOENT) {
 ret = PTR_ERR(inode);
 btrfs_release_path(path);
 goto out;
 }

 if (IS_ERR(inode)) {
 ...
 ret = create_free_space_inode(root, trans, block_group, path);

 and only later it actually sets BTRFS_DC_WRITTEN if space_cache option
 is disabled. Amazing!
 Although this is a different issue, do you know perhaps why these
 empty inodes are needed?

 Don't know if they are needed. But you have a point, it seems odd to
 create the free space cache inode if mount option nospace_cache was
 supplied. Thanks Alex. Testing the following patch:

 diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
 index c43ee8a..eb1b7da 100644
 --- a/fs/btrfs/extent-tree.c
 +++ b/fs/btrfs/extent-tree.c
 @@ -3162,6 +3162,9 @@ static int cache_save_setup(struct
 btrfs_block_group_cache *block_group,
 int retries = 0;
 int ret = 0;

 +   if (!btrfs_test_opt(root, SPACE_CACHE))
 +   return 0;
 +
 /*
  * If this block group is smaller than 100 megs don't bother 
 caching the
  * block group.



 Thanks!
 Alex.




 ** 2

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ touch /mnt/btrfs/foobar
 $ rm -f /mnt/btrfs/foobar
 $ umount /mnt/btrfs


 I tried the second repro script on kernel 3.8.13, and kmemleak does
 not report a leak (even if I force the kmemleak scan). I did not try
 the balance-repro script, though. Am I missing something?

 Maybe it's not an issue on 3.8.13 and older releases.
 This was on btrfs-next from August 19.

 thanks for testing


 Thanks,
 Alex.




 After a while, kmemleak reports the leak:

 $ cat /sys/kernel/debug/kmemleak
 unreferenced object 

[PATCH] Btrfs: fix possible memory leak in btrfs_create_tree()

2014-04-08 Thread Tsutomu Itoh
In btrfs_create_tree(), if btrfs_insert_root() fails, we should
free root-commit_root.

Reported-by: Alex Lyakas a...@zadarastorage.com
Signed-off-by: Tsutomu Itoh t-i...@jp.fujitsu.com
---
 fs/btrfs/disk-io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6d1ac7d..a35d7fb 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1374,6 +1374,7 @@ struct btrfs_root *btrfs_create_tree(struct 
btrfs_trans_handle *trans,
 fail:
if (leaf) {
btrfs_tree_unlock(leaf);
+   free_extent_buffer(root-commit_root);
free_extent_buffer(leaf);
}
kfree(root);
-- 
1.8.5.5

--
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


Re: [PATCH] Btrfs: fix memory leak in btrfs_create_tree()

2014-03-31 Thread Tsutomu Itoh

Hi Alex,

On 2014/03/28 0:50, Alex Lyakas wrote:

Hi Tsutomu Itoh,

On Thu, Mar 21, 2013 at 6:32 AM, Tsutomu Itoh t-i...@jp.fujitsu.com wrote:

We should free leaf and root before returning from the error
handling code.

Signed-off-by: Tsutomu Itoh t-i...@jp.fujitsu.com
---
  fs/btrfs/disk-io.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7d84651..b1b5baa 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1291,6 +1291,7 @@ struct btrfs_root *btrfs_create_tree(struct 
btrfs_trans_handle *trans,
   0, objectid, NULL, 0, 0, 0);
 if (IS_ERR(leaf)) {
 ret = PTR_ERR(leaf);
+   leaf = NULL;
 goto fail;
 }

@@ -1334,11 +1335,16 @@ struct btrfs_root *btrfs_create_tree(struct 
btrfs_trans_handle *trans,

 btrfs_tree_unlock(leaf);

+   return root;
+
  fail:
-   if (ret)
-   return ERR_PTR(ret);
+   if (leaf) {
+   btrfs_tree_unlock(leaf);
+   free_extent_buffer(leaf);

I believe this is not enough. Few lines above, another reference on
the root is taken by
root-commit_root = btrfs_root_node(root);


Thank you for pointing this out.

You are right.
Could you re-post your fix by the patch submitting form?

Thanks,
Tsutomu



So I believe the proper fix would be:
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d9698fd..260af79 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1354,10 +1354,10 @@ struct btrfs_root *btrfs_create_tree(struct
btrfs_trans_handle *trans,
 return root;

  fail:
-   if (leaf) {
+   if (leaf)
 btrfs_tree_unlock(leaf);
-   free_extent_buffer(leaf);
-   }
+   free_extent_buffer(root-node);
+   free_extent_buffer(root-commit_root);
 kfree(root);

 return ERR_PTR(ret);



Thanks,
Alex.




+   }
+   kfree(root);

-   return root;
+   return ERR_PTR(ret);
  }

  static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,




--
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


Re: [PATCH] Btrfs: fix memory leak in btrfs_create_tree()

2014-03-27 Thread Alex Lyakas
Hi Tsutomu Itoh,

On Thu, Mar 21, 2013 at 6:32 AM, Tsutomu Itoh t-i...@jp.fujitsu.com wrote:
 We should free leaf and root before returning from the error
 handling code.

 Signed-off-by: Tsutomu Itoh t-i...@jp.fujitsu.com
 ---
  fs/btrfs/disk-io.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

 diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
 index 7d84651..b1b5baa 100644
 --- a/fs/btrfs/disk-io.c
 +++ b/fs/btrfs/disk-io.c
 @@ -1291,6 +1291,7 @@ struct btrfs_root *btrfs_create_tree(struct 
 btrfs_trans_handle *trans,
   0, objectid, NULL, 0, 0, 0);
 if (IS_ERR(leaf)) {
 ret = PTR_ERR(leaf);
 +   leaf = NULL;
 goto fail;
 }

 @@ -1334,11 +1335,16 @@ struct btrfs_root *btrfs_create_tree(struct 
 btrfs_trans_handle *trans,

 btrfs_tree_unlock(leaf);

 +   return root;
 +
  fail:
 -   if (ret)
 -   return ERR_PTR(ret);
 +   if (leaf) {
 +   btrfs_tree_unlock(leaf);
 +   free_extent_buffer(leaf);
I believe this is not enough. Few lines above, another reference on
the root is taken by
root-commit_root = btrfs_root_node(root);

So I believe the proper fix would be:
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d9698fd..260af79 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1354,10 +1354,10 @@ struct btrfs_root *btrfs_create_tree(struct
btrfs_trans_handle *trans,
return root;

 fail:
-   if (leaf) {
+   if (leaf)
btrfs_tree_unlock(leaf);
-   free_extent_buffer(leaf);
-   }
+   free_extent_buffer(root-node);
+   free_extent_buffer(root-commit_root);
kfree(root);

return ERR_PTR(ret);



Thanks,
Alex.



 +   }
 +   kfree(root);

 -   return root;
 +   return ERR_PTR(ret);
  }

  static struct btrfs_root *alloc_log_tree(struct btrfs_trans_handle *trans,

 --
 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
--
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


[PATCH] Btrfs-progs: fsck: fix memory leak and unnecessary call to free

2014-03-15 Thread Rakesh Pandit
Free already allocated memory to item1_data if malloc fails for
item2_data in swap_values. Seems to be a typo from commit 70749a77.

Signed-off-by: Rakesh Pandit rak...@tuxera.com
---
 cmds-check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index d1cafe1..60708d0 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -2380,7 +2380,7 @@ static int swap_values(struct btrfs_root *root, struct 
btrfs_path *path,
return -ENOMEM;
item2_data = malloc(item2_size);
if (!item2_data) {
-   free(item2_data);
+   free(item1_data);
return -ENOMEM;
}
 
-- 
1.8.5.3

--
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


Re: [PATCH v6] Btrfs: fix memory leak of orphan block rsv

2013-11-06 Thread Filipe David Manana
On Mon, Nov 4, 2013 at 12:16 PM, Alex Lyakas
alex.bt...@zadarastorage.com wrote:
 Hi Filipe,
 any luck with this patch?:)

Hey Alex,

I haven't digged further, but I remember I couldn't reproduce your
issue (with latest btrfs-next of that day) of getting the free space
inodes created even when mount option nospace_cache is given.

What kernel were you using?


 Alex.

 On Wed, Oct 23, 2013 at 5:26 PM, Filipe David Manana fdman...@gmail.com 
 wrote:
 On Wed, Oct 23, 2013 at 3:14 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hello,

 On Wed, Oct 23, 2013 at 4:35 PM, Filipe David Manana fdman...@gmail.com 
 wrote:
 On Wed, Oct 23, 2013 at 2:33 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hi Filipe,


 On Tue, Aug 20, 2013 at 2:52 AM, Filipe David Borba Manana
 fdman...@gmail.com wrote:

 This issue is simple to reproduce and observe if kmemleak is enabled.
 Two simple ways to reproduce it:

 ** 1

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ btrfs balance start /mnt/btrfs
 $ umount /mnt/btrfs

 So here it seems that the leak can only happen in case the block-group
 has a free-space inode. This is what the orphan item is added for.
 Yes, here kmemleak reports.
 But: if space_cache option is disabled (and nospace_cache) enabled, it
 seems that btrfs still creates the FREE_SPACE inodes, although they
 are empty because in cache_save_setup:

 inode = lookup_free_space_inode(root, block_group, path);
 if (IS_ERR(inode)  PTR_ERR(inode) != -ENOENT) {
 ret = PTR_ERR(inode);
 btrfs_release_path(path);
 goto out;
 }

 if (IS_ERR(inode)) {
 ...
 ret = create_free_space_inode(root, trans, block_group, path);

 and only later it actually sets BTRFS_DC_WRITTEN if space_cache option
 is disabled. Amazing!
 Although this is a different issue, do you know perhaps why these
 empty inodes are needed?

 Don't know if they are needed. But you have a point, it seems odd to
 create the free space cache inode if mount option nospace_cache was
 supplied. Thanks Alex. Testing the following patch:

 diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
 index c43ee8a..eb1b7da 100644
 --- a/fs/btrfs/extent-tree.c
 +++ b/fs/btrfs/extent-tree.c
 @@ -3162,6 +3162,9 @@ static int cache_save_setup(struct
 btrfs_block_group_cache *block_group,
 int retries = 0;
 int ret = 0;

 +   if (!btrfs_test_opt(root, SPACE_CACHE))
 +   return 0;
 +
 /*
  * If this block group is smaller than 100 megs don't bother caching 
 the
  * block group.



 Thanks!
 Alex.




 ** 2

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ touch /mnt/btrfs/foobar
 $ rm -f /mnt/btrfs/foobar
 $ umount /mnt/btrfs


 I tried the second repro script on kernel 3.8.13, and kmemleak does
 not report a leak (even if I force the kmemleak scan). I did not try
 the balance-repro script, though. Am I missing something?

 Maybe it's not an issue on 3.8.13 and older releases.
 This was on btrfs-next from August 19.

 thanks for testing


 Thanks,
 Alex.




 After a while, kmemleak reports the leak:

 $ cat /sys/kernel/debug/kmemleak
 unreferenced object 0x880402b13e00 (size 128):
   comm btrfs, pid 19621, jiffies 4341648183 (age 70057.844s)
   hex dump (first 32 bytes):
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
 00 fc c6 b1 04 88 ff ff 04 00 04 00 ad 4e ad de  .N..
   backtrace:
 [817275a6] kmemleak_alloc+0x26/0x50
 [8117832b] kmem_cache_alloc_trace+0xeb/0x1d0
 [a04db499] btrfs_alloc_block_rsv+0x39/0x70 [btrfs]
 [a04f8bad] btrfs_orphan_add+0x13d/0x1b0 [btrfs]
 [a04e2b13] btrfs_remove_block_group+0x143/0x500 [btrfs]
 [a0518158] btrfs_relocate_chunk.isra.63+0x618/0x790 [btrfs]
 [a051bc27] btrfs_balance+0x8f7/0xe90 [btrfs]
 [a05240a0] btrfs_ioctl_balance+0x250/0x550 [btrfs]
 [a05269ca] btrfs_ioctl+0xdfa/0x25f0 [btrfs]
 [8119c936] do_vfs_ioctl+0x96/0x570
 [8119cea1] SyS_ioctl+0x91/0xb0
 [81750242] system_call_fastpath+0x16/0x1b
 [] 0x

 This affects btrfs-next, revision 
 be8e3cd00d7293dd177e3f8a4a1645ce09ca3acb
 (Btrfs: separate out tests into their own directory).

 Signed-off-by: Filipe David Borba Manana fdman...@gmail.com
 ---

 V2: removed atomic_t member in struct btrfs_block_rsv, as suggested by
 Josef Bacik, and use instead the condition reserved == 0 to decide
 when to free the block.
 V3: simplified patch, just kfree() (and not btrfs_free_block_rsv) the
 root's orphan_block_rsv when free'ing the root. Thanks Josef for
 the suggestion.
 V4: use btrfs_free_block_rsv() instead of kfree(). The error I was 
 getting
 in xfstests when using btrfs_free_block_rsv() was unrelated, Josef 
 just
 pointed it to me (separate issue).
 V5: move the free call below the iput() call, 

Re: [PATCH v6] Btrfs: fix memory leak of orphan block rsv

2013-11-04 Thread Alex Lyakas
Hi Filipe,
any luck with this patch?:)

Alex.

On Wed, Oct 23, 2013 at 5:26 PM, Filipe David Manana fdman...@gmail.com wrote:
 On Wed, Oct 23, 2013 at 3:14 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hello,

 On Wed, Oct 23, 2013 at 4:35 PM, Filipe David Manana fdman...@gmail.com 
 wrote:
 On Wed, Oct 23, 2013 at 2:33 PM, Alex Lyakas
 alex.bt...@zadarastorage.com wrote:
 Hi Filipe,


 On Tue, Aug 20, 2013 at 2:52 AM, Filipe David Borba Manana
 fdman...@gmail.com wrote:

 This issue is simple to reproduce and observe if kmemleak is enabled.
 Two simple ways to reproduce it:

 ** 1

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ btrfs balance start /mnt/btrfs
 $ umount /mnt/btrfs

 So here it seems that the leak can only happen in case the block-group
 has a free-space inode. This is what the orphan item is added for.
 Yes, here kmemleak reports.
 But: if space_cache option is disabled (and nospace_cache) enabled, it
 seems that btrfs still creates the FREE_SPACE inodes, although they
 are empty because in cache_save_setup:

 inode = lookup_free_space_inode(root, block_group, path);
 if (IS_ERR(inode)  PTR_ERR(inode) != -ENOENT) {
 ret = PTR_ERR(inode);
 btrfs_release_path(path);
 goto out;
 }

 if (IS_ERR(inode)) {
 ...
 ret = create_free_space_inode(root, trans, block_group, path);

 and only later it actually sets BTRFS_DC_WRITTEN if space_cache option
 is disabled. Amazing!
 Although this is a different issue, do you know perhaps why these
 empty inodes are needed?

 Don't know if they are needed. But you have a point, it seems odd to
 create the free space cache inode if mount option nospace_cache was
 supplied. Thanks Alex. Testing the following patch:

 diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
 index c43ee8a..eb1b7da 100644
 --- a/fs/btrfs/extent-tree.c
 +++ b/fs/btrfs/extent-tree.c
 @@ -3162,6 +3162,9 @@ static int cache_save_setup(struct
 btrfs_block_group_cache *block_group,
 int retries = 0;
 int ret = 0;

 +   if (!btrfs_test_opt(root, SPACE_CACHE))
 +   return 0;
 +
 /*
  * If this block group is smaller than 100 megs don't bother caching 
 the
  * block group.



 Thanks!
 Alex.




 ** 2

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ touch /mnt/btrfs/foobar
 $ rm -f /mnt/btrfs/foobar
 $ umount /mnt/btrfs


 I tried the second repro script on kernel 3.8.13, and kmemleak does
 not report a leak (even if I force the kmemleak scan). I did not try
 the balance-repro script, though. Am I missing something?

 Maybe it's not an issue on 3.8.13 and older releases.
 This was on btrfs-next from August 19.

 thanks for testing


 Thanks,
 Alex.




 After a while, kmemleak reports the leak:

 $ cat /sys/kernel/debug/kmemleak
 unreferenced object 0x880402b13e00 (size 128):
   comm btrfs, pid 19621, jiffies 4341648183 (age 70057.844s)
   hex dump (first 32 bytes):
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
 00 fc c6 b1 04 88 ff ff 04 00 04 00 ad 4e ad de  .N..
   backtrace:
 [817275a6] kmemleak_alloc+0x26/0x50
 [8117832b] kmem_cache_alloc_trace+0xeb/0x1d0
 [a04db499] btrfs_alloc_block_rsv+0x39/0x70 [btrfs]
 [a04f8bad] btrfs_orphan_add+0x13d/0x1b0 [btrfs]
 [a04e2b13] btrfs_remove_block_group+0x143/0x500 [btrfs]
 [a0518158] btrfs_relocate_chunk.isra.63+0x618/0x790 [btrfs]
 [a051bc27] btrfs_balance+0x8f7/0xe90 [btrfs]
 [a05240a0] btrfs_ioctl_balance+0x250/0x550 [btrfs]
 [a05269ca] btrfs_ioctl+0xdfa/0x25f0 [btrfs]
 [8119c936] do_vfs_ioctl+0x96/0x570
 [8119cea1] SyS_ioctl+0x91/0xb0
 [81750242] system_call_fastpath+0x16/0x1b
 [] 0x

 This affects btrfs-next, revision be8e3cd00d7293dd177e3f8a4a1645ce09ca3acb
 (Btrfs: separate out tests into their own directory).

 Signed-off-by: Filipe David Borba Manana fdman...@gmail.com
 ---

 V2: removed atomic_t member in struct btrfs_block_rsv, as suggested by
 Josef Bacik, and use instead the condition reserved == 0 to decide
 when to free the block.
 V3: simplified patch, just kfree() (and not btrfs_free_block_rsv) the
 root's orphan_block_rsv when free'ing the root. Thanks Josef for
 the suggestion.
 V4: use btrfs_free_block_rsv() instead of kfree(). The error I was getting
 in xfstests when using btrfs_free_block_rsv() was unrelated, Josef 
 just
 pointed it to me (separate issue).
 V5: move the free call below the iput() call, so that btrfs_evict_node()
 can process the orphan_block_rsv first to do some needed cleanup 
 before
 we free it.
 V6: free the root's orphan_block_rsv in close_ctree() too. After a balance
 the orphan_block_rsv of the tree of tree roots was being leaked, 
 because
 free_fs_root() is only called for filesystem 

Re: memory leak in =3.11.6

2013-10-30 Thread Duncan
Kai Krakow posted on Wed, 30 Oct 2013 00:30:26 +0100 as excerpted:

 For me it started late 3.9 if I remember right. But I'm pretty sure it
 startet with 3.10 which I mostly upgraded to for using skinny extents.
 While skinny extents has helped subjective performance a little bit I
 since experience the problem with ever increasing RAM usage - up to the
 point of 15 GB swap, full RAM, and almost none of it is used for
 caching.

That's an interesting point.  I've been cautious about enabling skinny 
extents as well, and haven't enabled them here.  (Nothing major, just a 
couple early reports that I expect have long been worked out by now, but 
I decided giving them a few more kernel releases to mature was probably a 
good idea.)

So it'd be interesting to see if there's a correlation between skinny 
extents and the memory issues as well, as there seems to be between 
qgroups and the memory issues.

-- 
Duncan - List replies preferred.   No HTML msgs.
Every nonfree program has a lord, a master --
and if you use the program, he is your master.  Richard Stallman

--
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


Re: memory leak in =3.11.6

2013-10-29 Thread Duncan
Jérôme Poulin posted on Mon, 28 Oct 2013 23:24:10 -0400 as excerpted:

 On Mon, Oct 28, 2013 at 6:34 PM, Kai Krakow hurikhan77+bt...@gmail.com
 wrote:

 If I close all my desktop programs, my system stays at +12 GB RAM usage
 while after a fresh boot it has 12+ GB free (of 16 GB). Cache stays low
 at 2 GB while after a few minutes uptime cache is about 5 GB.
 
 I probably have the same problem over here, after about 2 weeks of
 random read/write it seems my memory and swap get almost full and even
 after killing all process and getting in single user mode, memory won't
 free up.  Would you happen to have quota enabled too? Kernel is 3.11.4

FWIW, seeing nothing like that here, no quota/qgroups enabled.

But there's definitely something not right with qgroups at this time.  
First, a lot of folks with it enabled are reporting negative numbers 
which shouldn't be there, and second, all or very nearly all the huge 
memory usage issues reported seem to be from qgroups-enabled people.

So I'd call the qgroups feature broken at this time.  Don't use it if you 
can avoid it (but be aware that turning it off triggers the memory issue 
and ultimate crashing for some, so preferably turn it off when you're 
rebooting anyway), and if you really do need quotas for your use-case, 
you may be better off on a non-btrfs filesystem for the time being.

(Of course btrfs is still listed as experimental in any case, so one 
shouldn't be surprised if this sort of thing happens from time to time or 
with some features.)

-- 
Duncan - List replies preferred.   No HTML msgs.
Every nonfree program has a lord, a master --
and if you use the program, he is your master.  Richard Stallman

--
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


Re: memory leak in =3.11.6

2013-10-29 Thread Jérôme Poulin
On Tue, Oct 29, 2013 at 12:42 PM, Duncan 1i5t5.dun...@cox.net wrote:

 So I'd call the qgroups feature broken at this time

I agree. I made a page about Quotas on the wiki so to document my
findings as documentation is really sparse. If you consult the section
Known issues, you will find those:

* To get accurate information, you must issue a sync before using the
qgroup show command.
* The qgroup show command is missing some information, for example you
cannot see which subvolume is part of a parent qgroup.
* Creating a qgroup from an existing directory will show a 0 usage
until a full filesystem quota rescan is issued.
* Using btrfs subvolume delete will break qgroup unshared space usage.
* After deleting a subvolume, you must manually delete the associated qgroup.

Knowing this, if you create a qgroup after some files have been added
to a subvolume, those never get accounted for until you rescan the
whole volume. This means that if you manage to delete enough not
accounted for files, you get negative numbers.

Wiki: https://btrfs.wiki.kernel.org/index.php/Quota_support
--
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


Re: memory leak in =3.11.6

2013-10-29 Thread Kai Krakow
Jérôme Poulin jeromepou...@gmail.com schrieb:

 If I close all my desktop programs, my system stays at +12 GB RAM usage
 while after a fresh boot it has 12+ GB free (of 16 GB). Cache stays low
 at 2 GB while after a few minutes uptime cache is about 5 GB.
 
 I probably have the same problem over here, after about 2 weeks of
 random read/write it seems my memory and swap get almost full and even
 after killing all process and getting in single user mode, memory
 won't free up.  Would you happen to have quota enabled too? Kernel is
 3.11.4

No, quota is off for me. This is a three-device btrfs desktop system. Quota 
has no real use for me (except maybe for subvolume accounting).

I first thought this may be related to having zcache enabled, turned it off. 
But anything that changed is when memory stress arises my system stays more 
responsible and does not go havoc on the CPU.

I switched to zswap instead, since then swap usage is much lower. But still 
it does not help if memory is mysteriously used somewhere. At least IO 
stress is lower now with that setting and I am better able to cleanly shut 
down programs and the system for reboot.

Regards,
Kai

--
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


Re: memory leak in =3.11.6

2013-10-29 Thread Kai Krakow
Tomasz Chmielewski t...@virtall.com schrieb:

 I probably have the same problem over here, after about 2 weeks of
 random read/write it seems my memory and swap get almost full and even
 after killing all process and getting in single user mode, memory
 won't free up.  Would you happen to have quota enabled too? Kernel is
 3.11.4
 
 I experience the same daily (out of memory, server dying); sometimes
 several times daily.
 
 It's a *big* downer.
 
 I use 3.12-rc7, but I had this issue with earlier kernels, too.

For me it started late 3.9 if I remember right. But I'm pretty sure it 
startet with 3.10 which I mostly upgraded to for using skinny extents. While 
skinny extents has helped subjective performance a little bit I since 
experience the problem with ever increasing RAM usage - up to the point of 
15 GB swap, full RAM, and almost none of it is used for caching.

I wonder if there is a way to track where this lost memory is gone. Top 
shows no memory hogs. Slabtop shows some high numbers but nothing really 
impressive. It's just used for nothing it seems.

Regards,
Kai

--
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


Re: memory leak in =3.11.6

2013-10-29 Thread Wang Shilong

On 10/30/2013 06:46 AM, Jérôme Poulin wrote:

On Tue, Oct 29, 2013 at 12:42 PM, Duncan 1i5t5.dun...@cox.net wrote:

So I'd call the qgroups feature broken at this time

I agree. I made a page about Quotas on the wiki so to document my
findings as documentation is really sparse. If you consult the section
Known issues, you will find those:

* To get accurate information, you must issue a sync before using the
qgroup show command.

This is true in some extent.

qgroup accounting info(stored in disk) only update when commting
transaction.

Anyway, by triggering a sync is a little overhead. maybe we can
implement an sync ioctl for quota sync.

* The qgroup show command is missing some information, for example you
cannot see which subvolume is part of a parent qgroup.
This has been implemented in the latest chris's integration branch,you 
can use


btrfs qgroup show -p mnt

* Creating a qgroup from an existing directory will show a 0 usage
until a full filesystem quota rescan is issued

In the latest kernel. if you enable quota, it will create qgroup
for every existed subvolume, and then rescan will be triggered
automatically.(only happens in rescanning)

When creating a subvolume, you can use
btrfs sub create -i pid mnt

this will add a parent qgroup for a subvolume the moment it was created,
we can avoid rescanning for this case.

However, if you create a parent qgroup and then assign subvolume qgroup
to this parent qgroup online, you have to rescan.
  
* Using btrfs subvolume delete will break qgroup unshared space usage.

* After deleting a subvolume, you must manually delete the associated qgroup.
This is true, for now, deleting a subvolume will cause wrong qgroup 
accounting.
Jan sent a patch for this recently, hopely this problem can be solved as 
soon as

possible.

I have planed to make a patch to drop associated qgroup when deleting a 
subvolume

only if qgroup accounting problem has been fixed.



Knowing this, if you create a qgroup after some files have been added
to a subvolume, those never get accounted for until you rescan the
whole volume. This means that if you manage to delete enough not
accounted for files, you get negative numbers.

Wiki: https://btrfs.wiki.kernel.org/index.php/Quota_support
--
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



--
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


memory leak in =3.11.6

2013-10-28 Thread Kai Krakow
Hello!

Is this output in slabtop normal after a few days uptime with a daily rsync 
(btrfs to btrfs) and some compiling (gentoo emerge):

 Active / Total Objects (% used): 1120836 / 1810907 (61,9%)
 Active / Total Slabs (% used)  : 42492 / 42492 (100,0%)
 Active / Total Caches (% used) : 72 / 101 (71,3%)
 Active / Total Size (% used)   : 253898,56K / 324674,92K (78,2%)
 Minimum / Average / Maximum Object : 0,01K / 0,18K / 15,69K

  OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME   
109220  99107  90%0,98K   6830   16109280K btrfs_inode

If I close all my desktop programs, my system stays at +12 GB RAM usage 
while after a fresh boot it has 12+ GB free (of 16 GB). Cache stays low at 2 
GB while after a few minutes uptime cache is about 5 GB.

This looks like memory leaking somewhere. Top does not show where all this 
memory went (biggest process is 500 MB RES, next is 200 MB and lower).

Thanks,
Kai

--
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


Re: memory leak in =3.11.6

2013-10-28 Thread Jérôme Poulin
On Mon, Oct 28, 2013 at 6:34 PM, Kai Krakow hurikhan77+bt...@gmail.com wrote:

 If I close all my desktop programs, my system stays at +12 GB RAM usage
 while after a fresh boot it has 12+ GB free (of 16 GB). Cache stays low at 2
 GB while after a few minutes uptime cache is about 5 GB.

I probably have the same problem over here, after about 2 weeks of
random read/write it seems my memory and swap get almost full and even
after killing all process and getting in single user mode, memory
won't free up.  Would you happen to have quota enabled too? Kernel is
3.11.4
--
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


Re: memory leak in =3.11.6

2013-10-28 Thread Tomasz Chmielewski
 I probably have the same problem over here, after about 2 weeks of
 random read/write it seems my memory and swap get almost full and even
 after killing all process and getting in single user mode, memory
 won't free up.  Would you happen to have quota enabled too? Kernel is
 3.11.4

I experience the same daily (out of memory, server dying); sometimes
several times daily.

It's a *big* downer.

I use 3.12-rc7, but I had this issue with earlier kernels, too.

Yes, I have qgroups enabled.


A bit desperate, I've started dumping various system parameters (free,
uptime, ps, top, /proc/meminfo etc.) every minute - memory usage can
jump from some 8 GB to 32 GB within one minute; a minute later the
system is no longer responsive and has to be hard resetted.


Example - free-2013-10-29-03:29:01

 total   used   free sharedbuffers cached
Mem:  32638388   32484484 153904  0   4960   24122452
-/+ buffers/cache:8357072   24281316
Swap: 16776116132   16775984


free-2013-10-29-03:30:01

 total   used   free sharedbuffers cached
Mem:  32638388   32473636 164752  05441191896
-/+ buffers/cache:   312811961357192
Swap: 16776116408   16775708



What would cause such a huge memory usage jump?

top for the same two minutes; no userspace process is using the memory.
First one - we're using ~8 GB RAM, the second one - all 32 GB is used.

top-2013-10-29-03:29:01:

top - 03:29:01 up  2:00,  1 user,  load average: 9.00, 8.48, 8.82
Tasks: 268 total,   1 running, 267 sleeping,   0 stopped,   0 zombie
%Cpu(s):  0.4 us,  0.9 sy,  0.2 ni, 77.2 id, 21.2 wa,  0.0 hi,  0.1 si,  0.0 st
KiB Mem:  32638388 total, 32485196 used,   153192 free, 4964 buffers
KiB Swap: 16776116 total,  132 used, 16775984 free, 24121388 cached

  PID USER  PR  NI  VIRT  RES  SHR S  %CPU %MEMTIME+  COMMAND
15568 root  20   0 99220  71m 1144 D   6.3  0.2   0:27.59 rsync --exclude=/
16384 root  20   0 000 S   6.3  0.0   0:11.12 [btrfs-endio-met]
17016 root  20   0 000 S   6.3  0.0   0:02.89 [btrfs-endio-met]
19956 root  20   0  147m  46m  984 D   6.3  0.1   0:19.60 rsync --delete --
21798 root  20   0 48268  13m 1132 D   6.3  0.0   0:07.61 rsync --exclude=/
21824 root  20   0 24696 1568 1096 R   6.3  0.0   0:00.01 top -b -c -n 1
1 root  20   0 10644  912  764 S   0.0  0.0   0:00.46 init [2]
2 root  20   0 000 S   0.0  0.0   0:00.03 [kthreadd]
3 root  20   0 000 S   0.0  0.0   0:00.22 [ksoftirqd/0]
4 root  20   0 000 S   0.0  0.0   0:00.00 [kworker/0:0]
5 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/0:0H]
6 root  20   0 000 S   0.0  0.0   0:04.01 [kworker/u16:0]
7 root  rt   0 000 S   0.0  0.0   0:00.43 [migration/0]
8 root  20   0 000 S   0.0  0.0   0:00.00 [rcu_bh]
9 root  20   0 000 S   0.0  0.0   0:09.07 [rcu_sched]
   10 root  rt   0 000 S   0.0  0.0   0:00.02 [watchdog/0]
   11 root  rt   0 000 S   0.0  0.0   0:00.03 [watchdog/1]
   12 root  rt   0 000 S   0.0  0.0   0:00.00 [migration/1]
   13 root  20   0 000 S   0.0  0.0   0:00.10 [ksoftirqd/1]
   14 root  20   0 000 S   0.0  0.0   0:00.00 [kworker/1:0]
   15 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/1:0H]
   16 root  rt   0 000 S   0.0  0.0   0:00.03 [watchdog/2]
   17 root  rt   0 000 S   0.0  0.0   0:00.00 [migration/2]
   18 root  20   0 000 S   0.0  0.0   0:00.09 [ksoftirqd/2]
   20 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/2:0H]
   21 root  rt   0 000 S   0.0  0.0   0:00.02 [watchdog/3]
   22 root  rt   0 000 S   0.0  0.0   0:00.01 [migration/3]
   23 root  20   0 000 S   0.0  0.0   0:00.10 [ksoftirqd/3]
   24 root  20   0 000 S   0.0  0.0   0:00.00 [kworker/3:0]
   25 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/3:0H]
   26 root  rt   0 000 S   0.0  0.0   0:00.03 [watchdog/4]
   27 root  rt   0 000 S   0.0  0.0   0:00.02 [migration/4]
   28 root  20   0 000 S   0.0  0.0   0:00.01 [ksoftirqd/4]
   29 root  20   0 000 S   0.0  0.0   0:00.00 [kworker/4:0]
   30 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/4:0H]
   31 root  rt   0 000 S   0.0  0.0   0:00.02 [watchdog/5]
   32 root  rt   0 000 S   0.0  0.0   0:00.02 [migration/5]
   33 root  20   0 000 S   0.0  0.0   0:00.01 [ksoftirqd/5]
   34 root  20   0 000 S   0.0  0.0   0:00.00 [kworker/5:0]
   35 root   0 -20 000 S   0.0  0.0   0:00.00 [kworker/5:0H]
   36 

Re: [PATCH v6] Btrfs: fix memory leak of orphan block rsv

2013-10-23 Thread Alex Lyakas
Hi Filipe,


On Tue, Aug 20, 2013 at 2:52 AM, Filipe David Borba Manana
fdman...@gmail.com wrote:

 This issue is simple to reproduce and observe if kmemleak is enabled.
 Two simple ways to reproduce it:

 ** 1

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ btrfs balance start /mnt/btrfs
 $ umount /mnt/btrfs

 ** 2

 $ mkfs.btrfs -f /dev/loop0
 $ mount /dev/loop0 /mnt/btrfs
 $ touch /mnt/btrfs/foobar
 $ rm -f /mnt/btrfs/foobar
 $ umount /mnt/btrfs


I tried the second repro script on kernel 3.8.13, and kmemleak does
not report a leak (even if I force the kmemleak scan). I did not try
the balance-repro script, though. Am I missing something?

Thanks,
Alex.




 After a while, kmemleak reports the leak:

 $ cat /sys/kernel/debug/kmemleak
 unreferenced object 0x880402b13e00 (size 128):
   comm btrfs, pid 19621, jiffies 4341648183 (age 70057.844s)
   hex dump (first 32 bytes):
 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
 00 fc c6 b1 04 88 ff ff 04 00 04 00 ad 4e ad de  .N..
   backtrace:
 [817275a6] kmemleak_alloc+0x26/0x50
 [8117832b] kmem_cache_alloc_trace+0xeb/0x1d0
 [a04db499] btrfs_alloc_block_rsv+0x39/0x70 [btrfs]
 [a04f8bad] btrfs_orphan_add+0x13d/0x1b0 [btrfs]
 [a04e2b13] btrfs_remove_block_group+0x143/0x500 [btrfs]
 [a0518158] btrfs_relocate_chunk.isra.63+0x618/0x790 [btrfs]
 [a051bc27] btrfs_balance+0x8f7/0xe90 [btrfs]
 [a05240a0] btrfs_ioctl_balance+0x250/0x550 [btrfs]
 [a05269ca] btrfs_ioctl+0xdfa/0x25f0 [btrfs]
 [8119c936] do_vfs_ioctl+0x96/0x570
 [8119cea1] SyS_ioctl+0x91/0xb0
 [81750242] system_call_fastpath+0x16/0x1b
 [] 0x

 This affects btrfs-next, revision be8e3cd00d7293dd177e3f8a4a1645ce09ca3acb
 (Btrfs: separate out tests into their own directory).

 Signed-off-by: Filipe David Borba Manana fdman...@gmail.com
 ---

 V2: removed atomic_t member in struct btrfs_block_rsv, as suggested by
 Josef Bacik, and use instead the condition reserved == 0 to decide
 when to free the block.
 V3: simplified patch, just kfree() (and not btrfs_free_block_rsv) the
 root's orphan_block_rsv when free'ing the root. Thanks Josef for
 the suggestion.
 V4: use btrfs_free_block_rsv() instead of kfree(). The error I was getting
 in xfstests when using btrfs_free_block_rsv() was unrelated, Josef just
 pointed it to me (separate issue).
 V5: move the free call below the iput() call, so that btrfs_evict_node()
 can process the orphan_block_rsv first to do some needed cleanup before
 we free it.
 V6: free the root's orphan_block_rsv in close_ctree() too. After a balance
 the orphan_block_rsv of the tree of tree roots was being leaked, because
 free_fs_root() is only called for filesystem trees.

  fs/btrfs/disk-io.c |5 +
  1 file changed, 5 insertions(+)

 diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
 index 3b12c26..5d17163 100644
 --- a/fs/btrfs/disk-io.c
 +++ b/fs/btrfs/disk-io.c
 @@ -3430,6 +3430,8 @@ static void free_fs_root(struct btrfs_root *root)
  {
 iput(root-cache_inode);
 WARN_ON(!RB_EMPTY_ROOT(root-inode_tree));
 +   btrfs_free_block_rsv(root, root-orphan_block_rsv);
 +   root-orphan_block_rsv = NULL;
 if (root-anon_dev)
 free_anon_bdev(root-anon_dev);
 free_extent_buffer(root-node);
 @@ -3582,6 +3584,9 @@ int close_ctree(struct btrfs_root *root)

 btrfs_free_stripe_hash_table(fs_info);

 +   btrfs_free_block_rsv(root, root-orphan_block_rsv);
 +   root-orphan_block_rsv = NULL;
 +
 return 0;
  }

 --
 1.7.9.5

 --
 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
--
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


  1   2   3   >