| Message ID | 20130222175557.GA21264@thunk.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 > -- 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
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); > > -- 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
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 -- 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
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 -- 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
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);