Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Thu, 28 Sep 2023 10:12:29 +0530
Message-Id: <87sf6yrjre.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Kemeng Shi <shikemeng@huaweicloud.com>, tytso@mit.edu,
        adilger.kernel@dilger.ca
Cc:     ojaswin@linux.ibm.com, linux-ext4@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 02/12] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb
In-Reply-To: <309d082f-45b9-9dac-9921-6c3d44de17a7@huaweicloud.com>
Precedence: bulk

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>

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