Re: [PATCH v2 1/3] ocfs2: give ocfs2 the ability to reclaim suballoc free bg

[Heming Zhao <heming.zhao@suse.com>]

On 9/2/24 19:25, Joseph Qi wrote:
> 
> 
> On 7/29/24 4:04 PM, Heming Zhao wrote:
>> The current ocfs2 code can't reclaim suballocator block group space.
>> This cause ocfs2 to hold onto a lot of space in some cases. for example,
>> when creating lots of small files, the space is held/managed by
>> '//inode_alloc'. After the user deletes all the small files, the space
>> never returns to '//global_bitmap'. This issue prevents ocfs2 from
>> providing the needed space even when there is enough free space in a
>> small ocfs2 volume.
>> This patch gives ocfs2 the ability to reclaim suballoc free space when
>> the block group is free. For performance reasons, ocfs2 doesn't release
>> the first suballocator block group.
>>
>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>> ---
>>   fs/ocfs2/suballoc.c | 211 ++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 206 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
>> index f7b483f0de2a..1b64f4c87607 100644
>> --- a/fs/ocfs2/suballoc.c
>> +++ b/fs/ocfs2/suballoc.c
>> @@ -294,6 +294,60 @@ static int ocfs2_validate_group_descriptor(struct super_block *sb,
>>   	return ocfs2_validate_gd_self(sb, bh, 0);
>>   }
>>   
>> +/*
>> + * hint gd may already be released in _ocfs2_free_suballoc_bits(),
>> ... ...
>> +	alloc_bh = main_bm_bh;
>> +	fe = (struct ocfs2_dinode *) alloc_bh->b_data;
>> +	cl = &fe->id2.i_chain;
>> +	old_bg_contig_free_bits = 0;
>> +	brelse(group_bh);
>> +	group_bh = NULL;
>> +	start_bit = bg_start_bit;
>> +	undo_fn = _ocfs2_clear_bit;
>> +
>> +	/* reclaim clusters to global_bitmap */
>> +	goto reclaim;
>> +
>>   bail:
>> +	if (free_main_bm_bh) {
>> +		ocfs2_inode_unlock(main_bm_inode, 1);
>> +		brelse(main_bm_bh);
>> +	}
>> +	if (free_main_bm_inode) {
>> +		inode_unlock(main_bm_inode);
>> +		iput(main_bm_inode);
>> +	}
> 
> You've add too much logic into _ocfs2_free_suballoc_bits() and make it
> hard to review.
> Could you please factor out a new function and describe it clearly?
> e.g. why we need this function and how to achieve it.
> BTW, it seems that it contains a big loop, so please explictly specify
> the end condition.
> 
> Cc lkml as well.
> 
> Thanks,
> Joseph

Give me some time to do the factor out job.

- Heming

> 
>>   	brelse(group_bh);
>>   	return status;
>>   }
> 
>