[PATCH v2 1/3] ext4: Verify block bitmap even after fresh initialization

[PATCH v2 1/3] ext4: Verify block bitmap even after fresh initialization

[Lukas Czerner]

If we want to rely on the buffer_verified() flag of the block bitmap
buffer, we have to set it consistently. However currently if we're
initializing uninitialized block bitmap in
ext4_read_block_bitmap_nowait() we're not going to set buffer verified
at all.

We can do this by simply setting the flag on the buffer, but I think
it's actually better to run ext4_validate_block_bitmap() to make sure
that what we did in the ext4_init_block_bitmap() is right.

So run ext4_validate_block_bitmap() even after the block bitmap
initialization. Also bail out early from ext4_validate_block_bitmap() if
we see corrupt bitmap, since we already know it's corrupt and we do not
need to verify that.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: nothing changed

 fs/ext4/balloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 955bf49..cd6ea29 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -369,7 +369,7 @@ static void ext4_validate_block_bitmap(struct super_block *sb,
 	struct ext4_group_info *grp = ext4_get_group_info(sb, block_group);
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	if (buffer_verified(bh))
+	if (buffer_verified(bh) || EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
 		return;
 
 	ext4_lock_group(sb, block_group);
@@ -446,7 +446,7 @@ ext4_read_block_bitmap_nowait(struct super_block *sb, ext4_group_t block_group)
 		unlock_buffer(bh);
 		if (err)
 			ext4_error(sb, "Checksum bad for grp %u", block_group);
-		return bh;
+		goto verify;
 	}
 	ext4_unlock_group(sb, block_group);
 	if (buffer_uptodate(bh)) {
-- 
1.8.3.1

[PATCH v2 2/3] ext4: Try to initialize all groups we can in case of failure on ppc64

[Lukas Czerner]

Currently on the machines with page size > block size when initializing
block group buddy cache we initialize it for all the block group bitmaps
in the page. However in the case of read error, checksum error, or if
a single bitmap is in any way corrupted we would fail to initialize all
of the bitmaps. This is problematic because we will not have access to
the other allocation groups even though those might be perfectly fine
and usable.

Fix this by reading all the bitmaps instead of error out on the first
problem and simply skip the bitmaps which were either not read properly,
or are not valid.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: remove unnecessary else in condition

 fs/ext4/mballoc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 8d1e602..df02951 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -882,10 +882,8 @@ static int ext4_mb_init_cache(struct page *page, char *incore)
 
 	/* wait for I/O completion */
 	for (i = 0, group = first_group; i < groups_per_page; i++, group++) {
-		if (bh[i] && ext4_wait_block_bitmap(sb, group, bh[i])) {
+		if (bh[i] && ext4_wait_block_bitmap(sb, group, bh[i]))
 			err = -EIO;
-			goto out;
-		}
 	}
 
 	first_block = page->index * blocks_per_page;
@@ -898,6 +896,11 @@ static int ext4_mb_init_cache(struct page *page, char *incore)
 			/* skip initialized uptodate buddy */
 			continue;
 
+		if (!buffer_verified(bh[group - first_group]))
+			/* Skip faulty bitmaps */
+			continue;
+		err = 0;
+
 		/*
 		 * data carry information regarding this
 		 * particular group in the format specified
-- 
1.8.3.1

[PATCH v2 3/3] ext4: Return error code from ext4_mb_good_group()

[Lukas Czerner]

Currently ext4_mb_good_group() only returns 0 or 1 depending on whether
the allocation group is suitable for use or not. However we might get
various errors and fail while initializing new group including -EIO
which would never get propagated up the call chain. This might lead to
an endless loop at writeback when we're trying to find a good group to
allocate from and we fail to initialize new group (read error for
example).

Fix this by returning proper error code from ext4_mb_good_group() and
using it in ext4_mb_regular_allocator(). In ext4_mb_regular_allocator()
we will always return only the first occurred error from
ext4_mb_good_group() and we only propagate it back  to the caller if we
do not get any other errors and we fail to allocate any blocks.

Note that with other modes than errors=continue, we will fail
immediately in ext4_mb_good_group() in case of error, however with
errors=continue we should try to continue using the file system, that's
why we're not going to fail immediately when we see an error from
ext4_mb_good_group(), but rather when we fail to find a suitable block
group to allocate from due to an problem in group initialization.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: nothing changed

 fs/ext4/mballoc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index df02951..e0e77a7 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2034,7 +2034,7 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac,
 	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
 		int ret = ext4_mb_init_group(ac->ac_sb, group);
 		if (ret)
-			return 0;
+			return ret;
 	}
 
 	fragments = grp->bb_fragments;
@@ -2081,7 +2081,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 {
 	ext4_group_t ngroups, group, i;
 	int cr;
-	int err = 0;
+	int err = 0, first_err = 0;
 	struct ext4_sb_info *sbi;
 	struct super_block *sb;
 	struct ext4_buddy e4b;
@@ -2148,6 +2148,7 @@ repeat:
 		group = ac->ac_g_ex.fe_group;
 
 		for (i = 0; i < ngroups; group++, i++) {
+			int ret = 0;
 			cond_resched();
 			/*
 			 * Artificially restricted ngroups for non-extent
@@ -2157,8 +2158,12 @@ repeat:
 				group = 0;
 
 			/* This now checks without needing the buddy page */
-			if (!ext4_mb_good_group(ac, group, cr))
+			ret = ext4_mb_good_group(ac, group, cr);
+			if (ret <= 0) {
+				if (!first_err)
+					first_err = ret;
 				continue;
+			}
 
 			err = ext4_mb_load_buddy(sb, group, &e4b);
 			if (err)
@@ -2170,9 +2175,12 @@ repeat:
 			 * We need to check again after locking the
 			 * block group
 			 */
-			if (!ext4_mb_good_group(ac, group, cr)) {
+			ret = ext4_mb_good_group(ac, group, cr);
+			if (ret <= 0) {
 				ext4_unlock_group(sb, group);
 				ext4_mb_unload_buddy(&e4b);
+				if (!first_err)
+					first_err = ret;
 				continue;
 			}
 
@@ -2219,6 +2227,8 @@ repeat:
 		}
 	}
 out:
+	if (!err && ac->ac_status != AC_STATUS_FOUND && first_err)
+		err = first_err;
 	return err;
 }
 
