[PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Eryu Guan]

len is 0 means no extent needs to be removed, so return immediately.
Otherwise it could trigger the following BUG_ON()

	436	end = offset + len - 1;
	437	BUG_ON(end < offset);

This could be reproduced by a simple truncate(1) command by an
unprivileged user

	truncate -s $(($((2**32 - 1)) * 4096)) /mnt/ext4/testfile

The same is true for __es_insert_extent().

Patched kernel passed xfstests regression test.

Also remove comments about EXT4_I(inode)->i_es_lock, this rwlock
isn't hold by callers.

Cc: Zheng Liu <wenqing.lz@taobao.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 fs/ext4/extents_status.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 564d981..3ac09ca 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -328,6 +328,9 @@ static int __es_insert_extent(struct ext4_es_tree *tree, ext4_lblk_t offset,
 	struct extent_status *es;
 	ext4_lblk_t end = offset + len - 1;
 
+	if (!len)
+		return 0;
+
 	BUG_ON(end < offset);
 	es = tree->cache_es;
 	if (es && offset == (extent_status_end(es) + 1)) {
@@ -386,7 +389,6 @@ out:
 
 /*
  * ext4_es_insert_extent() adds a space to a delayed extent tree.
- * Caller holds inode->i_es_lock.
  *
  * ext4_es_insert_extent is called by ext4_da_write_begin and
  * ext4_es_remove_extent.
@@ -415,7 +417,6 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t offset,
 
 /*
  * ext4_es_remove_extent() removes a space from a delayed extent tree.
- * Caller holds inode->i_es_lock.
  *
  * Return 0 on success, error code on failure.
  */
@@ -433,6 +434,9 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t offset,
 	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
 		 offset, len, inode->i_ino);
 
+	if (!len)
+		return err;
+
 	end = offset + len - 1;
 	BUG_ON(end < offset);
 	write_lock(&EXT4_I(inode)->i_es_lock);
-- 
1.8.1.2

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Zheng Liu]

On Fri, Feb 22, 2013 at 01:34:03PM +0800, Eryu Guan wrote:
> len is 0 means no extent needs to be removed, so return immediately.
> Otherwise it could trigger the following BUG_ON()
> 
> 	436	end = offset + len - 1;
> 	437	BUG_ON(end < offset);
> 
> This could be reproduced by a simple truncate(1) command by an
> unprivileged user
> 
> 	truncate -s $(($((2**32 - 1)) * 4096)) /mnt/ext4/testfile
> 
> The same is true for __es_insert_extent().
> 
> Patched kernel passed xfstests regression test.
> 
> Also remove comments about EXT4_I(inode)->i_es_lock, this rwlock
> isn't hold by callers.
> 
> Cc: Zheng Liu <wenqing.lz@taobao.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>

Thanks for fixing it.

Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Regards,
                                                - Zheng

