[04/12] f2fs: remove wrong f2fs_bug_on when merging extents
diff mbox

Message ID 1435603176-63219-4-git-send-email-jaegeuk@kernel.org
State New
Headers show

Commit Message

Jaegeuk Kim June 29, 2015, 6:39 p.m. UTC
In f2fs_update_extent_tree, if there is existing extent, f2fs tries to split
it with two parts.
In each trial, __insert_extent_tree checks __is_front/back_mergeable, and then
if it hits to go, there is f2fs_bug_on(!den), which triggers a kernel panic.

Actually, we don't need to check this. Instead, we can do __try_back_merge only
when there exists a den pointer.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

?? June 30, 2015, 3 a.m. UTC | #1
Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Tuesday, June 30, 2015 2:39 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 04/12] f2fs: remove wrong f2fs_bug_on when merging extents
> 
> In f2fs_update_extent_tree, if there is existing extent, f2fs tries to split
> it with two parts.
> In each trial, __insert_extent_tree checks __is_front/back_mergeable, and then
> if it hits to go, there is f2fs_bug_on(!den), which triggers a kernel panic.
> 
> Actually, we don't need to check this. Instead, we can do __try_back_merge only
> when there exists a den pointer.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 9bedfa8..7817167 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -519,19 +519,19 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
> 
>  		if (ei->fofs < en->ei.fofs) {
>  			if (__is_front_mergeable(ei, &en->ei)) {
> -				f2fs_bug_on(sbi, !den);

I add a BUG_ON here because we assume that in extent cache there is no such 
two extents whose mapping address is continuous but without being merged,
since whenever we add a new extent(not splitted one) into cache, we tries to
merge it frontward/backward, then all extent with continuous mapping should be
merged. So when we split one extent to two parts, each part should not be able
to merge with others. Otherwise it should be a bug.

Is there some special case to trigger this BUG_ON?

Thanks,

>  				en->ei.fofs = ei->fofs;
>  				en->ei.blk = ei->blk;
>  				en->ei.len += ei->len;
> -				*den = __try_back_merge(sbi, et, en);
> +				if (den)
> +					*den = __try_back_merge(sbi, et, en);
>  				return en;
>  			}
>  			p = &(*p)->rb_left;
>  		} else if (ei->fofs >= en->ei.fofs + en->ei.len) {
>  			if (__is_back_mergeable(ei, &en->ei)) {
> -				f2fs_bug_on(sbi, !den);
>  				en->ei.len += ei->len;
> -				*den = __try_front_merge(sbi, et, en);
> +				if (den)
> +					*den = __try_front_merge(sbi, et, en);
>  				return en;
>  			}
>  			p = &(*p)->rb_right;
> --
> 2.1.1
> 
> 
> ------------------------------------------------------------------------------
> Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaegeuk Kim July 1, 2015, 1:02 a.m. UTC | #2
On Tue, Jun 30, 2015 at 11:00:13AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Tuesday, June 30, 2015 2:39 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 04/12] f2fs: remove wrong f2fs_bug_on when merging extents
> > 
> > In f2fs_update_extent_tree, if there is existing extent, f2fs tries to split
> > it with two parts.
> > In each trial, __insert_extent_tree checks __is_front/back_mergeable, and then
> > if it hits to go, there is f2fs_bug_on(!den), which triggers a kernel panic.
> > 
> > Actually, we don't need to check this. Instead, we can do __try_back_merge only
> > when there exists a den pointer.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 9bedfa8..7817167 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -519,19 +519,19 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
> > 
> >  		if (ei->fofs < en->ei.fofs) {
> >  			if (__is_front_mergeable(ei, &en->ei)) {
> > -				f2fs_bug_on(sbi, !den);
> 
> I add a BUG_ON here because we assume that in extent cache there is no such 
> two extents whose mapping address is continuous but without being merged,
> since whenever we add a new extent(not splitted one) into cache, we tries to
> merge it frontward/backward, then all extent with continuous mapping should be
> merged. So when we split one extent to two parts, each part should not be able
> to merge with others. Otherwise it should be a bug.

Oh, I see.
Let me take a look at this one more time.
I'm confusing whether this patch was written before fixing block address
calculation and then change the order of the patches.

Thanks,
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9bedfa8..7817167 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -519,19 +519,19 @@  static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
 
 		if (ei->fofs < en->ei.fofs) {
 			if (__is_front_mergeable(ei, &en->ei)) {
-				f2fs_bug_on(sbi, !den);
 				en->ei.fofs = ei->fofs;
 				en->ei.blk = ei->blk;
 				en->ei.len += ei->len;
-				*den = __try_back_merge(sbi, et, en);
+				if (den)
+					*den = __try_back_merge(sbi, et, en);
 				return en;
 			}
 			p = &(*p)->rb_left;
 		} else if (ei->fofs >= en->ei.fofs + en->ei.len) {
 			if (__is_back_mergeable(ei, &en->ei)) {
-				f2fs_bug_on(sbi, !den);
 				en->ei.len += ei->len;
-				*den = __try_front_merge(sbi, et, en);
+				if (den)
+					*den = __try_front_merge(sbi, et, en);
 				return en;
 			}
 			p = &(*p)->rb_right;