-- 
1.8.3.1

Re: [PATCH v2 3/3] ext4: Return error code from ext4_mb_good_group()

[Darrick J. Wong]

On Tue, Jun 02, 2015 at 02:24:50PM +0200, Lukas Czerner wrote:
> Currently ext4_mb_good_group() only returns 0 or 1 depending on whether
> the allocation group is suitable for use or not. However we might get
> various errors and fail while initializing new group including -EIO
> which would never get propagated up the call chain. This might lead to
> an endless loop at writeback when we're trying to find a good group to
> allocate from and we fail to initialize new group (read error for
> example).
> 
> Fix this by returning proper error code from ext4_mb_good_group() and
> using it in ext4_mb_regular_allocator(). In ext4_mb_regular_allocator()
> we will always return only the first occurred error from
> ext4_mb_good_group() and we only propagate it back  to the caller if we
> do not get any other errors and we fail to allocate any blocks.
> 
> Note that with other modes than errors=continue, we will fail
> immediately in ext4_mb_good_group() in case of error, however with
> errors=continue we should try to continue using the file system, that's
> why we're not going to fail immediately when we see an error from
> ext4_mb_good_group(), but rather when we fail to find a suitable block
> group to allocate from due to an problem in group initialization.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
> v2: nothing changed
> 
>  fs/ext4/mballoc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index df02951..e0e77a7 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2034,7 +2034,7 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac,

I'd prefer a comment clarifying the possible return values.  Otherwise,
everything (patches 1-3) looks ok to me.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

>  	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
>  		int ret = ext4_mb_init_group(ac->ac_sb, group);
>  		if (ret)
> -			return 0;
> +			return ret;
>  	}
>  
>  	fragments = grp->bb_fragments;
> @@ -2081,7 +2081,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  {
>  	ext4_group_t ngroups, group, i;
>  	int cr;
> -	int err = 0;
> +	int err = 0, first_err = 0;
>  	struct ext4_sb_info *sbi;
>  	struct super_block *sb;
>  	struct ext4_buddy e4b;
> @@ -2148,6 +2148,7 @@ repeat:
>  		group = ac->ac_g_ex.fe_group;
>  
>  		for (i = 0; i < ngroups; group++, i++) {
> +			int ret = 0;
>  			cond_resched();
>  			/*
>  			 * Artificially restricted ngroups for non-extent
> @@ -2157,8 +2158,12 @@ repeat:
>  				group = 0;
>  
>  			/* This now checks without needing the buddy page */
> -			if (!ext4_mb_good_group(ac, group, cr))
> +			ret = ext4_mb_good_group(ac, group, cr);
> +			if (ret <= 0) {
> +				if (!first_err)
> +					first_err = ret;
>  				continue;
> +			}
>  
>  			err = ext4_mb_load_buddy(sb, group, &e4b);
>  			if (err)
> @@ -2170,9 +2175,12 @@ repeat:
>  			 * We need to check again after locking the
>  			 * block group
>  			 */
> -			if (!ext4_mb_good_group(ac, group, cr)) {
> +			ret = ext4_mb_good_group(ac, group, cr);
> +			if (ret <= 0) {
>  				ext4_unlock_group(sb, group);
>  				ext4_mb_unload_buddy(&e4b);
> +				if (!first_err)
> +					first_err = ret;
>  				continue;
>  			}
>  
> @@ -2219,6 +2227,8 @@ repeat:
>  		}
>  	}
>  out:
> +	if (!err && ac->ac_status != AC_STATUS_FOUND && first_err)
> +		err = first_err;
>  	return err;
>  }
>  
> -- 
> 1.8.3.1
> 
> --
> 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 v2 3/3] ext4: Return error code from ext4_mb_good_group()

