on 7/22/2023 2:24 PM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> writes:
>
>> There are several reasons to add a general function 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.
>
> Thanks for the cleanup and sorry that I am starting to review this
> series only now. However I do have some review comments to understand a
> bit more on the patch series.
>
>>
>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
>> Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
>> ---
>> fs/ext4/mballoc.c | 157 +++++++++++++++++++++++++---------------------
>> 1 file changed, 87 insertions(+), 70 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index a2475b8c9fb5..58864a9116c0 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -3948,6 +3948,86 @@ void ext4_exit_mballoc(void)
>> ext4_groupinfo_destroy_slabs();
>> }
>>
>> +struct ext4_mark_context {
>> + struct super_block *sb;
>> + int state;
>> +};
>
> It's not totally clear the intention behind this structure from above
> since it lacking any comments.
>
> Can you please help me understand why do we need this.
> I still don't know whether we require this structure and what is it's
> purpose. Is it only for reducing the number of variable passing?
Exactly. It's only for reducing the number of variable passing.
> Let me do more reading...
>
> ...On more reading, I was previous considering to rename it to something
> like ext4_mb_mark_context, but then I realized the naming of this is
> something similar to ext4_allocation_context. So we may keep the naming
> as is.
Exactly again. The ext4_mark_context is based on ext4_allocation_context.
> So since this structure, presumably, is used for marking blk bits for
> mballoc. Why don't we pass useful information which is relevant for
> this operation like -
>
> 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 */
> };
>
> Maybe, super_block and handle we can pass as an argument as those doesn't
> define the ext4_mark_context for mballoc.
Actually, I try to put stable arguments need by bit mark into
ext4_mark_context then ext4_mark_context could be initialized once and used
multiple times. For example, if there is function to mark multiple bit
fragments, it will use ext4_allocation_context as:
struct ext4_mark_context mc = {
/* initialization */
}
/* mark fragment1 */
ext4_mb_mark_group_bb(&mc, group1, blkoff1, len1);
/* mark fragment2 */
ext4_mb_mark_group_bb(&mc, group2, blkoff2, len2);
And I thinks these stable arguments match "context" meaning which bit
mark needed to work around :).
Put bit mark relevant information into ext4_mark_context is absolutely
a great choice. I will arrange ext4_mark_context in this way if you
still prefer this.
> Since this structure is prepared not at the begining of any function, we
> may need a prepare function for it. e.g.
>
> static void ext4_mb_prepare_mark_context(&mc, ...)
> static int ext4_mb_mark_context(sb, handle, &mc); (instead of ext4_mb_mark_group_bb())
>
> Does this sounds better to you? Thoughts?
>
Yes, prepare function is a great idea. I will add this in next version.
> Otherwise I think having a common function for mb_mark_context looks
> like a nice cleanup.
>
Thanks! this means a lot to me!
--
Best wishes
Kemeng Shi
