On Wed, 6 Jul 2011, Kyungmin Park wrote:
> Hi Lukas,
>
> During code review batched discard support at ext4. I wonder why do
> you init the uninitialized block group during batched discard.
> As you know uninitialized block group mean that there's no operation
> at these blocks.
> So no need to trim it at all.
What you're describing is another flag, namely EXT4_BG_BLOCK_UNINIT,
which tells us that there was no allocation from that block bitmap since
the mkfs (as Amir already pointed out). Flag
EXT4_GROUP_INFO_NEED_INIT_BIT simply states that there is no buddy
initialized for this group.
That said the code is perfectly fine, and it should not affect even the
e2fsck which uses EXT4_BG_BLOCK_UNINIT to skip not used block groups
since we only change it on allocations.
It is true that after the commit
78944086663e6c1b03f3d60bf7610128149be5fc ext4: only load buddy bitmap in
ext4_trim_fs() when it is needed
we do not longer need to initialize the buddy right away, but wait ontil
it is really needed. Actually we do not need it at all, because is when
we are going to load the buddy the ext4_mb_load_buddy() will check for
the EXT4_GROUP_INFO_NEED_INIT_BIT and will initialize the buddy for us.
Yongqiang pointed out that we might use EXT4_BG_BLOCK_UNINIT to skip
group as well, but I do not think that it is a good idea, since the
initial discard at mkfs time might not be done (we just do not know it),
so any assumption like this are not right. Moreover there are patches
from Tao Ma which adds the code for skipping groups which has not been
freed from since the last fitrim call. Search the list for [PATCH 0/4
RESEND] ext4 trim bug fixes and improvement.
Thanks for pointing that out. I'll send the patch.
-Lukas
>
> How do you think?
>
> int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> {
> ...
>
> for (group = first_group; group <= last_group; group++) {
> grp = ext4_get_group_info(sb, group);
> /* We only do this if the grp has never been initialized */
> if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
> ret = ext4_mb_init_group(sb, group);
> if (ret)
> break;
> }
> ...
> }
> ...
> }
>
> Thank you,
> Kyungmin Park
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
