[PATCH] ext4: clarify ext4_error message in ext4_mb_generate_buddy_error()

[PATCH] ext4: clarify ext4_error message in ext4_mb_generate_buddy_error()

[Theodore Ts'o]

We are spending a lot of time explaining to users what this error
means.  Let's try to improve the message to avoid this problem.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/mballoc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7f72f50..2dcb936 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -752,8 +752,8 @@ void ext4_mb_generate_buddy(struct super_block *sb,
 
 	if (free != grp->bb_free) {
 		ext4_grp_locked_error(sb, group, 0, 0,
-				      "%u clusters in bitmap, %u in gd; "
-				      "block bitmap corrupt.",
+				      "block bitmap and bg descriptor "
+				      "inconsistent: %u vs %u free clusters",
 				      free, grp->bb_free);
 		/*
 		 * If we intend to continue, we consider group descriptor
-- 
2.0.0

Re: [PATCH] ext4: clarify ext4_error message in ext4_mb_generate_buddy_error()

[Lukáš Czerner]

On Sat, 5 Jul 2014, Theodore Ts'o wrote:

> Date: Sat,  5 Jul 2014 19:16:02 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>, stable@vger.kernel.org
> Subject: [PATCH] ext4: clarify ext4_error message in
>     ext4_mb_generate_buddy_error()
> 
> We are spending a lot of time explaining to users what this error
> means.  Let's try to improve the message to avoid this problem.

It is a bit better, even though strictly speaking it's not
right, because it is not block bitmap alone, but rather aggregation
of block bitmap and preallocations. But for the user this is really
an implementation detail they do not need to worry about I guess.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!
-Lukas

> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.kernel.org
> ---
>  fs/ext4/mballoc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7f72f50..2dcb936 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -752,8 +752,8 @@ void ext4_mb_generate_buddy(struct super_block *sb,
>  
>  	if (free != grp->bb_free) {
>  		ext4_grp_locked_error(sb, group, 0, 0,
> -				      "%u clusters in bitmap, %u in gd; "
> -				      "block bitmap corrupt.",
> +				      "block bitmap and bg descriptor "
> +				      "inconsistent: %u vs %u free clusters",
>  				      free, grp->bb_free);
>  		/*
>  		 * If we intend to continue, we consider group descriptor
> 

Re: [PATCH] ext4: clarify ext4_error message in ext4_mb_generate_buddy_error()

[Theodore Ts'o]

On Tue, Jul 08, 2014 at 09:03:48AM +0200, Lukáš Czerner wrote:
> 
> It is a bit better, even though strictly speaking it's not
> right, because it is not block bitmap alone, but rather aggregation
> of block bitmap and preallocations. But for the user this is really
> an implementation detail they do not need to worry about I guess.

Actually, no, because the preallocations aren't reflected in the
in-block bitmap.

And, oh sh*t, I wonder if that's the cause of the
ext4_mb_generate_buddy().  We don't need the buddy bitmaps to allocate
out of the preallocations, and it's not needed by
ext4_mb_mark_diskspace_used().  If the buddy bitmaps have been pushed
out of the page cache between when the blocks were originally
preallocated and when we try to use some preallocated blocks, and then
we have a race between ext4_mb_mark_diskspace_used() and
ext4_mb_buddy_generate(), that could explain the discrepancy between
the block group descriptors and the loaded buddy bitmap.

If this is what's going on, it doesn't explain why we hacen't been
seeing this until post 3.15, though....

				- 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

Re: [PATCH] ext4: clarify ext4_error message in ext4_mb_generate_buddy_error()

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2197 bytes --]

On Tue, 8 Jul 2014, Theodore Ts'o wrote:

> Date: Tue, 8 Jul 2014 08:54:54 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>, stable@vger.kernel.org
> Subject: Re: [PATCH] ext4: clarify ext4_error message in
>     ext4_mb_generate_buddy_error()
> 
> On Tue, Jul 08, 2014 at 09:03:48AM +0200, Lukáš Czerner wrote:
> > 
> > It is a bit better, even though strictly speaking it's not
> > right, because it is not block bitmap alone, but rather aggregation
> > of block bitmap and preallocations. But for the user this is really
> > an implementation detail they do not need to worry about I guess.
> 
> Actually, no, because the preallocations aren't reflected in the
> in-block bitmap.

But the "bitmap" argument we're getting in ext4_mb_generate_buddy() is
not just the original on disk bitmap, is it ? It's aggregation of the on
on disk block bitmap and preallocation (this is done in
ext4_mb_init_cache()). Or am I missing something ?

> 
> And, oh sh*t, I wonder if that's the cause of the
> ext4_mb_generate_buddy().  We don't need the buddy bitmaps to allocate
> out of the preallocations, and it's not needed by
> ext4_mb_mark_diskspace_used().  If the buddy bitmaps have been pushed
> out of the page cache between when the blocks were originally
> preallocated and when we try to use some preallocated blocks, and then
> we have a race between ext4_mb_mark_diskspace_used() and
> ext4_mb_buddy_generate(), that could explain the discrepancy between
> the block group descriptors and the loaded buddy bitmap.

ext4_mb_generate_buddy() is run under the group lock and the actual
ext4_set_bits() in ext4_mb_mark_diskspace_used as well. And again in
ext4_mb_generate_buddy() we do take into account preallocations
which will be marked as used in the buddy bitmap.

> 
> If this is what's going on, it doesn't explain why we hacen't been
> seeing this until post 3.15, though....
> 
> 				- 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
>