2008-02-14 10:31:32

by Aneesh Kumar K.V

[permalink] [raw]
Subject: [RFC/PATCH] ext4: Fix the memory leak and buffer head leak with respect to ext4_ext_path

The path variable returned via ext4_ext_find_extent is a kmalloc variable
and need to be freeded. It also contain refrences to buffer_head which need
to be dropped.

Signed-off-by: Aneesh Kumar K.V <[email protected]>
---
fs/ext4/extents.c | 6 +++---
fs/ext4/migrate.c | 5 +++++
include/linux/ext4_fs_extents.h | 1 +
3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e856f66..995ac16 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -349,7 +349,7 @@ static void ext4_ext_show_leaf(struct inode *inode, struct ext4_ext_path *path)
#define ext4_ext_show_leaf(inode,path)
#endif

-static void ext4_ext_drop_refs(struct ext4_ext_path *path)
+void ext4_ext_drop_refs(struct ext4_ext_path *path)
{
int depth = path->p_depth;
int i;
@@ -2200,10 +2200,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
newdepth = ext_depth(inode);
if (newdepth != depth) {
depth = newdepth;
- path = ext4_ext_find_extent(inode, iblock, NULL);
+ ext4_ext_drop_refs(path);
+ path = ext4_ext_find_extent(inode, iblock, path);
if (IS_ERR(path)) {
err = PTR_ERR(path);
- path = NULL;
goto out;
}
eh = path[depth].p_hdr;
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 8c6c685..5c1e27d 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -43,6 +43,7 @@ static int finish_range(handle_t *handle, struct inode *inode,

if (IS_ERR(path)) {
retval = PTR_ERR(path);
+ path = NULL;
goto err_out;
}

@@ -74,6 +75,10 @@ static int finish_range(handle_t *handle, struct inode *inode,
}
retval = ext4_ext_insert_extent(handle, inode, path, &newext);
err_out:
+ if (path) {
+ ext4_ext_drop_refs(path);
+ kfree(path);
+ }
lb->first_pblock = 0;
return retval;
}
diff --git a/include/linux/ext4_fs_extents.h b/include/linux/ext4_fs_extents.h
index 697da4b..1285c58 100644
--- a/include/linux/ext4_fs_extents.h
+++ b/include/linux/ext4_fs_extents.h
@@ -227,5 +227,6 @@ extern int ext4_ext_search_left(struct inode *, struct ext4_ext_path *,
ext4_lblk_t *, ext4_fsblk_t *);
extern int ext4_ext_search_right(struct inode *, struct ext4_ext_path *,
ext4_lblk_t *, ext4_fsblk_t *);
+extern void ext4_ext_drop_refs(struct ext4_ext_path *);
#endif /* _LINUX_EXT4_EXTENTS */

--
1.5.4.1.97.g40aab-dirty


2008-02-14 10:31:53

by Aneesh Kumar K.V

[permalink] [raw]
Subject: [RFC/PATCH] ext4: Request for journal write access early.

In ext4_ext_convert_to_initialized before we need to request for journal
write access before we even modify the extent length.

Signed-off-by: Aneesh Kumar K.V <[email protected]>
---
fs/ext4/extents.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 995ac16..c4d6f19 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2168,6 +2168,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
newblock = iblock - ee_block + ext_pblock(ex);
ex2 = ex;

+ err = ext4_ext_get_access(handle, inode, path + depth);
+ if (err)
+ goto out;
+
/* ex1: ee_block to iblock - 1 : uninitialized */
if (iblock > ee_block) {
ex1 = ex;
@@ -2210,6 +2214,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
ex = path[depth].p_ext;
if (ex2 != &newex)
ex2 = ex;
+
+ err = ext4_ext_get_access(handle, inode, path + depth);
+ if (err)
+ goto out;
}
allocated = max_blocks;
}
@@ -2230,9 +2238,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
ex2->ee_len = cpu_to_le16(allocated);
if (ex2 != ex)
goto insert;
- err = ext4_ext_get_access(handle, inode, path + depth);
- if (err)
- goto out;
/*
* New (initialized) extent starts from the first block
* in the current extent. i.e., ex2 == ex
--
1.5.4.1.97.g40aab-dirty

2008-02-14 18:34:13

by Mingming Cao

[permalink] [raw]
Subject: Re: [RFC/PATCH] ext4: Fix the memory leak and buffer head leak with respect to ext4_ext_path

On Thu, 2008-02-14 at 16:00 +0530, Aneesh Kumar K.V wrote:
> The path variable returned via ext4_ext_find_extent is a kmalloc variable
> and need to be freeded. It also contain refrences to buffer_head which need
> to be dropped.
>
Acked.

Mingming
> Signed-off-by: Aneesh Kumar K.V <[email protected]>
> ---
> fs/ext4/extents.c | 6 +++---
> fs/ext4/migrate.c | 5 +++++
> include/linux/ext4_fs_extents.h | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e856f66..995ac16 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -349,7 +349,7 @@ static void ext4_ext_show_leaf(struct inode *inode, struct ext4_ext_path *path)
> #define ext4_ext_show_leaf(inode,path)
> #endif
>
> -static void ext4_ext_drop_refs(struct ext4_ext_path *path)
> +void ext4_ext_drop_refs(struct ext4_ext_path *path)
> {
> int depth = path->p_depth;
> int i;
> @@ -2200,10 +2200,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> newdepth = ext_depth(inode);
> if (newdepth != depth) {
> depth = newdepth;
> - path = ext4_ext_find_extent(inode, iblock, NULL);
> + ext4_ext_drop_refs(path);
> + path = ext4_ext_find_extent(inode, iblock, path);
> if (IS_ERR(path)) {
> err = PTR_ERR(path);
> - path = NULL;
> goto out;
> }
> eh = path[depth].p_hdr;
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 8c6c685..5c1e27d 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -43,6 +43,7 @@ static int finish_range(handle_t *handle, struct inode *inode,
>
> if (IS_ERR(path)) {
> retval = PTR_ERR(path);
> + path = NULL;
> goto err_out;
> }
>
> @@ -74,6 +75,10 @@ static int finish_range(handle_t *handle, struct inode *inode,
> }
> retval = ext4_ext_insert_extent(handle, inode, path, &newext);
> err_out:
> + if (path) {
> + ext4_ext_drop_refs(path);
> + kfree(path);
> + }
> lb->first_pblock = 0;
> return retval;
> }
> diff --git a/include/linux/ext4_fs_extents.h b/include/linux/ext4_fs_extents.h
> index 697da4b..1285c58 100644
> --- a/include/linux/ext4_fs_extents.h
> +++ b/include/linux/ext4_fs_extents.h
> @@ -227,5 +227,6 @@ extern int ext4_ext_search_left(struct inode *, struct ext4_ext_path *,
> ext4_lblk_t *, ext4_fsblk_t *);
> extern int ext4_ext_search_right(struct inode *, struct ext4_ext_path *,
> ext4_lblk_t *, ext4_fsblk_t *);
> +extern void ext4_ext_drop_refs(struct ext4_ext_path *);
> #endif /* _LINUX_EXT4_EXTENTS */
>

2008-02-14 18:47:05

by Mingming Cao

[permalink] [raw]
Subject: Re: [RFC/PATCH] ext4: Request for journal write access early.

On Thu, 2008-02-14 at 16:00 +0530, Aneesh Kumar K.V wrote:
> In ext4_ext_convert_to_initialized before we need to request for journal
> write access before we even modify the extent length.
>
Acked.
Thanks
Mingming
> Signed-off-by: Aneesh Kumar K.V <[email protected]>
> ---
> fs/ext4/extents.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 995ac16..c4d6f19 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2168,6 +2168,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> newblock = iblock - ee_block + ext_pblock(ex);
> ex2 = ex;
>
> + err = ext4_ext_get_access(handle, inode, path + depth);
> + if (err)
> + goto out;
> +
> /* ex1: ee_block to iblock - 1 : uninitialized */
> if (iblock > ee_block) {
> ex1 = ex;
> @@ -2210,6 +2214,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> ex = path[depth].p_ext;
> if (ex2 != &newex)
> ex2 = ex;
> +
> + err = ext4_ext_get_access(handle, inode, path + depth);
> + if (err)
> + goto out;
> }
> allocated = max_blocks;
> }
> @@ -2230,9 +2238,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> ex2->ee_len = cpu_to_le16(allocated);
> if (ex2 != ex)
> goto insert;
> - err = ext4_ext_get_access(handle, inode, path + depth);
> - if (err)
> - goto out;
> /*
> * New (initialized) extent starts from the first block
> * in the current extent. i.e., ex2 == ex

2008-02-14 23:21:28

by Mingming Cao

[permalink] [raw]
Subject: Re: [RFC/PATCH] ext4: Fix the memory leak and buffer head leak with respect to ext4_ext_path

Added in ext4 patch queue,
Thanks

On Thu, 2008-02-14 at 16:00 +0530, Aneesh Kumar K.V wrote:
> The path variable returned via ext4_ext_find_extent is a kmalloc variable
> and need to be freeded. It also contain refrences to buffer_head which need
> to be dropped.
>
> Signed-off-by: Aneesh Kumar K.V <[email protected]>
> ---
> fs/ext4/extents.c | 6 +++---
> fs/ext4/migrate.c | 5 +++++
> include/linux/ext4_fs_extents.h | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e856f66..995ac16 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -349,7 +349,7 @@ static void ext4_ext_show_leaf(struct inode *inode, struct ext4_ext_path *path)
> #define ext4_ext_show_leaf(inode,path)
> #endif
>
> -static void ext4_ext_drop_refs(struct ext4_ext_path *path)
> +void ext4_ext_drop_refs(struct ext4_ext_path *path)
> {
> int depth = path->p_depth;
> int i;
> @@ -2200,10 +2200,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> newdepth = ext_depth(inode);
> if (newdepth != depth) {
> depth = newdepth;
> - path = ext4_ext_find_extent(inode, iblock, NULL);
> + ext4_ext_drop_refs(path);
> + path = ext4_ext_find_extent(inode, iblock, path);
> if (IS_ERR(path)) {
> err = PTR_ERR(path);
> - path = NULL;
> goto out;
> }
> eh = path[depth].p_hdr;
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 8c6c685..5c1e27d 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -43,6 +43,7 @@ static int finish_range(handle_t *handle, struct inode *inode,
>
> if (IS_ERR(path)) {
> retval = PTR_ERR(path);
> + path = NULL;
> goto err_out;
> }
>
> @@ -74,6 +75,10 @@ static int finish_range(handle_t *handle, struct inode *inode,
> }
> retval = ext4_ext_insert_extent(handle, inode, path, &newext);
> err_out:
> + if (path) {
> + ext4_ext_drop_refs(path);
> + kfree(path);
> + }
> lb->first_pblock = 0;
> return retval;
> }
> diff --git a/include/linux/ext4_fs_extents.h b/include/linux/ext4_fs_extents.h
> index 697da4b..1285c58 100644
> --- a/include/linux/ext4_fs_extents.h
> +++ b/include/linux/ext4_fs_extents.h
> @@ -227,5 +227,6 @@ extern int ext4_ext_search_left(struct inode *, struct ext4_ext_path *,
> ext4_lblk_t *, ext4_fsblk_t *);
> extern int ext4_ext_search_right(struct inode *, struct ext4_ext_path *,
> ext4_lblk_t *, ext4_fsblk_t *);
> +extern void ext4_ext_drop_refs(struct ext4_ext_path *);
> #endif /* _LINUX_EXT4_EXTENTS */
>

