On Mon, 7 Oct 2013, jon ernst wrote:
> Date: Mon, 7 Oct 2013 10:03:55 -0400
> From: jon ernst <jonernst07@xxxxxxxxx>
> To: Lukáš Czerner <lczerner@xxxxxxxxxx>
> Cc: "linux-ext4@xxxxxxxxxxxxxxx List" <linux-ext4@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx> wrote:
> > On Thu, 3 Oct 2013, jon ernst wrote:
> >
> >> Date: Thu, 3 Oct 2013 22:45:06 -0400
> >> From: jon ernst <jonernst07@xxxxxxxxx>
> >> To: "linux-ext4@xxxxxxxxxxxxxxx List" <linux-ext4@xxxxxxxxxxxxxxx>
> >> 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@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
