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

[PATCH 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>
---
 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 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>
---
 fs/ext4/mballoc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 8d1e602..5677a03 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,12 @@ 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;
+		else
+			err = 0;
+
 		/*
 		 * data carry information regarding this
 		 * particular group in the format specified
-- 
1.8.3.1

[PATCH 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>
---
 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 5677a03..0617dda 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2035,7 +2035,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;
@@ -2082,7 +2082,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;
@@ -2149,6 +2149,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
@@ -2158,8 +2159,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)
@@ -2171,9 +2176,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;
 			}
 
@@ -2220,6 +2228,8 @@ repeat:
 		}
 	}
 out:
+	if (!err && ac->ac_status != AC_STATUS_FOUND && first_err)
+		err = first_err;
 	return err;
 }
 
-- 
1.8.3.1

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

[Andreas Dilger]

> On Jun 1, 2015, at 1:55 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> 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>
> ---
> fs/ext4/mballoc.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 8d1e602..5677a03 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,12 @@ 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;
> +		else
> +			err = 0;

Not really a need for "else" here after the "continue" line, but that
is mostly harmless.

Cheers, Andreas

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

[Andreas Dilger]

> On Jun 1, 2015, at 1:54 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> 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>
> ---
> 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;

(defect) This looks like it will miss the ext4_unlock_group() call
below, since that is missing in the "verify:" branch.  Either you
need to add an explicit ext4_unlock_group() call before "goto verify",
or add a new "unlock_group:" label that unlocks the group.


> 	}
> 	ext4_unlock_group(sb, block_group);
> 	if (buffer_uptodate(bh)) {

Cheers, Andreas

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

[Lukáš Czerner]

On Mon, 1 Jun 2015, Andreas Dilger wrote:

> Date: Mon, 1 Jun 2015 13:15:35 -0600
> From: Andreas Dilger <adilger@dilger.ca>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 2/3] ext4: Try to initialize all groups we can in case of
>     failure on ppc64
> 
> 
> > On Jun 1, 2015, at 1:55 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> > 
> > 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>
> > ---
> > fs/ext4/mballoc.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 8d1e602..5677a03 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,12 @@ 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;
> > +		else
> > +			err = 0;
> 
> Not really a need for "else" here after the "continue" line, but that
> is mostly harmless.
> 
> Cheers, Andreas

Good point, I can fix that.

Thanks!
-Lukas

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

[Lukáš Czerner]

On Mon, 1 Jun 2015, Andreas Dilger wrote:

> Date: Mon, 1 Jun 2015 13:20:31 -0600
> From: Andreas Dilger <adilger@dilger.ca>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/3] ext4: Verify block bitmap even after fresh
>     initialization
> 
> 
> > On Jun 1, 2015, at 1:54 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> > 
> > 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>
> > ---
> > 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;
> 
> (defect) This looks like it will miss the ext4_unlock_group() call
> below, since that is missing in the "verify:" branch.  Either you
> need to add an explicit ext4_unlock_group() call before "goto verify",
> or add a new "unlock_group:" label that unlocks the group.

Hi,

the code snippet tricked you ;). Take a look at the code there is
ext4_unlock_group() just before unlock_buffer() which is missing in
the snipper above.

Thanks!
-Lukas

> 
> 
> > 	}
> > 	ext4_unlock_group(sb, block_group);
> > 	if (buffer_uptodate(bh)) {
> 
> Cheers, Andreas
> 
> 
> 
> 
> 
>