Re: [PATCH v6 01/11] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb

[IBM]

Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> writes:

> on 8/31/2023 8:33 PM, Ritesh Harjani wrote:
>> Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> writes:
>> 
>> Hello Kemeng,
>> 
>>> There are several reasons to add a general function to update block
>>> bitmap and group descriptor on disk:
>> 
>> ... named ext4_mb_mark_context(<params>)
>> 
>>> 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>
>>> Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
>>> ---
>>>  fs/ext4/mballoc.c | 169 +++++++++++++++++++++++++++-------------------
>>>  1 file changed, 99 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>> index c91db9f57524..e2be572deb75 100644
>>> --- a/fs/ext4/mballoc.c
>>> +++ b/fs/ext4/mballoc.c
>>> @@ -3952,6 +3952,100 @@ void ext4_exit_mballoc(void)
>>>  	ext4_groupinfo_destroy_slabs();
>>>  }
>>>  
>>> +/*
>>> + * Collect global setting to reduce the number of variable passing to
>>> + * ext4_mb_mark_context. Pass target group blocks range directly to
>>> + * reuse the prepared global setting for multiple block ranges and
>>> + * to show clearly the specific block range will be marked.
>>> + */
>>> +struct ext4_mark_context {
>>> +	struct super_block *sb;
>>> +	int state;
>>> +};
>> 
>> This structure definition does not reflect of it's naming.
>> Why can't we also add cblk & clen, flags to it?
>> 
>> I think the idea of defining a new function named
>> ext4_mb_prepare_mark_context() was that we can prepare "struct ext4_mark_context"
>> with different cblk, clen & flags arguments for cases where we might
>> have to call ext4_mb_mark_context() more than once in the same function
>> or call ext4_mb_mark_context() anywhere but at the start of the function.
>> 
>> As I see it in the current series, we are calling
>> ext4_mb_prepare_mark_context() at the start of every function. Just for
>> this purpose we don't need an extra function, right? That we can directly do
>> at the time of declaring a structure variable itself (like how you did
>> in previous version)
>> 
> Hi Ritesh, thanks for reply. The ext4_mark_context structure aims to reduce
> variable passing to ext4_mb_mark_context. If we have to prepare a lot
> member in ext4_mb_prepare_mark_context, then too many variables issue occurs
> in ext4_mb_prepare_mark_context.
> The name of ext4_mark_context maybe not proper. Actually I want a structure
> to collect information which is not strongly relevant to mark blk bits. In
> this way, we can initialize them at beginning of function, then there is no
> need to pay attention to them or to pass them respectively in each call to
> ext4_mb_mark_context. Instead, we foucus on the useful information called
> with ext4_mb_mark_context.
> This design also achive the goal to define ext4_mb_mark_context once for
> multiple use in the same function as ext4_mark_context unlikely changes
> after initialization at beginning.
>> What do you think of the approach where we add cblk, clen & flags
>> variables to ext4_mark_context()? Do you see any problems/difficulties
>> with that design?
>> 
> The providing desgin looks good to me. Please let me konw if you still
> perfre this and I will do this in next version. Thanks!
>

I would have still preferred, the block and len arguments inside struct
ext4_mark_context, because that better explains the use and definition of
structure and it's prepare function.
However, since this is not any functionality change, I am fine if you
prefer the current design(as you mentioned above).
We can always discuss & change it later too :) 

Since otherwise the refactoring changes looks good to me.
Please feel free to add -

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>

Thanks! 
-ritesh