> ---
>  fs/ext4/extents_status.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 564d981..3ac09ca 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -328,6 +328,9 @@ static int __es_insert_extent(struct ext4_es_tree *tree, ext4_lblk_t offset,
>  	struct extent_status *es;
>  	ext4_lblk_t end = offset + len - 1;
>  
> +	if (!len)
> +		return 0;
> +
>  	BUG_ON(end < offset);
>  	es = tree->cache_es;
>  	if (es && offset == (extent_status_end(es) + 1)) {
> @@ -386,7 +389,6 @@ out:
>  
>  /*
>   * ext4_es_insert_extent() adds a space to a delayed extent tree.
> - * Caller holds inode->i_es_lock.
>   *
>   * ext4_es_insert_extent is called by ext4_da_write_begin and
>   * ext4_es_remove_extent.
> @@ -415,7 +417,6 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t offset,
>  
>  /*
>   * ext4_es_remove_extent() removes a space from a delayed extent tree.
> - * Caller holds inode->i_es_lock.
>   *
>   * Return 0 on success, error code on failure.
>   */
> @@ -433,6 +434,9 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t offset,
>  	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
>  		 offset, len, inode->i_ino);
>  
> +	if (!len)
> +		return err;
> +
>  	end = offset + len - 1;
>  	BUG_ON(end < offset);
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Theodore Ts'o]

This patch didn't apply since it was apparently against an older
version of the extents status patches.  Here is the version after I
fixed it up so it would apply into the current ext4 tree.  Zheng, can
you do a quick sanity check to make sure I didn't screw up anything?
Thanks!

Eryu, thanks for testing and submitting a bug fix!!

						- Ted

>From 7d46e5051453b2c4dfac4e31ae1afb30064cc404 Mon Sep 17 00:00:00 2001
From: Eryu Guan <guaneryu@gmail.com>
Date: Fri, 22 Feb 2013 12:54:36 -0500
Subject: [PATCH] ext4: no need to remove extent if len is 0 in
 ext4_es_remove_extent()

len is 0 means no extent needs to be removed, so return immediately.
Otherwise it could trigger the following BUG_ON() in
ext4_es_remove_extent()

	end = lblk + len - 1;
	BUG_ON(end < lblk);

This could be reproduced by a simple truncate(1) command by an
unprivileged user

	truncate -s $(($((2**32 - 1)) * 4096)) /mnt/ext4/testfile

The same is true for __es_insert_extent().

Patched kernel passed xfstests regression test.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
---
 fs/ext4/extents_status.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 9f1380e..2be245b 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -392,6 +392,9 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
 	struct rb_node *parent = NULL;
 	struct extent_status *es;
 
+	if (!len)
+		return 0;
+
 	while (*p) {
 		parent = *p;
 		es = rb_entry(parent, struct extent_status, rb_node);
@@ -456,6 +459,9 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 	es_debug("add [%u/%u) %llu %llx to extent status tree of inode %lu\n",
 		 lblk, len, pblk, status, inode->i_ino);
 
+	if (!len)
+		return 0;
+
 	BUG_ON(end < lblk);
 
 	newes.es_lblk = lblk;
@@ -649,6 +655,9 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
 		 lblk, len, inode->i_ino);
 
+	if (!len)
+		return err;
+
 	end = lblk + len - 1;
 	BUG_ON(end < lblk);
 
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Eryu Guan]

On Sat, Feb 23, 2013 at 1:55 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> This patch didn't apply since it was apparently against an older
> version of the extents status patches.  Here is the version after I
> fixed it up so it would apply into the current ext4 tree.  Zheng, can

Thanks Ted! I was making the patch on top of Linus' tree.
Linus' tree vs ext4 tree which one is preferred for submitting patch?

Thanks,
Eryu
> you do a quick sanity check to make sure I didn't screw up anything?
> Thanks!
>
> Eryu, thanks for testing and submitting a bug fix!!
>
>                                                 - Ted
>
> From 7d46e5051453b2c4dfac4e31ae1afb30064cc404 Mon Sep 17 00:00:00 2001
> From: Eryu Guan <guaneryu@gmail.com>
> Date: Fri, 22 Feb 2013 12:54:36 -0500
> Subject: [PATCH] ext4: no need to remove extent if len is 0 in
>  ext4_es_remove_extent()
>
> len is 0 means no extent needs to be removed, so return immediately.
> Otherwise it could trigger the following BUG_ON() in
> ext4_es_remove_extent()
>
>         end = lblk + len - 1;
>         BUG_ON(end < lblk);
>
> This could be reproduced by a simple truncate(1) command by an
> unprivileged user
>
>         truncate -s $(($((2**32 - 1)) * 4096)) /mnt/ext4/testfile
>
> The same is true for __es_insert_extent().
>
> Patched kernel passed xfstests regression test.
>
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
> ---
>  fs/ext4/extents_status.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 9f1380e..2be245b 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -392,6 +392,9 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>         struct rb_node *parent = NULL;
>         struct extent_status *es;
>
> +       if (!len)
> +               return 0;
> +
>         while (*p) {
>                 parent = *p;
>                 es = rb_entry(parent, struct extent_status, rb_node);
> @@ -456,6 +459,9 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>         es_debug("add [%u/%u) %llu %llx to extent status tree of inode %lu\n",
>                  lblk, len, pblk, status, inode->i_ino);
>
> +       if (!len)
> +               return 0;
> +
>         BUG_ON(end < lblk);
>
>         newes.es_lblk = lblk;
> @@ -649,6 +655,9 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>         es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
>                  lblk, len, inode->i_ino);
>
> +       if (!len)
> +               return err;
> +
>         end = lblk + len - 1;
>         BUG_ON(end < lblk);
>
> --
> 1.7.12.rc0.22.gcdd159b
>

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Theodore Ts'o]

