Re: [PATCH v7 02/12] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb

[Kemeng Shi <shikemeng@huaweicloud.com>]

on 9/28/2023 12:42 PM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@huaweicloud.com> writes:
> 
>> on 9/27/2023 4:49 PM, Ritesh Harjani wrote:
>>> Kemeng Shi <shikemeng@huaweicloud.com> 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@huaweicloud.com>
>>>> ---
>>>>  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@doe.com/
> 
> So, I don't think we need any changes to this patch. 
>     Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> 
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
>