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

[Kemeng Shi]

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!
> Since otherwise the refactoring changes looks good to me.
> Please feel free to add -
> 
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
> 
> Thanks! 
> -ritesh
>