2008-02-14 23:21:30

by Mingming Cao

[permalink] [raw]
Subject: Re: [RFC/PATCH] ext4: Request for journal write access early.

Added in ext4 patch queue,
Thanks
On Thu, 2008-02-14 at 16:00 +0530, Aneesh Kumar K.V wrote:
> In ext4_ext_convert_to_initialized before we need to request for journal
> write access before we even modify the extent length.
>
> Signed-off-by: Aneesh Kumar K.V <[email protected]>
> ---
> fs/ext4/extents.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 995ac16..c4d6f19 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2168,6 +2168,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> newblock = iblock - ee_block + ext_pblock(ex);
> ex2 = ex;
>
> + err = ext4_ext_get_access(handle, inode, path + depth);
> + if (err)
> + goto out;
> +
> /* ex1: ee_block to iblock - 1 : uninitialized */
> if (iblock > ee_block) {
> ex1 = ex;
> @@ -2210,6 +2214,10 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> ex = path[depth].p_ext;
> if (ex2 != &newex)
> ex2 = ex;
> +
> + err = ext4_ext_get_access(handle, inode, path + depth);
> + if (err)
> + goto out;
> }
> allocated = max_blocks;
> }
> @@ -2230,9 +2238,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> ex2->ee_len = cpu_to_le16(allocated);
> if (ex2 != ex)
> goto insert;
> - err = ext4_ext_get_access(handle, inode, path + depth);
> - if (err)
> - goto out;
> /*
> * New (initialized) extent starts from the first block
> * in the current extent. i.e., ex2 == ex

2008-02-16 00:07:53

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [RFC/PATCH] ext4: Request for journal write access early.

On Thu, Feb 14, 2008 at 04:00:52PM +0530, Aneesh Kumar K.V wrote:
> In ext4_ext_convert_to_initialized before we need to request for journal
> write access before we even modify the extent length.

This isn't a grammatically correct sentence, and it doesn't explain
what is going on. I rewrote the patch description as follows:

ext4: Get journal write access before modifying the extent tree

When the user was writing into an unitialized extent,
ext4_ext_convert_to_initialize() was not requesting journal write access
before it started to modify the extent tree. Fix this oversight.

- Ted