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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9eacc6e6-ae33-ac1e-cc7c-2defcc0517d9@huaweicloud.com>
Date:   Thu, 28 Sep 2023 15:45:37 +0800
From:   Kemeng Shi <shikemeng@...weicloud.com>
To:     Ritesh Harjani <ritesh.list@...il.com>, tytso@....edu,
        adilger.kernel@...ger.ca
Cc:     ojaswin@...ux.ibm.com, linux-ext4@...r.kernel.org,
        linux-kernel@...r.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



on 9/28/2023 12:42 PM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@...weicloud.com> writes:
> 
>> on 9/27/2023 4:49 PM, Ritesh Harjani wrote:
>>> Kemeng Shi <shikemeng@...weicloud.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@...weicloud.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@...il.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
>