ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

[jon ernst]

Hi,

I found that ext4_wait_block_bitmap() and
ext4_read_block_bitmap_nowait() handle bitmap verification
differently.
wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
meets, it will skip ext4_validate_block_bitmap()

In my opinion, they'd better do same thing.
In that way, we can also return "fail" in ext4_valid_block_bitmap()
method when we meet FLEX_BG.



diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index dc5d572..366807a 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
super_block *sb,
                 * or it has to also read the block group where the bitmaps
                 * are located to verify they are set.
                 */
-               return 0;
+               return 1;
        }
        group_first_block = ext4_group_first_block_no(sb, block_group);

@@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
*sb, ext4_group_t block_group,
                return 1;
        }
        clear_buffer_new(bh);
-       /* Panic or remount fs read-only if block bitmap is invalid */
-       ext4_validate_block_bitmap(sb, desc, block_group, bh);
+
+       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
+        return 0;
+        }
+       /* Panic or remount fs read-only if block bitmap is invalid */
+       ext4_validate_block_bitmap(sb, desc, block_group, bh);
        /* ...but check for error just in case errors=continue. */
        return !buffer_verified(bh);
 }

Re: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

[Carlos Maiolino]

Hi,

looks like that, with this patch, ext4_read_block_bitmap_nowait() might sleep in
wait_on_buffer() called from ext4_wait_block_bitmap(), which is not really the
intention of ext4_read_block_bitmap_nowait().

Am I wrong here or this might really happen?


On Thu, Oct 03, 2013 at 10:45:06PM -0400, jon ernst wrote:
> Hi,
> 
> I found that ext4_wait_block_bitmap() and
> ext4_read_block_bitmap_nowait() handle bitmap verification
> differently.
> wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
> But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
> meets, it will skip ext4_validate_block_bitmap()
> 
> In my opinion, they'd better do same thing.
> In that way, we can also return "fail" in ext4_valid_block_bitmap()
> method when we meet FLEX_BG.
> 
> 
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index dc5d572..366807a 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
> super_block *sb,
>                  * or it has to also read the block group where the bitmaps
>                  * are located to verify they are set.
>                  */
> -               return 0;
> +               return 1;
>         }
>         group_first_block = ext4_group_first_block_no(sb, block_group);
> 
> @@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
> *sb, ext4_group_t block_group,
>                 return 1;
>         }
>         clear_buffer_new(bh);
> -       /* Panic or remount fs read-only if block bitmap is invalid */
> -       ext4_validate_block_bitmap(sb, desc, block_group, bh);
> +
> +       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> +        return 0;
> +        }
> +       /* Panic or remount fs read-only if block bitmap is invalid */
> +       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>         /* ...but check for error just in case errors=continue. */
>         return !buffer_verified(bh);
>  }
> --
> 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

-- 
Carlos

Re: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

[Lukáš Czerner]

On Thu, 3 Oct 2013, jon ernst wrote:

> Date: Thu, 3 Oct 2013 22:45:06 -0400
> From: jon ernst <jonernst07@gmail.com>
> To: "linux-ext4@vger.kernel.org List" <linux-ext4@vger.kernel.org>
> Subject: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle
>     bitmap verification differently
> 
> Hi,

Hi Jon,

Btw the patch has some issues and it seems to be badly formatted, or
even corrupted. You're also missing some Signed-off-by line and the
subject is not good either. Please see
Documentation/SubmittingPatches, use git to create patches and use
email client which does not automatically wrap your lines.

> 
> I found that ext4_wait_block_bitmap() and
> ext4_read_block_bitmap_nowait() handle bitmap verification
> differently.
> wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
> But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
> meets, it will skip ext4_validate_block_bitmap()
> 
> In my opinion, they'd better do same thing.

Why ?

> In that way, we can also return "fail" in ext4_valid_block_bitmap()
> method when we meet FLEX_BG.

This does not make sense at all. Why do you suggest that we should
"fail" in the case that we have FLEG_BG feature enabled (which is
default btw) ?