[Lukáš Czerner]

On Tue, 2 Jun 2015, Darrick J. Wong wrote:

> Date: Tue, 2 Jun 2015 09:27:18 -0700
> From: Darrick J. Wong <darrick.wong@oracle.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] ext4: Return error code from ext4_mb_good_group()
> 
> On Tue, Jun 02, 2015 at 02:24:50PM +0200, Lukas Czerner wrote:
> > Currently ext4_mb_good_group() only returns 0 or 1 depending on whether
> > the allocation group is suitable for use or not. However we might get
> > various errors and fail while initializing new group including -EIO
> > which would never get propagated up the call chain. This might lead to
> > an endless loop at writeback when we're trying to find a good group to
> > allocate from and we fail to initialize new group (read error for
> > example).
> > 
> > Fix this by returning proper error code from ext4_mb_good_group() and
> > using it in ext4_mb_regular_allocator(). In ext4_mb_regular_allocator()
> > we will always return only the first occurred error from
> > ext4_mb_good_group() and we only propagate it back  to the caller if we
> > do not get any other errors and we fail to allocate any blocks.
> > 
> > Note that with other modes than errors=continue, we will fail
> > immediately in ext4_mb_good_group() in case of error, however with
> > errors=continue we should try to continue using the file system, that's
> > why we're not going to fail immediately when we see an error from
> > ext4_mb_good_group(), but rather when we fail to find a suitable block
> > group to allocate from due to an problem in group initialization.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> > v2: nothing changed
> > 
> >  fs/ext4/mballoc.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index df02951..e0e77a7 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -2034,7 +2034,7 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac,
> 
> I'd prefer a comment clarifying the possible return values.  Otherwise,
> everything (patches 1-3) looks ok to me.

Right, with this change the comment is not entirely accurate
anymore. I'll fix that.

Thanks!
-Lukas

> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> >  	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
> >  		int ret = ext4_mb_init_group(ac->ac_sb, group);
> >  		if (ret)
> > -			return 0;
> > +			return ret;
> >  	}
> >  
> >  	fragments = grp->bb_fragments;
> > @@ -2081,7 +2081,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
> >  {
> >  	ext4_group_t ngroups, group, i;
> >  	int cr;
> > -	int err = 0;
> > +	int err = 0, first_err = 0;
> >  	struct ext4_sb_info *sbi;
> >  	struct super_block *sb;
> >  	struct ext4_buddy e4b;
> > @@ -2148,6 +2148,7 @@ repeat:
> >  		group = ac->ac_g_ex.fe_group;
> >  
> >  		for (i = 0; i < ngroups; group++, i++) {
> > +			int ret = 0;
> >  			cond_resched();
> >  			/*
> >  			 * Artificially restricted ngroups for non-extent
> > @@ -2157,8 +2158,12 @@ repeat:
> >  				group = 0;
> >  
> >  			/* This now checks without needing the buddy page */
> > -			if (!ext4_mb_good_group(ac, group, cr))
> > +			ret = ext4_mb_good_group(ac, group, cr);
> > +			if (ret <= 0) {
> > +				if (!first_err)
> > +					first_err = ret;
> >  				continue;
> > +			}
> >  
> >  			err = ext4_mb_load_buddy(sb, group, &e4b);
> >  			if (err)
> > @@ -2170,9 +2175,12 @@ repeat:
> >  			 * We need to check again after locking the
> >  			 * block group
> >  			 */
> > -			if (!ext4_mb_good_group(ac, group, cr)) {
> > +			ret = ext4_mb_good_group(ac, group, cr);
> > +			if (ret <= 0) {
> >  				ext4_unlock_group(sb, group);
> >  				ext4_mb_unload_buddy(&e4b);
> > +				if (!first_err)
> > +					first_err = ret;
> >  				continue;
> >  			}
> >  
> > @@ -2219,6 +2227,8 @@ repeat:
> >  		}
> >  	}
> >  out:
> > +	if (!err && ac->ac_status != AC_STATUS_FOUND && first_err)
> > +		err = first_err;
> >  	return err;
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 
> > --
> > 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
>