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 10:07 PM, Ritesh Harjani wrote:
>> 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 :) 
>> 
> Thanks for the reivew. Since more improvement is needed, I would like to
> define ext4_mark_context as you suggested in previous version:
>     ext4_mark_context {
>         ext4_group_t mc_group;          /* block group */
>         ext4_grpblk_t mc_clblk;	    /* block in cluster units */
>         ext4_grpblk_t mc_cllen;	    /* len in cluster units */
>         ext4_grpblk_t mc_clupdates;     /* number of clusters marked/unmarked */
>         unsigned int mc_flags;          /* flags ... */
>         bool mc_state;                  /* to set or unset state */
>     };
> And super_block and handle are passed as arguments.
>
> Besides, as we will pass a lot arguments in prepare function anyway. What
> about simply passing all arguments to ext4_mb_prepare_mark_context
> directly:
> static inline void ext4_mb_mark_context(handle_t *handle,
>                                                 struct super_block *sb,
>                                                 int state,
>                                                 ext4_group_t group,
>                                                 ext4_grpblk_t blkoff,
>                                                 ext4_grpblk_t len,
>                                                 int flags,
>                                                 ext4_grpblk_t *changed)
> Look forward to your reply. Thanks!

Sounds good to me. Thanks

-ritesh