On Sat, Feb 23, 2013 at 11:40:13AM +0800, Eryu Guan wrote:
> On Sat, Feb 23, 2013 at 1:55 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> > This patch didn't apply since it was apparently against an older
> > version of the extents status patches.  Here is the version after I
> > fixed it up so it would apply into the current ext4 tree.  Zheng, can
> 
> Thanks Ted! I was making the patch on top of Linus' tree.
> Linus' tree vs ext4 tree which one is preferred for submitting patch?

The ext4 tree in general is the one which is preferred; the dev branch
is the tip of what we hope to push to Linus.  At the moment, it's in
final testing.  The three branch pointers which are important on the
ext4 tree are origin, master, and dev.  The origin branch is where we
have branched off of Linus's tree.  At the moment, ext4/origin is
pointing at v3.8-rc3.  The ext4/master branch is always between origin
and dev (inclusive).  The dev branch is a rewinding branch, which
means that everything between master and dev may be get modified
(i.e., to add a Reviewed-by: or to fix up some comments, etc.), or may
get dropped (if it turns out we discover the patch is not ready for
prime time).  The dev branch is also what gets included into
linux-next.

The master branch represents those patches which have been
"finalized", which means once we bump the master branch, all of the
commits between origin and master (inclusive) are guaranteed not to
change.  So for people who are building on top of master, it's safe
for them to use git.  For people who are building on top of dev, if
you want to make changes, it's recommended you use a tool like quilt,
guilt, or stgit.

Speaking of quilt/guilt, the set of patches between master and dev can
be found here:

	http://repo.or.cz/w/ext4-patch-queue.git
	git://repo.or.cz/ext4-patch-queue.git

For those people who are interested, or who want to more easily cherry
pick specific patches out of the ext4 patch queue, the ext4/dev branch
(usually, assuming I've remembered to update the ext4 patch queue
tree) can be reconstructed as follows:

    git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ext4
    mkdir -p ext4/.git/patches
    cd ext4/.git/patches
    git clone git://repo.or.cz/ext4-patch-queue.git dev
    cd dev
    sh timestamps
    cd ../../..
    git branch dev $(head -1 .git/patches/dev/series | sed -e 's/# BASE //')
    git checkout dev
    guilt push stable-boundary
    guilt pop

(This assumes you are using guilt version v0.35, found at
git://repo.or.cz/guilt.git; note that the tip of the guilt tree has
incompatible changes in how they parse patches, so I haven't upgraded
to the tip of guilt tree yet.)

Anyway, most people will send me patches against Linus's tree, and
that's fine; if there are problems, I can usually fix up the patches.
But it's most convenient for me if people send against either the
ext4/master, or most preferably, the ext4/dev branch.

BTW, I've updated the ext4 wiki to include the above information.

Thanks,

     	  	      	   	   - Ted

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Eryu Guan]

