Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> writes: > on 9/27/2023 4:49 PM, Ritesh Harjani wrote: >> Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> writes: >> >>> There are several reasons to add a general function ext4_mb_mark_context >>> to update block bitmap and group descriptor on disk: >>> 1. pair behavior of alloc/free bits. For example, >>> ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups >>> in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this. >>> 2. remove repeat code to read from disk, update and write back to disk. >>> 3. reduce future unit test mocks to catch real IO to update structure >>> on disk. >>> >>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> >>> --- >>> fs/ext4/mballoc.c | 147 ++++++++++++++++++++++++---------------------- >>> 1 file changed, 77 insertions(+), 70 deletions(-) >>> >>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >>> index cf09adfbaf11..e1320eea46e9 100644 >>> --- a/fs/ext4/mballoc.c >>> +++ b/fs/ext4/mballoc.c >>> @@ -3953,6 +3953,80 @@ void ext4_exit_mballoc(void) >>> ext4_groupinfo_destroy_slabs(); >>> } >>> >>> +static int >>> +ext4_mb_mark_context(struct super_block *sb, bool state, ext4_group_t group, >>> + ext4_grpblk_t blkoff, ext4_grpblk_t len) >> >> >> ext4_grpblk_t is defined as int. >> /* data type for block offset of block group */ >> typedef int ext4_grpblk_t; >> >> I think len should be unsigned int (u32) here. >> > Hi Ritesh, thanks for reply and a lot suggestions to this patch and other > patches in this series. > I define len as ext4_grpblk_t as I think ext4_grpblk_t is supposed to fit > block or cluster number of single group. > At different places the use of datatype for no. of blocks/clusters within a group gets very confusing :( However, IMO ext4_grpblk_t should be fine for using len argument here. I did respond about that while reviewing in some later patches [1] [1]: https://lore.kernel.org/linux-ext4/87r0mkey45.fsf@xxxxxxx/ So, I don't think we need any changes to this patch. Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> Also overall the series looks good. There are just some minor changes suggested in 1st patch and some commit msg updates suggested for other changes. If you send a v8, then I think that looks good to be picked up :) Thanks a lot for working on it & the suggested changes! -ritesh