> 
> 
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index dc5d572..366807a 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
> super_block *sb,
>                  * or it has to also read the block group where the bitmaps
>                  * are located to verify they are set.
>                  */
> -               return 0;
> +               return 1;
>         }
>         group_first_block = ext4_group_first_block_no(sb, block_group);
> 
> @@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
> *sb, ext4_group_t block_group,
>                 return 1;
>         }
>         clear_buffer_new(bh);
> -       /* Panic or remount fs read-only if block bitmap is invalid */
> -       ext4_validate_block_bitmap(sb, desc, block_group, bh);
> +
> +       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> +        return 0;

This is wrong from multiple reasons. First of all you're not holding
group lock so what is preventing others to actually initialize the
bitmap before you return 0 ?

Secondly, uninit group will never get that far, because it'll be
initialized in ext4_read_block_bitmap_nowait() and we will not
actually need to wait for the buffer.

Thanks!
-Lukas

> +        }
> +       /* Panic or remount fs read-only if block bitmap is invalid */
> +       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>         /* ...but check for error just in case errors=continue. */
>         return !buffer_verified(bh);
>  }
> --
> 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: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

[jon ernst]

On Mon, Oct 7, 2013 at 8:45 AM, Lukáš Czerner <lczerner@redhat.com> wrote:
> On Thu, 3 Oct 2013, jon ernst wrote:
>
>> Date: Thu, 3 Oct 2013 22:45:06 -0400
>> From: jon ernst <jonernst07@gmail.com>
>> To: "linux-ext4@vger.kernel.org List" <linux-ext4@vger.kernel.org>
>> Subject: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle
>>     bitmap verification differently
>>
>> Hi,
>
> Hi Jon,
>
> Btw the patch has some issues and it seems to be badly formatted, or
> even corrupted. You're also missing some Signed-off-by line and the
> subject is not good either. Please see
> Documentation/SubmittingPatches, use git to create patches and use
> email client which does not automatically wrap your lines.

Thanks for your instruction. I will pay attention.
>
>>
>> I found that ext4_wait_block_bitmap() and
>> ext4_read_block_bitmap_nowait() handle bitmap verification
>> differently.
>> wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
>> But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
>> meets, it will skip ext4_validate_block_bitmap()
>>
>> In my opinion, they'd better do same thing.
>
> Why ?
>
>> In that way, we can also return "fail" in ext4_valid_block_bitmap()
>> method when we meet FLEX_BG.
>
> This does not make sense at all. Why do you suggest that we should
> "fail" in the case that we have FLEG_BG feature enabled (which is
> default btw) ?
>

I was thinking when we have FLEG_BG feature enabled, we actually don't
have valid bitmap in that block. So semantically , we don't have valid
block bitmap there.

>>
>>
>>
>> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
>> index dc5d572..366807a 100644
>> --- a/fs/ext4/balloc.c
>> +++ b/fs/ext4/balloc.c
>> @@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
>> super_block *sb,
>>                  * or it has to also read the block group where the bitmaps
>>                  * are located to verify they are set.
>>                  */
>> -               return 0;
>> +               return 1;
>>         }
>>         group_first_block = ext4_group_first_block_no(sb, block_group);
>>
>> @@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
>> *sb, ext4_group_t block_group,
>>                 return 1;
>>         }
>>         clear_buffer_new(bh);
>> -       /* Panic or remount fs read-only if block bitmap is invalid */
>> -       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>> +
>> +       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
>> +        return 0;
>
> This is wrong from multiple reasons. First of all you're not holding
> group lock so what is preventing others to actually initialize the
> bitmap before you return 0 ?
>
Got it.

> Secondly, uninit group will never get that far, because it'll be
> initialized in ext4_read_block_bitmap_nowait() and we will not
> actually need to wait for the buffer.
>

Thank you! Very helpful information.

Best,
Jon

> Thanks!
> -Lukas
>
>> +        }
>> +       /* Panic or remount fs read-only if block bitmap is invalid */
>> +       ext4_validate_block_bitmap(sb, desc, block_group, bh);
>>         /* ...but check for error just in case errors=continue. */
>>         return !buffer_verified(bh);
>>  }
>> --
>> 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
>>
--
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: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle bitmap verification differently

[Lukáš Czerner]

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

On Mon, 7 Oct 2013, jon ernst wrote:

> Date: Mon, 7 Oct 2013 10:03:55 -0400
> From: jon ernst <jonernst07@gmail.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: "linux-ext4@vger.kernel.org List" <linux-ext4@vger.kernel.org>
> Subject: Re: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait()
>     handle bitmap verification differently
> 
> On Mon, Oct 7, 2013 at 8:45 AM, Lukáš Czerner <lczerner@redhat.com> wrote:
> > On Thu, 3 Oct 2013, jon ernst wrote:
> >
> >> Date: Thu, 3 Oct 2013 22:45:06 -0400
> >> From: jon ernst <jonernst07@gmail.com>
> >> To: "linux-ext4@vger.kernel.org List" <linux-ext4@vger.kernel.org>
> >> Subject: ext4_wait_block_bitmap() and ext4_read_block_bitmap_nowait() handle
> >>     bitmap verification differently
> >>
> >> Hi,
> >
> > Hi Jon,
> >
> > Btw the patch has some issues and it seems to be badly formatted, or
> > even corrupted. You're also missing some Signed-off-by line and the
> > subject is not good either. Please see
> > Documentation/SubmittingPatches, use git to create patches and use
> > email client which does not automatically wrap your lines.
> 
> Thanks for your instruction. I will pay attention.
> >
> >>
> >> I found that ext4_wait_block_bitmap() and
> >> ext4_read_block_bitmap_nowait() handle bitmap verification
> >> differently.
> >> wait_block_bitmap() calls ext4_validate_block_bitmap() all the time.
> >> But  read_block_bitmap_nowait() checks EXT4_BG_BLOCK_UNINIT, if it
> >> meets, it will skip ext4_validate_block_bitmap()
> >>
> >> In my opinion, they'd better do same thing.
> >
> > Why ?
> >
> >> In that way, we can also return "fail" in ext4_valid_block_bitmap()
> >> method when we meet FLEX_BG.
> >
> > This does not make sense at all. Why do you suggest that we should
> > "fail" in the case that we have FLEG_BG feature enabled (which is
> > default btw) ?
> >
> 
> I was thinking when we have FLEG_BG feature enabled, we actually don't
> have valid bitmap in that block. So semantically , we don't have valid
> block bitmap there.

Well, yes. We do not have a valid bitmap simply because the bitmap
may not be there in the first place. And if there is no bitmap it
can not be invalid. So in the case there may be no bitmap we just
skip this one. I think that the comment in the code explains quite
well why we return 0, so I do not think there is a problem which
needs fixing.

Also this patch looks entirely untested. You can always use xfstests
on the ext4 file system with your changes to make sure that it does
not break anything.

Thanks!
-Lukas

> 
> >>
> >>
> >>
> >> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> >> index dc5d572..366807a 100644
> >> --- a/fs/ext4/balloc.c
> >> +++ b/fs/ext4/balloc.c
> >> @@ -319,7 +319,7 @@ static ext4_fsblk_t ext4_valid_block_bitmap(struct
> >> super_block *sb,
> >>                  * or it has to also read the block group where the bitmaps
> >>                  * are located to verify they are set.
> >>                  */
> >> -               return 0;
> >> +               return 1;
> >>         }
> >>         group_first_block = ext4_group_first_block_no(sb, block_group);
> >>
> >> @@ -472,8 +472,12 @@ int ext4_wait_block_bitmap(struct super_block
> >> *sb, ext4_group_t block_group,
> >>                 return 1;
> >>         }
> >>         clear_buffer_new(bh);
> >> -       /* Panic or remount fs read-only if block bitmap is invalid */
> >> -       ext4_validate_block_bitmap(sb, desc, block_group, bh);
> >> +
> >> +       if (desc->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)) {
> >> +        return 0;
> >
> > This is wrong from multiple reasons. First of all you're not holding
> > group lock so what is preventing others to actually initialize the
> > bitmap before you return 0 ?
> >
> Got it.
> 
> > Secondly, uninit group will never get that far, because it'll be
> > initialized in ext4_read_block_bitmap_nowait() and we will not
> > actually need to wait for the buffer.
> >
> 
> Thank you! Very helpful information.
> 
> Best,
> Jon
> 
> > Thanks!
> > -Lukas
> >
> >> +        }
> >> +       /* Panic or remount fs read-only if block bitmap is invalid */
> >> +       ext4_validate_block_bitmap(sb, desc, block_group, bh);
> >>         /* ...but check for error just in case errors=continue. */
> >>         return !buffer_verified(bh);
> >>  }
> >> --
> >> 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
> >>
> --
> 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
>