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] [thread-next>] [day] [month] [year] [list]

Message-Id: <87sf6yrjre.fsf@doe.com>
Date:   Thu, 28 Sep 2023 10:12:29 +0530
From:   Ritesh Harjani (IBM) <ritesh.list@...il.com>
To:     Kemeng Shi <shikemeng@...weicloud.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

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>

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