on 9/28/2023 12:42 PM, Ritesh Harjani wrote: > 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> > Sorry for missing this. > 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! Thanks a lot for your suggestion and patience! I will send a v8 include all changes that you suggested. Thanks! > > -ritesh >
