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

[Heming Zhao]

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@xxxxxxxx>
> > ---
> >   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;
> >   }