On Sun, Feb 24, 2013 at 7:45 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Feb 23, 2013 at 11:40:13AM +0800, Eryu Guan wrote:
>> On Sat, Feb 23, 2013 at 1:55 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> > This patch didn't apply since it was apparently against an older
>> > version of the extents status patches.  Here is the version after I
>> > fixed it up so it would apply into the current ext4 tree.  Zheng, can
>>
>> Thanks Ted! I was making the patch on top of Linus' tree.
>> Linus' tree vs ext4 tree which one is preferred for submitting patch?
>
> The ext4 tree in general is the one which is preferred; the dev branch
> is the tip of what we hope to push to Linus.  At the moment, it's in
> final testing.  The three branch pointers which are important on the
> ext4 tree are origin, master, and dev.  The origin branch is where we
> have branched off of Linus's tree.  At the moment, ext4/origin is
> pointing at v3.8-rc3.  The ext4/master branch is always between origin
> and dev (inclusive).  The dev branch is a rewinding branch, which
> means that everything between master and dev may be get modified
> (i.e., to add a Reviewed-by: or to fix up some comments, etc.), or may
> get dropped (if it turns out we discover the patch is not ready for
> prime time).  The dev branch is also what gets included into
> linux-next.
>
> The master branch represents those patches which have been
> "finalized", which means once we bump the master branch, all of the
> commits between origin and master (inclusive) are guaranteed not to
> change.  So for people who are building on top of master, it's safe
> for them to use git.  For people who are building on top of dev, if
> you want to make changes, it's recommended you use a tool like quilt,
> guilt, or stgit.
>
> Speaking of quilt/guilt, the set of patches between master and dev can
> be found here:
>
>         http://repo.or.cz/w/ext4-patch-queue.git
>         git://repo.or.cz/ext4-patch-queue.git
>
> For those people who are interested, or who want to more easily cherry
> pick specific patches out of the ext4 patch queue, the ext4/dev branch
> (usually, assuming I've remembered to update the ext4 patch queue
> tree) can be reconstructed as follows:
>
>     git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git ext4
>     mkdir -p ext4/.git/patches
>     cd ext4/.git/patches
>     git clone git://repo.or.cz/ext4-patch-queue.git dev
>     cd dev
>     sh timestamps
>     cd ../../..
>     git branch dev $(head -1 .git/patches/dev/series | sed -e 's/# BASE //')
>     git checkout dev
>     guilt push stable-boundary
>     guilt pop
>
> (This assumes you are using guilt version v0.35, found at
> git://repo.or.cz/guilt.git; note that the tip of the guilt tree has
> incompatible changes in how they parse patches, so I haven't upgraded
> to the tip of guilt tree yet.)
>
> Anyway, most people will send me patches against Linus's tree, and
> that's fine; if there are problems, I can usually fix up the patches.
> But it's most convenient for me if people send against either the
> ext4/master, or most preferably, the ext4/dev branch.
>
> BTW, I've updated the ext4 wiki to include the above information.

Thanks for your excellent explanation! I think I'd like to try
ext4/master first :)

Eryu
>
> Thanks,
>
>                                    - Ted

Re: [PATCH] ext4: no need to remove extent if len is 0 in ext4_es_remove_extent()

[Zheng Liu]

Hi Ted,

One minor comment below for the note.

On 02/23/2013 01:55 AM, Theodore Ts'o wrote:
[snip]
> From 7d46e5051453b2c4dfac4e31ae1afb30064cc404 Mon Sep 17 00:00:00 2001
> From: Eryu Guan <guaneryu@gmail.com>
> Date: Fri, 22 Feb 2013 12:54:36 -0500
> Subject: [PATCH] ext4: no need to remove extent if len is 0 in
>  ext4_es_remove_extent()
> 
> len is 0 means no extent needs to be removed, so return immediately.
> Otherwise it could trigger the following BUG_ON() in
> ext4_es_remove_extent()
> 
> 	end = lblk + len - 1;
> 	BUG_ON(end < lblk);
> 
> This could be reproduced by a simple truncate(1) command by an
> unprivileged user
> 
> 	truncate -s $(($((2**32 - 1)) * 4096)) /mnt/ext4/testfile
> 
> The same is true for __es_insert_extent().
> 
> Patched kernel passed xfstests regression test.
> 
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
> ---
>  fs/ext4/extents_status.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 9f1380e..2be245b 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -392,6 +392,9 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>  	struct rb_node *parent = NULL;
>  	struct extent_status *es;
>  
> +	if (!len)
> +		return 0;
> +

This will cause a compile error because we don't define a 'len'
variable.  But I have noticed that you have fixed it in latest dev
branch.  So just for the note.

Otherwise the patch looks good.
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

Thanks for fixing it,
						- Zheng

>  	while (*p) {
>  		parent = *p;
>  		es = rb_entry(parent, struct extent_status, rb_node);
> @@ -456,6 +459,9 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	es_debug("add [%u/%u) %llu %llx to extent status tree of inode %lu\n",
>  		 lblk, len, pblk, status, inode->i_ino);
>  
> +	if (!len)
> +		return 0;
> +
>  	BUG_ON(end < lblk);
>  
>  	newes.es_lblk = lblk;
> @@ -649,6 +655,9 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
>  		 lblk, len, inode->i_ino);
>  
> +	if (!len)
> +		return err;
> +
>  	end = lblk + len - 1;
>  	BUG_ON(end < lblk);
>  
>