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

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

[Joseph Qi]

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(),
> + * we first check gd descriptor signature, then do the
> + * ocfs2_read_group_descriptor() jobs.
> + */
> +static int ocfs2_read_hint_group_descriptor(struct inode *inode, struct ocfs2_dinode *di,
> +				u64 gd_blkno, struct buffer_head **bh)
> +{
> +	int rc;
> +	struct buffer_head *tmp = *bh;
> +	struct ocfs2_group_desc *gd;
> +
> +	rc = ocfs2_read_block(INODE_CACHE(inode), gd_blkno, &tmp, NULL);
> +	if (rc)
> +		goto out;
> +
> +	gd = (struct ocfs2_group_desc *) tmp->b_data;
> +	if (!OCFS2_IS_VALID_GROUP_DESC(gd)) {
> +		/*
> +		 * Invalid gd cache was set in ocfs2_read_block(),
> +		 * which will affect block_group allocation.
> +		 * Path:
> +		 * ocfs2_reserve_suballoc_bits
> +		 *  ocfs2_block_group_alloc
> +		 *   ocfs2_block_group_alloc_contig
> +		 *    ocfs2_set_new_buffer_uptodate
> +		 */
> +		ocfs2_remove_from_cache(INODE_CACHE(inode), tmp);
> +		rc = -EIDRM;
> +		goto free_bh;
> +	}
> +
> +	if (!buffer_jbd(tmp)) {
> +		rc = ocfs2_validate_group_descriptor(inode->i_sb, tmp);
> +		if (rc)
> +			goto free_bh;
> +	}
> +
> +	rc = ocfs2_validate_gd_parent(inode->i_sb, di, tmp, 0);
> +	if (rc)
> +		goto free_bh;
> +
> +	/* If ocfs2_read_block() got us a new bh, pass it up. */
> +	if (!*bh)
> +		*bh = tmp;
> +
> +	return rc;
> +
> +free_bh:
> +	brelse(tmp);
> +out:
> +	return rc;
> +}
> +
>  int ocfs2_read_group_descriptor(struct inode *inode, struct ocfs2_dinode *di,
>  				u64 gd_blkno, struct buffer_head **bh)
>  {
> @@ -1730,10 +1784,11 @@ static int ocfs2_search_one_group(struct ocfs2_alloc_context *ac,
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)ac->ac_bh->b_data;
>  	struct inode *alloc_inode = ac->ac_inode;
>  
> -	ret = ocfs2_read_group_descriptor(alloc_inode, di,
> +	ret = ocfs2_read_hint_group_descriptor(alloc_inode, di,
>  					  res->sr_bg_blkno, &group_bh);
>  	if (ret < 0) {
> -		mlog_errno(ret);
> +		if (ret != -EIDRM)
> +			mlog_errno(ret);
>  		return ret;
>  	}
>  
> @@ -1961,6 +2016,7 @@ static int ocfs2_claim_suballoc_bits(struct ocfs2_alloc_context *ac,
>  		goto bail;
>  	}
>  
> +	/* the hint bg may already be released, we quiet search this group. */
>  	res->sr_bg_blkno = hint;
>  	if (res->sr_bg_blkno) {
>  		/* Attempt to short-circuit the usual search mechanism
> @@ -1971,12 +2027,16 @@ static int ocfs2_claim_suballoc_bits(struct ocfs2_alloc_context *ac,
>  						min_bits, res, &bits_left);
>  		if (!status)
>  			goto set_hint;
> +		if (status == -EIDRM) {
> +			res->sr_bg_blkno = 0;
> +			goto chain_search;
> +		}
>  		if (status < 0 && status != -ENOSPC) {
>  			mlog_errno(status);
>  			goto bail;
>  		}
>  	}
> -
> +chain_search:
>  	cl = (struct ocfs2_chain_list *) &fe->id2.i_chain;
>  
>  	victim = ocfs2_find_victim_chain(cl);
> @@ -2077,6 +2137,12 @@ int ocfs2_claim_metadata(handle_t *handle,
>  	return status;
>  }
>  
> +/*
> + * after ocfs2 has the ability to release block group unused space,
> + * the ->ip_last_used_group may be invalid. so this function returns
> + * ac->ac_last_group need to verify.
> + * refer the 'hint' in ocfs2_claim_suballoc_bits() for more details.
> + */
>  static void ocfs2_init_inode_ac_group(struct inode *dir,
>  				      struct buffer_head *parent_di_bh,
>  				      struct ocfs2_alloc_context *ac)
> @@ -2534,6 +2600,16 @@ static int _ocfs2_free_suballoc_bits(handle_t *handle,
>  	struct ocfs2_group_desc *group;
>  	__le16 old_bg_contig_free_bits = 0;
>  
> +	struct buffer_head *main_bm_bh = NULL;
> +	struct inode *main_bm_inode = NULL;
> +	struct ocfs2_super *osb = OCFS2_SB(alloc_inode->i_sb);
> +	struct ocfs2_chain_rec *rec;
> +	u64 start_blk;
> +	int idx, i, next_free_rec, len = 0;
> +	int free_main_bm_inode = 0, free_main_bm_bh = 0;
> +	u16 bg_start_bit;
> +
> +reclaim:
>  	/* The alloc_bh comes from ocfs2_free_dinode() or
>  	 * ocfs2_free_clusters().  The callers have all locked the
>  	 * allocator and gotten alloc_bh from the lock call.  This
> @@ -2577,13 +2653,138 @@ static int _ocfs2_free_suballoc_bits(handle_t *handle,
>  		goto bail;
>  	}
>  
> -	le32_add_cpu(&cl->cl_recs[le16_to_cpu(group->bg_chain)].c_free,
> -		     count);
> +	idx = le16_to_cpu(group->bg_chain);
> +	rec = &(cl->cl_recs[idx]);
> +
> +	le32_add_cpu(&rec->c_free, count);
>  	tmp_used = le32_to_cpu(fe->id1.bitmap1.i_used);
>  	fe->id1.bitmap1.i_used = cpu_to_le32(tmp_used - count);
>  	ocfs2_journal_dirty(handle, alloc_bh);
>  
> +	/* bypass: global_bitmap, not empty rec, first item in cl_recs[] */
> +	if (ocfs2_is_cluster_bitmap(alloc_inode) ||
> +	    (le32_to_cpu(rec->c_free) != (le32_to_cpu(rec->c_total) - 1)) ||
> +	    (le16_to_cpu(cl->cl_next_free_rec) == 1)) {
> +		goto bail;
> +	}
> +
> +	status = ocfs2_extend_trans(handle,
> +				ocfs2_calc_group_alloc_credits(osb->sb,
> +						 le16_to_cpu(cl->cl_cpg)));
> +	if (status) {
> +		mlog_errno(status);
> +		goto bail;
> +	}
> +	status = ocfs2_journal_access_di(handle, INODE_CACHE(alloc_inode),
> +					 alloc_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> +	if (status < 0) {
> +		mlog_errno(status);
> +		goto bail;
> +	}
> +
> +	/*
> +	 * Only clear the rec item in-place.
> +	 *
> +	 * If idx is not the last, we don't compress (remove the empty item)
> +	 * the cl_recs[]. If not, we need to do lots jobs.
> +	 *
> +	 * Compress cl_recs[] code example:
> +	 * if (idx != cl->cl_next_free_rec - 1)
> +	 * 	memmove(&cl->cl_recs[idx], &cl->cl_recs[idx + 1],
> +	 * 		sizeof(struct ocfs2_chain_rec) *
> +	 * 		(cl->cl_next_free_rec - idx - 1));
> +	 * for(i = idx; i < cl->cl_next_free_rec-1; i++) {
> +	 * 	group->bg_chain = "later group->bg_chain";
> +	 * 	group->bg_blkno = xxx;
> +	 * 	... ...
> +	 * }
> +	 */
> +
> +	tmp_used = le32_to_cpu(fe->id1.bitmap1.i_total);
> +	fe->id1.bitmap1.i_total = cpu_to_le32(tmp_used - le32_to_cpu(rec->c_total));
> +
> +	/* Substraction 1 for the block group itself */
> +	tmp_used = le32_to_cpu(fe->id1.bitmap1.i_used);
> +	fe->id1.bitmap1.i_used = cpu_to_le32(tmp_used - 1);
> +
> +	tmp_used = le32_to_cpu(fe->i_clusters);
> +	fe->i_clusters = cpu_to_le32(tmp_used - le16_to_cpu(cl->cl_cpg));
> +
> +	spin_lock(&OCFS2_I(alloc_inode)->ip_lock);
> +	OCFS2_I(alloc_inode)->ip_clusters -= le32_to_cpu(fe->i_clusters);
> +	fe->i_size = cpu_to_le64(ocfs2_clusters_to_bytes(alloc_inode->i_sb,
> +					     le32_to_cpu(fe->i_clusters)));
> +	spin_unlock(&OCFS2_I(alloc_inode)->ip_lock);
> +	i_size_write(alloc_inode, le64_to_cpu(fe->i_size));
> +	alloc_inode->i_blocks = ocfs2_inode_sector_count(alloc_inode);
> +
> +	ocfs2_journal_dirty(handle, alloc_bh);
> +	ocfs2_update_inode_fsync_trans(handle, alloc_inode, 0);
> +
> +	start_blk = le64_to_cpu(rec->c_blkno);
> +	count = le32_to_cpu(rec->c_total) / le16_to_cpu(cl->cl_bpc);
> +
> +	/*
> +	 * If the rec is the last one, let's compress the chain list by
> +	 * removing the empty cl_recs[] at the end.
> +	 */
> +	next_free_rec = le16_to_cpu(cl->cl_next_free_rec);
> +	if (idx == (next_free_rec - 1)) {
> +		len++; /* the last item */
> +		for (i = (next_free_rec - 2); i > 0; i--) {
> +			if (cl->cl_recs[i].c_free == cl->cl_recs[i].c_total)
> +				len++;
> +			else
> +				break;
> +		}
> +	}
> +	le16_add_cpu(&cl->cl_next_free_rec, -len);
> +
> +	rec->c_free = 0;
> +	rec->c_total = 0;
> +	rec->c_blkno = 0;
> +	ocfs2_remove_from_cache(INODE_CACHE(alloc_inode), group_bh);
> +	memset(group, 0, sizeof(struct ocfs2_group_desc));
> +
> +	/* prepare job for reclaim clusters */
> +	main_bm_inode = ocfs2_get_system_file_inode(osb,
> +						    GLOBAL_BITMAP_SYSTEM_INODE,
> +						    OCFS2_INVALID_SLOT);
> +	if (!main_bm_inode)
> +		goto bail; /* ignore the error in reclaim path */
> +
> +	inode_lock(main_bm_inode);
> +	free_main_bm_inode = 1;
> +
> +	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
> +	if (status < 0)
> +		goto bail; /* ignore the error in reclaim path */
> +	free_main_bm_bh = 1;
> +
> +	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
> +				     &bg_start_bit);
> +	alloc_inode = main_bm_inode;
> +	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

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

Re: [PATCH v2 2/3] ocfs2: detect released suballocator bg for fh_to_[dentry|parent]

[Joseph Qi]

On 7/29/24 4:04 PM, Heming Zhao wrote:
> After ocfs2 has the ability to reclaim suballoc free bg, the
> suballocator block group may be released. This change makes xfstest
> case 426 failed.
> 
> The existed code call stack:
> 
> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
>  ocfs2_get_dentry
>   ocfs2_test_inode_bit
>    ocfs2_test_suballoc_bit
>     ocfs2_read_group_descriptor
>      + read released bg, triggers validate fails, then cause -EROFS
> 
> how to fix:
> The read bg failure is expectation, we should ignore this error.
> 
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/suballoc.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index 1b64f4c87607..dc421f55ed8f 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -3037,7 +3037,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>  	struct ocfs2_group_desc *group;
>  	struct buffer_head *group_bh = NULL;
>  	u64 bg_blkno;
> -	int status;
> +	int status, quiet = 0;
>  
>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
>  				      (unsigned int)bit);
> @@ -3053,11 +3053,16 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>  
>  	bg_blkno = group_blkno ? group_blkno :
>  		   ocfs2_which_suballoc_group(blkno, bit);
> -	status = ocfs2_read_group_descriptor(suballoc, alloc_di, bg_blkno,
> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
>  					     &group_bh);
>  	if (status < 0) {
> -		mlog(ML_ERROR, "read group %llu failed %d\n",
> -		     (unsigned long long)bg_blkno, status);
> +		if (status == -EIDRM) {

I don't think EIDRM is a proper error code in this case.
We can pass a output parameter or a flag to indicate a released one.
And please explicitly specify that this can be treated normally in
the related code logic of ocfs2_read_hint_group_descriptor().

Thanks,
Joseph

> +			quiet = 1;
> +			status = -EINVAL;
> +		} else {
> +			mlog(ML_ERROR, "read group %llu failed %d\n",
> +					(unsigned long long)bg_blkno, status);
> +		}
>  		goto bail;
>  	}
>  
> @@ -3067,7 +3072,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>  bail:
>  	brelse(group_bh);
>  
> -	if (status)
> +	if (status && (!quiet))
>  		mlog_errno(status);
>  	return status;
>  }
> @@ -3087,7 +3092,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>   */
>  int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res)
>  {
> -	int status;
> +	int status, quiet = 0;
>  	u64 group_blkno = 0;
>  	u16 suballoc_bit = 0, suballoc_slot = 0;
>  	struct inode *inode_alloc_inode;
> @@ -3129,8 +3134,12 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res)
>  
>  	status = ocfs2_test_suballoc_bit(osb, inode_alloc_inode, alloc_bh,
>  					 group_blkno, blkno, suballoc_bit, res);
> -	if (status < 0)
> -		mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
> +	if (status < 0) {
> +		if (status == -EINVAL)
> +			quiet = 1;
> +		else
> +			mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
> +	}
>  
>  	ocfs2_inode_unlock(inode_alloc_inode, 0);
>  	inode_unlock(inode_alloc_inode);
> @@ -3138,7 +3147,7 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res)
>  	iput(inode_alloc_inode);
>  	brelse(alloc_bh);
>  bail:
> -	if (status)
> +	if (status && !quiet)
>  		mlog_errno(status);
>  	return status;
>  }

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