[PATCH RESEND v4 0/2] ocfs2: give ocfs2 the ability to reclaim suballocator free bg

[PATCH RESEND v4 0/2] ocfs2: give ocfs2 the ability to reclaim suballocator free bg

[Heming Zhao]

why resend?
- the v4 summary description in the cover letter is incorrect.

v4:

Most of the changes involve revising comments. For the code itself, there are
function renames and parameter usage adjustments, but no changes to the code
logic.

For patch [1/2]:
- Based on Joseph's v3 review comments:
  1. Modify the caller to initialize the ocfs2_read_hint_group_descriptor()
     input parameter '*released'.
  2. Rename the _reclaim_to_main_bm() to _ocfs2_reclaim_suballoc_to_main()
  3. Change the text "not empty rec" to "non empty rec"

- Revise the comments preceding the function _ocfs2_reclaim_suballoc_to_main().

- For the question: "How to distinguish the release case or a bug?"
  I write a comment before ocfs2_read_hint_group_descriptor().

- Revise the commit log to make it clearer.

For patch [2/2]:
- Revise the commit log to make it clearer.

Remove the v3 patch: "ocfs2: adjust spinlock_t ip_lock protection scope"
- Reason: This patch belongs to a different topic/job and should be
          handled separately.

v3:

For patch [1/3]:
- Factor out the reclaim code into a new function '_reclaim_to_main_bm'.
- The function ocfs2_read_hint_group_descriptor doesn't return -EIDRM
  when the group descriptor is invalid. The new code logic returns 0,
  and the input parameter '*released' is set to 1.

For patch [2/3]:
- Modify the code to follow the new logic of ocfs2_read_hint_group_descriptor
  as introduced in patch [1/3].

For patch [3/3]:
- No new changes.

v3 patch has passed the xfstests:
./check -g quick -T -b -s ocfs2 -e generic/032 -e generic/076 \
-e generic/081 -e generic/266 -e generic/272 -e generic/281 \
-e generic/331 -e generic/338 -e generic/347 -e generic/361 \
-e generic/479 -e generic/480 -e generic/628 -e generic/629 \
-e generic/648 -e generic/650

v2:

Create 2 new patches:
- ocfs2: detect released suballocator bg for fh_to_[dentry|parent]
- ocfs2: adjust spinlock_t ip_lock protection scope

In ocfs2_read_hint_group_descriptor()
- bypass the validation of GD when the BH is already managed by jbd2.

In _ocfs2_free_suballoc_bits()
- Move up the position of the vars 'idx' & 'rec'.
- Move up the position of the ocfs2_journal_dirty.
- Use le[16|32]_to_cpu() to access cl/fe/rec vars.
- Add error handling for calling ocfs2_extend_trans().
- adjust spin_lock ->ip_lock protection scope.
- Follow Glass's review comments, add 'comment' & 'else-break' for the
  'for-loop'.

v1:

Only create patch:
- ocfs2: give ocfs2 the ability to reclaim suballoc free bg

Heming Zhao (2):
  ocfs2: give ocfs2 the ability to reclaim suballoc free bg
  ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

 fs/ocfs2/suballoc.c | 336 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 317 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH RESEND v4 1/2] ocfs2: give ocfs2 the ability to reclaim suballocator free bg

[Heming Zhao]

The current ocfs2 code can't reclaim suballocator block group space.
In some cases, this causes ocfs2 to hold onto a lot of space.
For example, when creating lots of small files, the space is held/managed
by the '//inode_alloc'. After the user deletes all the small files,
the space never returns to the '//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 suballocator free space
when the block group is freed. For performance reasons, this patch keeps
the first suballocator block group active.

Signed-off-by: Heming Zhao <heming.zhao@suse.com>
Reviewed-by: Su Yue <glass.su@suse.com>
---
 fs/ocfs2/suballoc.c | 308 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 299 insertions(+), 9 deletions(-)

diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index 6ac4dcd54588..de2f09217142 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -294,6 +294,74 @@ static int ocfs2_validate_group_descriptor(struct super_block *sb,
 	return ocfs2_validate_gd_self(sb, bh, 0);
 }
 
+/*
+ * The hint group descriptor (gd) may already have been released
+ * in _ocfs2_free_suballoc_bits(). We first check the gd signature,
+ * then perform the standard ocfs2_read_group_descriptor() jobs.
+ *
+ * If the gd signature is invalid, we return 'rc=0' and set
+ * '*released=1'. The caller is expected to handle this specific case.
+ * Otherwise, we return the actual error code.
+ *
+ * We treat gd signature corruption case as a release case. The
+ * caller ocfs2_claim_suballoc_bits() will use ocfs2_search_chain()
+ * to search each gd block. The code will eventually find this
+ * corrupted gd block - Late, but not missed.
+ *
+ * Note:
+ * The caller is responsible for initializing the '*released' status.
+ */
+static int ocfs2_read_hint_group_descriptor(struct inode *inode,
+			struct ocfs2_dinode *di, u64 gd_blkno,
+			struct buffer_head **bh, int *released)
+{
+	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);
+		*released = 1; /* we return 'rc=0' for this case */
+		goto free_bh;
+	}
+
+	/* below jobs same with ocfs2_read_group_descriptor() */
+	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)
 {
@@ -1724,7 +1792,7 @@ static int ocfs2_search_one_group(struct ocfs2_alloc_context *ac,
 				  u32 bits_wanted,
 				  u32 min_bits,
 				  struct ocfs2_suballoc_result *res,
-				  u16 *bits_left)
+				  u16 *bits_left, int *released)
 {
 	int ret;
 	struct buffer_head *group_bh = NULL;
@@ -1732,9 +1800,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,
-					  res->sr_bg_blkno, &group_bh);
-	if (ret < 0) {
+	ret = ocfs2_read_hint_group_descriptor(alloc_inode, di,
+				res->sr_bg_blkno, &group_bh, released);
+	if (*released) {
+		return 0;
+	} else if (ret < 0) {
 		mlog_errno(ret);
 		return ret;
 	}
@@ -1949,6 +2019,7 @@ static int ocfs2_claim_suballoc_bits(struct ocfs2_alloc_context *ac,
 				     struct ocfs2_suballoc_result *res)
 {
 	int status;
+	int released = 0;
 	u16 victim, i;
 	u16 bits_left = 0;
 	u64 hint = ac->ac_last_group;
@@ -1975,6 +2046,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
@@ -1982,7 +2054,12 @@ static int ocfs2_claim_suballoc_bits(struct ocfs2_alloc_context *ac,
 		 * allocation group. This helps us maintain some
 		 * contiguousness across allocations. */
 		status = ocfs2_search_one_group(ac, handle, bits_wanted,
-						min_bits, res, &bits_left);
+						min_bits, res, &bits_left,
+						&released);
+		if (released) {
+			res->sr_bg_blkno = 0;
+			goto chain_search;
+		}
 		if (!status)
 			goto set_hint;
 		if (status < 0 && status != -ENOSPC) {
@@ -1990,7 +2067,7 @@ static int ocfs2_claim_suballoc_bits(struct ocfs2_alloc_context *ac,
 			goto bail;
 		}
 	}
-
+chain_search:
 	cl = (struct ocfs2_chain_list *) &fe->id2.i_chain;
 
 	victim = ocfs2_find_victim_chain(cl);
@@ -2102,6 +2179,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)
@@ -2540,6 +2623,198 @@ static int ocfs2_block_group_clear_bits(handle_t *handle,
 	return status;
 }
 
+/*
+ * Reclaim the suballocator managed space to main bitmap.
+ * This function first works on the suballocator to perform the
+ * cleanup rec/alloc_inode job, then switches to the main bitmap
+ * to reclaim released space.
+ *
+ * handle: The transaction handle
+ * alloc_inode: The suballoc inode
+ * alloc_bh: The buffer_head of suballoc inode
+ * group_bh: The group descriptor buffer_head of suballocator managed.
+ *           Caller should release the input group_bh.
+ */
+static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
+			struct inode *alloc_inode,
+			struct buffer_head *alloc_bh,
+			struct buffer_head *group_bh)
+{
+	int idx, status = 0;
+	int i, next_free_rec, len = 0;
+	__le16 old_bg_contig_free_bits = 0;
+	u16 start_bit;
+	u32 tmp_used;
+	u64 bg_blkno, start_blk;
+	unsigned int count;
+	struct ocfs2_chain_rec *rec;
+	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_dinode *fe = (struct ocfs2_dinode *) alloc_bh->b_data;
+	struct ocfs2_chain_list *cl = &fe->id2.i_chain;
+	struct ocfs2_group_desc *group = (struct ocfs2_group_desc *) group_bh->b_data;
+
+	idx = le16_to_cpu(group->bg_chain);
+	rec = &(cl->cl_recs[idx]);
+
+	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 suballocator 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 should be counted first */
+		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);
+
+	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
+	if (status < 0)
+		goto free_bm_inode; /* ignore the error in reclaim path */
+
+	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
+				     &start_bit);
+	fe = (struct ocfs2_dinode *) main_bm_bh->b_data;
+	cl = &fe->id2.i_chain;
+	/* reuse group_bh, caller will release the input group_bh */
+	group_bh = NULL;
+
+	/* reclaim clusters to global_bitmap */
+	status = ocfs2_read_group_descriptor(main_bm_inode, fe, bg_blkno,
+					     &group_bh);
+	if (status < 0) {
+		mlog_errno(status);
+		goto free_bm_bh;
+	}
+	group = (struct ocfs2_group_desc *) group_bh->b_data;
+
+	if ((count + start_bit) > le16_to_cpu(group->bg_bits)) {
+		ocfs2_error(alloc_inode->i_sb,
+			"reclaim length (%d) beyands block group length (%d)",
+			count + start_bit, le16_to_cpu(group->bg_bits));
+		goto free_group_bh;
+	}
+
+	old_bg_contig_free_bits = group->bg_contig_free_bits;
+	status = ocfs2_block_group_clear_bits(handle, main_bm_inode,
+					      group, group_bh,
+					      start_bit, count, 0,
+					      _ocfs2_clear_bit);
+	if (status < 0) {
+		mlog_errno(status);
+		goto free_group_bh;
+	}
+
+	status = ocfs2_journal_access_di(handle, INODE_CACHE(main_bm_inode),
+					 main_bm_bh, OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		ocfs2_block_group_set_bits(handle, main_bm_inode, group, group_bh,
+				start_bit, count,
+				le16_to_cpu(old_bg_contig_free_bits), 1);
+		goto free_group_bh;
+	}
+
+	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, main_bm_bh);
+
+free_group_bh:
+	brelse(group_bh);
+
+free_bm_bh:
+	ocfs2_inode_unlock(main_bm_inode, 1);
+	brelse(main_bm_bh);
+
+free_bm_inode:
+	inode_unlock(main_bm_inode);
+	iput(main_bm_inode);
+
+bail:
+	return status;
+}
+
 /*
  * expects the suballoc inode to already be locked.
  */
@@ -2552,12 +2827,13 @@ static int _ocfs2_free_suballoc_bits(handle_t *handle,
 				     void (*undo_fn)(unsigned int bit,
 						     unsigned long *bitmap))
 {
-	int status = 0;
+	int idx, status = 0;
 	u32 tmp_used;
 	struct ocfs2_dinode *fe = (struct ocfs2_dinode *) alloc_bh->b_data;
 	struct ocfs2_chain_list *cl = &fe->id2.i_chain;
 	struct buffer_head *group_bh = NULL;
 	struct ocfs2_group_desc *group;
+	struct ocfs2_chain_rec *rec;
 	__le16 old_bg_contig_free_bits = 0;
 
 	/* The alloc_bh comes from ocfs2_free_dinode() or
@@ -2603,12 +2879,26 @@ 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);
 
+	/*
+	 * Reclaim suballocator free space.
+	 * Bypass: global_bitmap, non empty rec, first rec 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;
+	}
+
+	_ocfs2_reclaim_suballoc_to_main(handle, alloc_inode, alloc_bh, group_bh);
+
 bail:
 	brelse(group_bh);
 	return status;
-- 
2.43.0

[PATCH RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Heming Zhao]

After ocfs2 gained the ability to reclaim suballocator free block
group (BGs), a suballocator block group may be released. This change
causes the xfstest case generic/426 to fail.

generic/426 expects return value -ENOENT or -ESTALE, but the current
code triggers -EROFS.

Call stack before ocfs2 gained the ability to reclaim bg:

ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
 ocfs2_get_dentry
  + ocfs2_test_inode_bit
  |  ocfs2_test_suballoc_bit
  |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
  |   |                             //the bg block was always found.
  |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
  |
  + if (!set) //because the above *res is 0
     status = -ESTALE //the generic/426 expected return value

Current call stack that triggers -EROFS:

ocfs2_get_dentry
 ocfs2_test_inode_bit
  ocfs2_test_suballoc_bit
   ocfs2_read_group_descriptor
    + if reading a released bg, validation fails and triggers -EROFS

How to fix:
Since the read BG is already released, we must avoid triggering -EROFS.
With this commit, we use ocfs2_read_hint_group_descriptor() to detect
the released BG block. This approach quietly handles this type of error
and returns -EINVAL, which triggers the caller's existing conversion
path to -ESTALE.

Signed-off-by: Heming Zhao <heming.zhao@suse.com>
Reviewed-by: Su Yue <glass.su@suse.com>
---
 fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index de2f09217142..a126d83ddb1c 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -3152,7 +3152,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, released;
 
 	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
 				      (unsigned int)bit);
@@ -3168,11 +3168,15 @@ 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,
-					     &group_bh);
-	if (status < 0) {
+	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
+					     &group_bh, &released);
+	if (released) {
+		quiet = 1;
+		status = -EINVAL;
+		goto bail;
+	} else if (status < 0) {
 		mlog(ML_ERROR, "read group %llu failed %d\n",
-		     (unsigned long long)bg_blkno, status);
+				(unsigned long long)bg_blkno, status);
 		goto bail;
 	}
 
@@ -3182,7 +3186,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;
 }
@@ -3202,7 +3206,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;
@@ -3244,8 +3248,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);
@@ -3253,7 +3261,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;
 }
-- 
2.43.0

Re: [PATCH RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Joseph Qi]

On 2025/12/2 14:39, Heming Zhao wrote:
> After ocfs2 gained the ability to reclaim suballocator free block
> group (BGs), a suballocator block group may be released. This change
> causes the xfstest case generic/426 to fail.
> 
> generic/426 expects return value -ENOENT or -ESTALE, but the current
> code triggers -EROFS.
> 
> Call stack before ocfs2 gained the ability to reclaim bg:
> 
> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
>  ocfs2_get_dentry
>   + ocfs2_test_inode_bit
>   |  ocfs2_test_suballoc_bit
>   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
>   |   |                             //the bg block was always found.
>   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
>   |
>   + if (!set) //because the above *res is 0
>      status = -ESTALE //the generic/426 expected return value
> 
> Current call stack that triggers -EROFS:
> 
> ocfs2_get_dentry
>  ocfs2_test_inode_bit
>   ocfs2_test_suballoc_bit
>    ocfs2_read_group_descriptor
>     + if reading a released bg, validation fails and triggers -EROFS
> 
> How to fix:
> Since the read BG is already released, we must avoid triggering -EROFS.
> With this commit, we use ocfs2_read_hint_group_descriptor() to detect
> the released BG block. This approach quietly handles this type of error
> and returns -EINVAL, which triggers the caller's existing conversion
> path to -ESTALE.
> 
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> Reviewed-by: Su Yue <glass.su@suse.com>
> ---
>  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index de2f09217142..a126d83ddb1c 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -3152,7 +3152,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, released;
>  
>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
>  				      (unsigned int)bit);
> @@ -3168,11 +3168,15 @@ 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,
> -					     &group_bh);
> -	if (status < 0) {
> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
> +					     &group_bh, &released);
> +	if (released) {
> +		quiet = 1;
> +		status = -EINVAL;
> +		goto bail;
> +	} else if (status < 0) {
>  		mlog(ML_ERROR, "read group %llu failed %d\n",
> -		     (unsigned long long)bg_blkno, status);
> +				(unsigned long long)bg_blkno, status);
>  		goto bail;
>  	}
>  
> @@ -3182,7 +3186,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;
>  }
> @@ -3202,7 +3206,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;
> @@ -3244,8 +3248,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)

This seems not right, since there is other case which will also return -EINVAL.
So how about return -ESTALE in this case?

Thanks,
Joseph

> +			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);
> @@ -3253,7 +3261,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 RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Heming Zhao]

On Wed, Dec 10, 2025 at 09:55:03AM +0800, Joseph Qi wrote:
> 
> 
> On 2025/12/2 14:39, Heming Zhao wrote:
> > After ocfs2 gained the ability to reclaim suballocator free block
> > group (BGs), a suballocator block group may be released. This change
> > causes the xfstest case generic/426 to fail.
> > 
> > generic/426 expects return value -ENOENT or -ESTALE, but the current
> > code triggers -EROFS.
> > 
> > Call stack before ocfs2 gained the ability to reclaim bg:
> > 
> > ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
> >  ocfs2_get_dentry
> >   + ocfs2_test_inode_bit
> >   |  ocfs2_test_suballoc_bit
> >   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
> >   |   |                             //the bg block was always found.
> >   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
> >   |
> >   + if (!set) //because the above *res is 0
> >      status = -ESTALE //the generic/426 expected return value
> > 
> > Current call stack that triggers -EROFS:
> > 
> > ocfs2_get_dentry
> >  ocfs2_test_inode_bit
> >   ocfs2_test_suballoc_bit
> >    ocfs2_read_group_descriptor
> >     + if reading a released bg, validation fails and triggers -EROFS
> > 
> > How to fix:
> > Since the read BG is already released, we must avoid triggering -EROFS.
> > With this commit, we use ocfs2_read_hint_group_descriptor() to detect
> > the released BG block. This approach quietly handles this type of error
> > and returns -EINVAL, which triggers the caller's existing conversion
> > path to -ESTALE.
> > 
> > Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> > Reviewed-by: Su Yue <glass.su@suse.com>
> > ---
> >  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> > index de2f09217142..a126d83ddb1c 100644
> > --- a/fs/ocfs2/suballoc.c
> > +++ b/fs/ocfs2/suballoc.c
> > @@ -3152,7 +3152,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, released;
> >  
> >  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
> >  				      (unsigned int)bit);
> > @@ -3168,11 +3168,15 @@ 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,
> > -					     &group_bh);
> > -	if (status < 0) {
> > +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
> > +					     &group_bh, &released);
> > +	if (released) {
> > +		quiet = 1;
> > +		status = -EINVAL;
> > +		goto bail;
> > +	} else if (status < 0) {
> >  		mlog(ML_ERROR, "read group %llu failed %d\n",
> > -		     (unsigned long long)bg_blkno, status);
> > +				(unsigned long long)bg_blkno, status);
> >  		goto bail;
> >  	}
> >  
> > @@ -3182,7 +3186,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;
> >  }
> > @@ -3202,7 +3206,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;
> > @@ -3244,8 +3248,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)
> 
> This seems not right, since there is other case which will also return -EINVAL.
> So how about return -ESTALE in this case?
> 
> Thanks,
> Joseph

I agree with your idea that we can get a more specific errno, but we might
introduce some slightly unnecessary work here.

The ocfs2_test_inode_bit() and ocfs2_test_suballoc_bit() only serve for NFS
export paths:

```
ocfs2_fh_to_[dentry|parent]
 ocfs2_get_dentry //converts -EINVAL to -ESTALE
  ocfs2_test_inode_bit //<== here, current returns -EINVAL
   ocfs2_test_suballoc_bit //test the released gd

ocfs2_get_parent //converts -EINVAL to -ESTALE
 ocfs2_test_inode_bit //<== here, current returns -EINVAL
  ocfs2_test_suballoc_bit
```

the current code design treats -EINVAL as a speical case, converting it to -ESTALE.

If we change the ocfs2_test_inode_bit() return value from -EINVAL to -ESTALE.
This will add another special errno in the error handling path.

The code changes are show below:

```
diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
index b95724b767e1..8992989b85a5 100644
--- a/fs/ocfs2/export.c
+++ b/fs/ocfs2/export.c
@@ -74,7 +74,7 @@ static struct dentry *ocfs2_get_dentry(struct super_block *sb,
              * nice
              */
             status = -ESTALE;
-        } else
+        } else if (status != -ESTALE)
             mlog(ML_ERROR, "test inode bit failed %d\n", status);
         goto unlock_nfs_sync;
     }
@@ -162,7 +162,7 @@ static struct dentry *ocfs2_get_parent(struct dentry *child)
     if (status < 0) {
         if (status == -EINVAL) {
             status = -ESTALE;
-        } else
+        } else if (status != -ESTALE)
             mlog(ML_ERROR, "test inode bit failed %d\n", status);
         parent = ERR_PTR(status);
         goto bail_unlock;

diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index c274b649b022..ddcfa6e001e8 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -3172,7 +3172,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
                          &group_bh, &released);
     if (released) {
         quiet = 1;
-        status = -EINVAL;
+        status = -ESTALE;
         goto bail;
     } else if (status < 0) {
         mlog(ML_ERROR, "read group %llu failed %d\n",
@@ -3249,7 +3249,7 @@ 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) {
-        if (status == -EINVAL)
+        if (status == -ESTALE)
             quiet = 1;
         else
             mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
```

However, I am ok with your approach. If you think it is better to return -ESTALE,
I will fix it in the next version.

Thanks,
Heming
> 
> > +			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);
> > @@ -3253,7 +3261,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 RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Joseph Qi]

On 2025/12/10 12:14, Heming Zhao wrote:
> On Wed, Dec 10, 2025 at 09:55:03AM +0800, Joseph Qi wrote:
>>
>>
>> On 2025/12/2 14:39, Heming Zhao wrote:
>>> After ocfs2 gained the ability to reclaim suballocator free block
>>> group (BGs), a suballocator block group may be released. This change
>>> causes the xfstest case generic/426 to fail.
>>>
>>> generic/426 expects return value -ENOENT or -ESTALE, but the current
>>> code triggers -EROFS.
>>>
>>> Call stack before ocfs2 gained the ability to reclaim bg:
>>>
>>> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
>>>  ocfs2_get_dentry
>>>   + ocfs2_test_inode_bit
>>>   |  ocfs2_test_suballoc_bit
>>>   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
>>>   |   |                             //the bg block was always found.
>>>   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
>>>   |
>>>   + if (!set) //because the above *res is 0
>>>      status = -ESTALE //the generic/426 expected return value
>>>
>>> Current call stack that triggers -EROFS:
>>>
>>> ocfs2_get_dentry
>>>  ocfs2_test_inode_bit
>>>   ocfs2_test_suballoc_bit
>>>    ocfs2_read_group_descriptor
>>>     + if reading a released bg, validation fails and triggers -EROFS
>>>
>>> How to fix:
>>> Since the read BG is already released, we must avoid triggering -EROFS.
>>> With this commit, we use ocfs2_read_hint_group_descriptor() to detect
>>> the released BG block. This approach quietly handles this type of error
>>> and returns -EINVAL, which triggers the caller's existing conversion
>>> path to -ESTALE.
>>>
>>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>>> Reviewed-by: Su Yue <glass.su@suse.com>
>>> ---
>>>  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
>>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
>>> index de2f09217142..a126d83ddb1c 100644
>>> --- a/fs/ocfs2/suballoc.c
>>> +++ b/fs/ocfs2/suballoc.c
>>> @@ -3152,7 +3152,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, released;
>>>  
>>>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
>>>  				      (unsigned int)bit);
>>> @@ -3168,11 +3168,15 @@ 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,
>>> -					     &group_bh);
>>> -	if (status < 0) {
>>> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
>>> +					     &group_bh, &released);
>>> +	if (released) {
>>> +		quiet = 1;
>>> +		status = -EINVAL;
>>> +		goto bail;
>>> +	} else if (status < 0) {
>>>  		mlog(ML_ERROR, "read group %llu failed %d\n",
>>> -		     (unsigned long long)bg_blkno, status);
>>> +				(unsigned long long)bg_blkno, status);
>>>  		goto bail;
>>>  	}
>>>  
>>> @@ -3182,7 +3186,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;
>>>  }
>>> @@ -3202,7 +3206,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;
>>> @@ -3244,8 +3248,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)
>>
>> This seems not right, since there is other case which will also return -EINVAL.
>> So how about return -ESTALE in this case?
>>
>> Thanks,
>> Joseph
> 
> I agree with your idea that we can get a more specific errno, but we might
> introduce some slightly unnecessary work here.
> 
> The ocfs2_test_inode_bit() and ocfs2_test_suballoc_bit() only serve for NFS
> export paths:
> 
> ```
> ocfs2_fh_to_[dentry|parent]
>  ocfs2_get_dentry //converts -EINVAL to -ESTALE
>   ocfs2_test_inode_bit //<== here, current returns -EINVAL
>    ocfs2_test_suballoc_bit //test the released gd
> 
> ocfs2_get_parent //converts -EINVAL to -ESTALE
>  ocfs2_test_inode_bit //<== here, current returns -EINVAL
>   ocfs2_test_suballoc_bit
> ```
> 
> the current code design treats -EINVAL as a speical case, converting it to -ESTALE.
> 
> If we change the ocfs2_test_inode_bit() return value from -EINVAL to -ESTALE.
> This will add another special errno in the error handling path.
> 
> The code changes are show below:
> 
> ```
> diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
> index b95724b767e1..8992989b85a5 100644
> --- a/fs/ocfs2/export.c
> +++ b/fs/ocfs2/export.c
> @@ -74,7 +74,7 @@ static struct dentry *ocfs2_get_dentry(struct super_block *sb,
>               * nice
>               */
>              status = -ESTALE;
> -        } else
> +        } else if (status != -ESTALE)
>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
>          goto unlock_nfs_sync;
>      }
> @@ -162,7 +162,7 @@ static struct dentry *ocfs2_get_parent(struct dentry *child)
>      if (status < 0) {
>          if (status == -EINVAL) {
>              status = -ESTALE;
> -        } else
> +        } else if (status != -ESTALE)
>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
>          parent = ERR_PTR(status);
>          goto bail_unlock;
> 
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index c274b649b022..ddcfa6e001e8 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -3172,7 +3172,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>                           &group_bh, &released);
>      if (released) {
>          quiet = 1;
> -        status = -EINVAL;
> +        status = -ESTALE;
>          goto bail;
>      } else if (status < 0) {
>          mlog(ML_ERROR, "read group %llu failed %d\n",
> @@ -3249,7 +3249,7 @@ 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) {
> -        if (status == -EINVAL)
> +        if (status == -ESTALE)
>              quiet = 1;
>          else
>              mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
> ```
> 
> However, I am ok with your approach. If you think it is better to return -ESTALE,
> I will fix it in the next version.
> 

Okay, it seems -EINVAL is also fine. But why you silent the error log
for -EINVAL? This will also silent another case, which is different from
before.
BTW, why can't we just return 0 in case of release?

Joseph

Re: [PATCH RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Heming Zhao]

On Wed, Dec 10, 2025 at 05:00:09PM +0800, Joseph Qi wrote:
> 
> 
> On 2025/12/10 12:14, Heming Zhao wrote:
> > On Wed, Dec 10, 2025 at 09:55:03AM +0800, Joseph Qi wrote:
> >>
> >>
> >> On 2025/12/2 14:39, Heming Zhao wrote:
> >>> After ocfs2 gained the ability to reclaim suballocator free block
> >>> group (BGs), a suballocator block group may be released. This change
> >>> causes the xfstest case generic/426 to fail.
> >>>
> >>> generic/426 expects return value -ENOENT or -ESTALE, but the current
> >>> code triggers -EROFS.
> >>>
> >>> Call stack before ocfs2 gained the ability to reclaim bg:
> >>>
> >>> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
> >>>  ocfs2_get_dentry
> >>>   + ocfs2_test_inode_bit
> >>>   |  ocfs2_test_suballoc_bit
> >>>   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
> >>>   |   |                             //the bg block was always found.
> >>>   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
> >>>   |
> >>>   + if (!set) //because the above *res is 0
> >>>      status = -ESTALE //the generic/426 expected return value
> >>>
> >>> Current call stack that triggers -EROFS:
> >>>
> >>> ocfs2_get_dentry
> >>>  ocfs2_test_inode_bit
> >>>   ocfs2_test_suballoc_bit
> >>>    ocfs2_read_group_descriptor
> >>>     + if reading a released bg, validation fails and triggers -EROFS
> >>>
> >>> How to fix:
> >>> Since the read BG is already released, we must avoid triggering -EROFS.
> >>> With this commit, we use ocfs2_read_hint_group_descriptor() to detect
> >>> the released BG block. This approach quietly handles this type of error
> >>> and returns -EINVAL, which triggers the caller's existing conversion
> >>> path to -ESTALE.
> >>>
> >>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> >>> Reviewed-by: Su Yue <glass.su@suse.com>
> >>> ---
> >>>  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
> >>>  1 file changed, 18 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> >>> index de2f09217142..a126d83ddb1c 100644
> >>> --- a/fs/ocfs2/suballoc.c
> >>> +++ b/fs/ocfs2/suballoc.c
> >>> @@ -3152,7 +3152,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, released;
> >>>  
> >>>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
> >>>  				      (unsigned int)bit);
> >>> @@ -3168,11 +3168,15 @@ 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,
> >>> -					     &group_bh);
> >>> -	if (status < 0) {
> >>> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
> >>> +					     &group_bh, &released);
> >>> +	if (released) {
> >>> +		quiet = 1;
> >>> +		status = -EINVAL;
> >>> +		goto bail;
> >>> +	} else if (status < 0) {
> >>>  		mlog(ML_ERROR, "read group %llu failed %d\n",
> >>> -		     (unsigned long long)bg_blkno, status);
> >>> +				(unsigned long long)bg_blkno, status);
> >>>  		goto bail;
> >>>  	}
> >>>  
> >>> @@ -3182,7 +3186,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;
> >>>  }
> >>> @@ -3202,7 +3206,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;
> >>> @@ -3244,8 +3248,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)
> >>
> >> This seems not right, since there is other case which will also return -EINVAL.
> >> So how about return -ESTALE in this case?
> >>
> >> Thanks,
> >> Joseph
> > 
> > I agree with your idea that we can get a more specific errno, but we might
> > introduce some slightly unnecessary work here.
> > 
> > The ocfs2_test_inode_bit() and ocfs2_test_suballoc_bit() only serve for NFS
> > export paths:
> > 
> > ```
> > ocfs2_fh_to_[dentry|parent]
> >  ocfs2_get_dentry //converts -EINVAL to -ESTALE
> >   ocfs2_test_inode_bit //<== here, current returns -EINVAL
> >    ocfs2_test_suballoc_bit //test the released gd
> > 
> > ocfs2_get_parent //converts -EINVAL to -ESTALE
> >  ocfs2_test_inode_bit //<== here, current returns -EINVAL
> >   ocfs2_test_suballoc_bit
> > ```
> > 
> > the current code design treats -EINVAL as a speical case, converting it to -ESTALE.
> > 
> > If we change the ocfs2_test_inode_bit() return value from -EINVAL to -ESTALE.
> > This will add another special errno in the error handling path.
> > 
> > The code changes are show below:
> > 
> > ```
> > diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
> > index b95724b767e1..8992989b85a5 100644
> > --- a/fs/ocfs2/export.c
> > +++ b/fs/ocfs2/export.c
> > @@ -74,7 +74,7 @@ static struct dentry *ocfs2_get_dentry(struct super_block *sb,
> >               * nice
> >               */
> >              status = -ESTALE;
> > -        } else
> > +        } else if (status != -ESTALE)
> >              mlog(ML_ERROR, "test inode bit failed %d\n", status);
> >          goto unlock_nfs_sync;
> >      }
> > @@ -162,7 +162,7 @@ static struct dentry *ocfs2_get_parent(struct dentry *child)
> >      if (status < 0) {
> >          if (status == -EINVAL) {
> >              status = -ESTALE;
> > -        } else
> > +        } else if (status != -ESTALE)
> >              mlog(ML_ERROR, "test inode bit failed %d\n", status);
> >          parent = ERR_PTR(status);
> >          goto bail_unlock;
> > 
> > diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> > index c274b649b022..ddcfa6e001e8 100644
> > --- a/fs/ocfs2/suballoc.c
> > +++ b/fs/ocfs2/suballoc.c
> > @@ -3172,7 +3172,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
> >                           &group_bh, &released);
> >      if (released) {
> >          quiet = 1;
> > -        status = -EINVAL;
> > +        status = -ESTALE;
> >          goto bail;
> >      } else if (status < 0) {
> >          mlog(ML_ERROR, "read group %llu failed %d\n",
> > @@ -3249,7 +3249,7 @@ 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) {
> > -        if (status == -EINVAL)
> > +        if (status == -ESTALE)
> >              quiet = 1;
> >          else
> >              mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
> > ```
> > 
> > However, I am ok with your approach. If you think it is better to return -ESTALE,
> > I will fix it in the next version.
> > 
> 
> Okay, it seems -EINVAL is also fine. But why you silent the error log
> for -EINVAL? This will also silent another case, which is different from
> before.

You are right, the patch code silents all -EINVAL cases, which is
incorrect logic.

> BTW, why can't we just return 0 in case of release?
> 
> Joseph
> 

There are two reasons why we should return an errno:
1> This is still an abnormal case where user is searching for an inode that
   is already deleted.

2> I am not familiar with NFS. However, the meaning from the xfstests
   generic/426 source code, which calls "open_by_handle -d", and the comment
   at the beginning of the src/open_by_handle.c [1]:

   ```
   6. Get file handles for existing test set, unlink all test files,
      remove test_dir, drop caches, try to open all files by handle
      and expect ESTALE:
   
      open_by_handle -dp <test_dir> [N]
   ```

   The expected return value is '-ESTALE', not '0' (success).

Returning to my patch, I think your previous idea (to return specific
-ESTALE) makes sense. Although we are adding another special error case,
this change keeps the existing error path working as before.

May I sent the v5 patch which includes the change for -ESTALE (using code
similar to my previous email in this thread)?

[1]: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/src/open_by_handle.c

- Heming

Re: [PATCH RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Joseph Qi]

On 2025/12/10 22:07, Heming Zhao wrote:
> On Wed, Dec 10, 2025 at 05:00:09PM +0800, Joseph Qi wrote:
>>
>>
>> On 2025/12/10 12:14, Heming Zhao wrote:
>>> On Wed, Dec 10, 2025 at 09:55:03AM +0800, Joseph Qi wrote:
>>>>
>>>>
>>>> On 2025/12/2 14:39, Heming Zhao wrote:
>>>>> After ocfs2 gained the ability to reclaim suballocator free block
>>>>> group (BGs), a suballocator block group may be released. This change
>>>>> causes the xfstest case generic/426 to fail.
>>>>>
>>>>> generic/426 expects return value -ENOENT or -ESTALE, but the current
>>>>> code triggers -EROFS.
>>>>>
>>>>> Call stack before ocfs2 gained the ability to reclaim bg:
>>>>>
>>>>> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
>>>>>  ocfs2_get_dentry
>>>>>   + ocfs2_test_inode_bit
>>>>>   |  ocfs2_test_suballoc_bit
>>>>>   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
>>>>>   |   |                             //the bg block was always found.
>>>>>   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
>>>>>   |
>>>>>   + if (!set) //because the above *res is 0
>>>>>      status = -ESTALE //the generic/426 expected return value
>>>>>
>>>>> Current call stack that triggers -EROFS:
>>>>>
>>>>> ocfs2_get_dentry
>>>>>  ocfs2_test_inode_bit
>>>>>   ocfs2_test_suballoc_bit
>>>>>    ocfs2_read_group_descriptor
>>>>>     + if reading a released bg, validation fails and triggers -EROFS
>>>>>
>>>>> How to fix:
>>>>> Since the read BG is already released, we must avoid triggering -EROFS.
>>>>> With this commit, we use ocfs2_read_hint_group_descriptor() to detect
>>>>> the released BG block. This approach quietly handles this type of error
>>>>> and returns -EINVAL, which triggers the caller's existing conversion
>>>>> path to -ESTALE.
>>>>>
>>>>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>>>>> Reviewed-by: Su Yue <glass.su@suse.com>
>>>>> ---
>>>>>  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
>>>>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
>>>>> index de2f09217142..a126d83ddb1c 100644
>>>>> --- a/fs/ocfs2/suballoc.c
>>>>> +++ b/fs/ocfs2/suballoc.c
>>>>> @@ -3152,7 +3152,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, released;
>>>>>  
>>>>>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
>>>>>  				      (unsigned int)bit);
>>>>> @@ -3168,11 +3168,15 @@ 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,
>>>>> -					     &group_bh);
>>>>> -	if (status < 0) {
>>>>> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
>>>>> +					     &group_bh, &released);
>>>>> +	if (released) {
>>>>> +		quiet = 1;
>>>>> +		status = -EINVAL;
>>>>> +		goto bail;
>>>>> +	} else if (status < 0) {
>>>>>  		mlog(ML_ERROR, "read group %llu failed %d\n",
>>>>> -		     (unsigned long long)bg_blkno, status);
>>>>> +				(unsigned long long)bg_blkno, status);
>>>>>  		goto bail;
>>>>>  	}
>>>>>  
>>>>> @@ -3182,7 +3186,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;
>>>>>  }
>>>>> @@ -3202,7 +3206,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;
>>>>> @@ -3244,8 +3248,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)
>>>>
>>>> This seems not right, since there is other case which will also return -EINVAL.
>>>> So how about return -ESTALE in this case?
>>>>
>>>> Thanks,
>>>> Joseph
>>>
>>> I agree with your idea that we can get a more specific errno, but we might
>>> introduce some slightly unnecessary work here.
>>>
>>> The ocfs2_test_inode_bit() and ocfs2_test_suballoc_bit() only serve for NFS
>>> export paths:
>>>
>>> ```
>>> ocfs2_fh_to_[dentry|parent]
>>>  ocfs2_get_dentry //converts -EINVAL to -ESTALE
>>>   ocfs2_test_inode_bit //<== here, current returns -EINVAL
>>>    ocfs2_test_suballoc_bit //test the released gd
>>>
>>> ocfs2_get_parent //converts -EINVAL to -ESTALE
>>>  ocfs2_test_inode_bit //<== here, current returns -EINVAL
>>>   ocfs2_test_suballoc_bit
>>> ```
>>>
>>> the current code design treats -EINVAL as a speical case, converting it to -ESTALE.
>>>
>>> If we change the ocfs2_test_inode_bit() return value from -EINVAL to -ESTALE.
>>> This will add another special errno in the error handling path.
>>>
>>> The code changes are show below:
>>>
>>> ```
>>> diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
>>> index b95724b767e1..8992989b85a5 100644
>>> --- a/fs/ocfs2/export.c
>>> +++ b/fs/ocfs2/export.c
>>> @@ -74,7 +74,7 @@ static struct dentry *ocfs2_get_dentry(struct super_block *sb,
>>>               * nice
>>>               */
>>>              status = -ESTALE;
>>> -        } else
>>> +        } else if (status != -ESTALE)
>>>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
>>>          goto unlock_nfs_sync;
>>>      }
>>> @@ -162,7 +162,7 @@ static struct dentry *ocfs2_get_parent(struct dentry *child)
>>>      if (status < 0) {
>>>          if (status == -EINVAL) {
>>>              status = -ESTALE;
>>> -        } else
>>> +        } else if (status != -ESTALE)
>>>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
>>>          parent = ERR_PTR(status);
>>>          goto bail_unlock;
>>>
>>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
>>> index c274b649b022..ddcfa6e001e8 100644
>>> --- a/fs/ocfs2/suballoc.c
>>> +++ b/fs/ocfs2/suballoc.c
>>> @@ -3172,7 +3172,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
>>>                           &group_bh, &released);
>>>      if (released) {
>>>          quiet = 1;
>>> -        status = -EINVAL;
>>> +        status = -ESTALE;
>>>          goto bail;
>>>      } else if (status < 0) {
>>>          mlog(ML_ERROR, "read group %llu failed %d\n",
>>> @@ -3249,7 +3249,7 @@ 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) {
>>> -        if (status == -EINVAL)
>>> +        if (status == -ESTALE)
>>>              quiet = 1;
>>>          else
>>>              mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
>>> ```
>>>
>>> However, I am ok with your approach. If you think it is better to return -ESTALE,
>>> I will fix it in the next version.
>>>
>>
>> Okay, it seems -EINVAL is also fine. But why you silent the error log
>> for -EINVAL? This will also silent another case, which is different from
>> before.
> 
> You are right, the patch code silents all -EINVAL cases, which is
> incorrect logic.
> 
>> BTW, why can't we just return 0 in case of release?
>>
>> Joseph
>>
> 
> There are two reasons why we should return an errno:
> 1> This is still an abnormal case where user is searching for an inode that
>    is already deleted.
> 
> 2> I am not familiar with NFS. However, the meaning from the xfstests
>    generic/426 source code, which calls "open_by_handle -d", and the comment
>    at the beginning of the src/open_by_handle.c [1]:
> 
>    ```
>    6. Get file handles for existing test set, unlink all test files,
>       remove test_dir, drop caches, try to open all files by handle
>       and expect ESTALE:
>    
>       open_by_handle -dp <test_dir> [N]
>    ```
> 
>    The expected return value is '-ESTALE', not '0' (success).
> 
> Returning to my patch, I think your previous idea (to return specific
> -ESTALE) makes sense. Although we are adding another special error case,
> this change keeps the existing error path working as before.
> 
> May I sent the v5 patch which includes the change for -ESTALE (using code
> similar to my previous email in this thread)?
> 
Or reuse the existing -EINVAL logic but without silent error log?
It seems log this case also make sense.

Joseph

Re: [PATCH RESEND v4 2/2] ocfs2: detect released suballocator BG for fh_to_[dentry|parent]

[Heming Zhao]

On Thu, Dec 11, 2025 at 09:12:34AM +0800, Joseph Qi wrote:
> 
> 
> On 2025/12/10 22:07, Heming Zhao wrote:
> > On Wed, Dec 10, 2025 at 05:00:09PM +0800, Joseph Qi wrote:
> >>
> >>
> >> On 2025/12/10 12:14, Heming Zhao wrote:
> >>> On Wed, Dec 10, 2025 at 09:55:03AM +0800, Joseph Qi wrote:
> >>>>
> >>>>
> >>>> On 2025/12/2 14:39, Heming Zhao wrote:
> >>>>> After ocfs2 gained the ability to reclaim suballocator free block
> >>>>> group (BGs), a suballocator block group may be released. This change
> >>>>> causes the xfstest case generic/426 to fail.
> >>>>>
> >>>>> generic/426 expects return value -ENOENT or -ESTALE, but the current
> >>>>> code triggers -EROFS.
> >>>>>
> >>>>> Call stack before ocfs2 gained the ability to reclaim bg:
> >>>>>
> >>>>> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent
> >>>>>  ocfs2_get_dentry
> >>>>>   + ocfs2_test_inode_bit
> >>>>>   |  ocfs2_test_suballoc_bit
> >>>>>   |   + ocfs2_read_group_descriptor //Since ocfs2 never releases the bg,
> >>>>>   |   |                             //the bg block was always found.
> >>>>>   |   + *res = ocfs2_test_bit //unlink was called, and the bit is zero
> >>>>>   |
> >>>>>   + if (!set) //because the above *res is 0
> >>>>>      status = -ESTALE //the generic/426 expected return value
> >>>>>
> >>>>> Current call stack that triggers -EROFS:
> >>>>>
> >>>>> ocfs2_get_dentry
> >>>>>  ocfs2_test_inode_bit
> >>>>>   ocfs2_test_suballoc_bit
> >>>>>    ocfs2_read_group_descriptor
> >>>>>     + if reading a released bg, validation fails and triggers -EROFS
> >>>>>
> >>>>> How to fix:
> >>>>> Since the read BG is already released, we must avoid triggering -EROFS.
> >>>>> With this commit, we use ocfs2_read_hint_group_descriptor() to detect
> >>>>> the released BG block. This approach quietly handles this type of error
> >>>>> and returns -EINVAL, which triggers the caller's existing conversion
> >>>>> path to -ESTALE.
> >>>>>
> >>>>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> >>>>> Reviewed-by: Su Yue <glass.su@suse.com>
> >>>>> ---
> >>>>>  fs/ocfs2/suballoc.c | 28 ++++++++++++++++++----------
> >>>>>  1 file changed, 18 insertions(+), 10 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> >>>>> index de2f09217142..a126d83ddb1c 100644
> >>>>> --- a/fs/ocfs2/suballoc.c
> >>>>> +++ b/fs/ocfs2/suballoc.c
> >>>>> @@ -3152,7 +3152,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, released;
> >>>>>  
> >>>>>  	trace_ocfs2_test_suballoc_bit((unsigned long long)blkno,
> >>>>>  				      (unsigned int)bit);
> >>>>> @@ -3168,11 +3168,15 @@ 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,
> >>>>> -					     &group_bh);
> >>>>> -	if (status < 0) {
> >>>>> +	status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno,
> >>>>> +					     &group_bh, &released);
> >>>>> +	if (released) {
> >>>>> +		quiet = 1;
> >>>>> +		status = -EINVAL;
> >>>>> +		goto bail;
> >>>>> +	} else if (status < 0) {
> >>>>>  		mlog(ML_ERROR, "read group %llu failed %d\n",
> >>>>> -		     (unsigned long long)bg_blkno, status);
> >>>>> +				(unsigned long long)bg_blkno, status);
> >>>>>  		goto bail;
> >>>>>  	}
> >>>>>  
> >>>>> @@ -3182,7 +3186,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;
> >>>>>  }
> >>>>> @@ -3202,7 +3206,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;
> >>>>> @@ -3244,8 +3248,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)
> >>>>
> >>>> This seems not right, since there is other case which will also return -EINVAL.
> >>>> So how about return -ESTALE in this case?
> >>>>
> >>>> Thanks,
> >>>> Joseph
> >>>
> >>> I agree with your idea that we can get a more specific errno, but we might
> >>> introduce some slightly unnecessary work here.
> >>>
> >>> The ocfs2_test_inode_bit() and ocfs2_test_suballoc_bit() only serve for NFS
> >>> export paths:
> >>>
> >>> ```
> >>> ocfs2_fh_to_[dentry|parent]
> >>>  ocfs2_get_dentry //converts -EINVAL to -ESTALE
> >>>   ocfs2_test_inode_bit //<== here, current returns -EINVAL
> >>>    ocfs2_test_suballoc_bit //test the released gd
> >>>
> >>> ocfs2_get_parent //converts -EINVAL to -ESTALE
> >>>  ocfs2_test_inode_bit //<== here, current returns -EINVAL
> >>>   ocfs2_test_suballoc_bit
> >>> ```
> >>>
> >>> the current code design treats -EINVAL as a speical case, converting it to -ESTALE.
> >>>
> >>> If we change the ocfs2_test_inode_bit() return value from -EINVAL to -ESTALE.
> >>> This will add another special errno in the error handling path.
> >>>
> >>> The code changes are show below:
> >>>
> >>> ```
> >>> diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
> >>> index b95724b767e1..8992989b85a5 100644
> >>> --- a/fs/ocfs2/export.c
> >>> +++ b/fs/ocfs2/export.c
> >>> @@ -74,7 +74,7 @@ static struct dentry *ocfs2_get_dentry(struct super_block *sb,
> >>>               * nice
> >>>               */
> >>>              status = -ESTALE;
> >>> -        } else
> >>> +        } else if (status != -ESTALE)
> >>>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
> >>>          goto unlock_nfs_sync;
> >>>      }
> >>> @@ -162,7 +162,7 @@ static struct dentry *ocfs2_get_parent(struct dentry *child)
> >>>      if (status < 0) {
> >>>          if (status == -EINVAL) {
> >>>              status = -ESTALE;
> >>> -        } else
> >>> +        } else if (status != -ESTALE)
> >>>              mlog(ML_ERROR, "test inode bit failed %d\n", status);
> >>>          parent = ERR_PTR(status);
> >>>          goto bail_unlock;
> >>>
> >>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> >>> index c274b649b022..ddcfa6e001e8 100644
> >>> --- a/fs/ocfs2/suballoc.c
> >>> +++ b/fs/ocfs2/suballoc.c
> >>> @@ -3172,7 +3172,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb,
> >>>                           &group_bh, &released);
> >>>      if (released) {
> >>>          quiet = 1;
> >>> -        status = -EINVAL;
> >>> +        status = -ESTALE;
> >>>          goto bail;
> >>>      } else if (status < 0) {
> >>>          mlog(ML_ERROR, "read group %llu failed %d\n",
> >>> @@ -3249,7 +3249,7 @@ 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) {
> >>> -        if (status == -EINVAL)
> >>> +        if (status == -ESTALE)
> >>>              quiet = 1;
> >>>          else
> >>>              mlog(ML_ERROR, "test suballoc bit failed %d\n", status);
> >>> ```
> >>>
> >>> However, I am ok with your approach. If you think it is better to return -ESTALE,
> >>> I will fix it in the next version.
> >>>
> >>
> >> Okay, it seems -EINVAL is also fine. But why you silent the error log
> >> for -EINVAL? This will also silent another case, which is different from
> >> before.
> > 
> > You are right, the patch code silents all -EINVAL cases, which is
> > incorrect logic.
> > 
> >> BTW, why can't we just return 0 in case of release?
> >>
> >> Joseph
> >>
> > 
> > There are two reasons why we should return an errno:
> > 1> This is still an abnormal case where user is searching for an inode that
> >    is already deleted.
> > 
> > 2> I am not familiar with NFS. However, the meaning from the xfstests
> >    generic/426 source code, which calls "open_by_handle -d", and the comment
> >    at the beginning of the src/open_by_handle.c [1]:
> > 
> >    ```
> >    6. Get file handles for existing test set, unlink all test files,
> >       remove test_dir, drop caches, try to open all files by handle
> >       and expect ESTALE:
> >    
> >       open_by_handle -dp <test_dir> [N]
> >    ```
> > 
> >    The expected return value is '-ESTALE', not '0' (success).
> > 
> > Returning to my patch, I think your previous idea (to return specific
> > -ESTALE) makes sense. Although we are adding another special error case,
> > this change keeps the existing error path working as before.
> > 
> > May I sent the v5 patch which includes the change for -ESTALE (using code
> > similar to my previous email in this thread)?
> > 
> Or reuse the existing -EINVAL logic but without silent error log?
> It seems log this case also make sense.
> 
> Joseph

The ext4 doesn't output any warning/error messages during generic/426, and before
this patch set (where ocfs2 never released suballocator space), ocfs2 also never
output any messages. So, I suggest ocfs2 should remain quite.
Adding -ESTALE makes sense, because the new behavior, where ocfs2 can release bg,
introduces a new error handling path.

- Heming

Re: [PATCH RESEND v4 0/2] ocfs2: give ocfs2 the ability to reclaim suballocator free bg

[Joseph Qi]

On 2025/12/2 14:39, Heming Zhao wrote:
> why resend?
> - the v4 summary description in the cover letter is incorrect.
> 
> v4:
> 
> Most of the changes involve revising comments. For the code itself, there are
> function renames and parameter usage adjustments, but no changes to the code
> logic.
> 
> For patch [1/2]:
> - Based on Joseph's v3 review comments:
>   1. Modify the caller to initialize the ocfs2_read_hint_group_descriptor()
>      input parameter '*released'.
>   2. Rename the _reclaim_to_main_bm() to _ocfs2_reclaim_suballoc_to_main()
>   3. Change the text "not empty rec" to "non empty rec"
> 
> - Revise the comments preceding the function _ocfs2_reclaim_suballoc_to_main().
> 
> - For the question: "How to distinguish the release case or a bug?"
>   I write a comment before ocfs2_read_hint_group_descriptor().
> 
> - Revise the commit log to make it clearer.
> 
> For patch [2/2]:
> - Revise the commit log to make it clearer.
> 
> Remove the v3 patch: "ocfs2: adjust spinlock_t ip_lock protection scope"
> - Reason: This patch belongs to a different topic/job and should be
>           handled separately.
> 
> v3:
> 
> For patch [1/3]:
> - Factor out the reclaim code into a new function '_reclaim_to_main_bm'.
> - The function ocfs2_read_hint_group_descriptor doesn't return -EIDRM
>   when the group descriptor is invalid. The new code logic returns 0,
>   and the input parameter '*released' is set to 1.
> 
> For patch [2/3]:
> - Modify the code to follow the new logic of ocfs2_read_hint_group_descriptor
>   as introduced in patch [1/3].
> 
> For patch [3/3]:
> - No new changes.
> 
> v3 patch has passed the xfstests:
> ./check -g quick -T -b -s ocfs2 -e generic/032 -e generic/076 \
> -e generic/081 -e generic/266 -e generic/272 -e generic/281 \
> -e generic/331 -e generic/338 -e generic/347 -e generic/361 \
> -e generic/479 -e generic/480 -e generic/628 -e generic/629 \
> -e generic/648 -e generic/650
> 

Hi, could you please send out the ocfs2-test results as well?

Thanks,
Joseph

> v2:
> 
> Create 2 new patches:
> - ocfs2: detect released suballocator bg for fh_to_[dentry|parent]
> - ocfs2: adjust spinlock_t ip_lock protection scope
> 
> In ocfs2_read_hint_group_descriptor()
> - bypass the validation of GD when the BH is already managed by jbd2.
> 
> In _ocfs2_free_suballoc_bits()
> - Move up the position of the vars 'idx' & 'rec'.
> - Move up the position of the ocfs2_journal_dirty.
> - Use le[16|32]_to_cpu() to access cl/fe/rec vars.
> - Add error handling for calling ocfs2_extend_trans().
> - adjust spin_lock ->ip_lock protection scope.
> - Follow Glass's review comments, add 'comment' & 'else-break' for the
>   'for-loop'.
> 
> v1:
> 
> Only create patch:
> - ocfs2: give ocfs2 the ability to reclaim suballoc free bg
> 
> Heming Zhao (2):
>   ocfs2: give ocfs2 the ability to reclaim suballoc free bg
>   ocfs2: detect released suballocator BG for fh_to_[dentry|parent]
> 
>  fs/ocfs2/suballoc.c | 336 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 317 insertions(+), 19 deletions(-)
> 

Re: [PATCH RESEND v4 0/2] ocfs2: give ocfs2 the ability to reclaim suballocator free bg

[Heming Zhao]

On Fri, Dec 05, 2025 at 05:22:47PM +0800, Joseph Qi wrote:
> 
> 
> On 2025/12/2 14:39, Heming Zhao wrote:
> > why resend?
> > - the v4 summary description in the cover letter is incorrect.
> > 
> > v4:
> > 
> > Most of the changes involve revising comments. For the code itself, there are
> > function renames and parameter usage adjustments, but no changes to the code
> > logic.
> > 
> > For patch [1/2]:
> > - Based on Joseph's v3 review comments:
> >   1. Modify the caller to initialize the ocfs2_read_hint_group_descriptor()
> >      input parameter '*released'.
> >   2. Rename the _reclaim_to_main_bm() to _ocfs2_reclaim_suballoc_to_main()
> >   3. Change the text "not empty rec" to "non empty rec"
> > 
> > - Revise the comments preceding the function _ocfs2_reclaim_suballoc_to_main().
> > 
> > - For the question: "How to distinguish the release case or a bug?"
> >   I write a comment before ocfs2_read_hint_group_descriptor().
> > 
> > - Revise the commit log to make it clearer.
> > 
> > For patch [2/2]:
> > - Revise the commit log to make it clearer.
> > 
> > Remove the v3 patch: "ocfs2: adjust spinlock_t ip_lock protection scope"
> > - Reason: This patch belongs to a different topic/job and should be
> >           handled separately.
> > 
> > v3:
> > 
> > For patch [1/3]:
> > - Factor out the reclaim code into a new function '_reclaim_to_main_bm'.
> > - The function ocfs2_read_hint_group_descriptor doesn't return -EIDRM
> >   when the group descriptor is invalid. The new code logic returns 0,
> >   and the input parameter '*released' is set to 1.
> > 
> > For patch [2/3]:
> > - Modify the code to follow the new logic of ocfs2_read_hint_group_descriptor
> >   as introduced in patch [1/3].
> > 
> > For patch [3/3]:
> > - No new changes.
> > 
> > v3 patch has passed the xfstests:
> > ./check -g quick -T -b -s ocfs2 -e generic/032 -e generic/076 \
> > -e generic/081 -e generic/266 -e generic/272 -e generic/281 \
> > -e generic/331 -e generic/338 -e generic/347 -e generic/361 \
> > -e generic/479 -e generic/480 -e generic/628 -e generic/629 \
> > -e generic/648 -e generic/650
> > 
> 
> Hi, could you please send out the ocfs2-test results as well?
> 
> Thanks,
> Joseph

I have run the xfstest with above style:
./check -g quick -T -b -s ocfs2 -e generic/032 -e generic/076 \
-e generic/081 -e generic/266 -e generic/272 -e generic/281 \
-e generic/331 -e generic/338 -e generic/347 -e generic/361 \
-e generic/479 -e generic/480 -e generic/628 -e generic/629 \
-e generic/648 -e generic/650

with/without the patch set, *Failures* are same:
generic/003 generic/007 generic/228 generic/322 generic/329 generic/376
generic/383 generic/384 generic/385 generic/386 generic/420 generic/424
generic/448 generic/449 generic/510 generic/513 generic/537 generic/552
generic/563 generic/578 generic/594 generic/607 generic/620 generic/630
generic/741 generic/755

For the ocfs2-test, it seems that open-mpi4 does not work correctly in my test
environment (OpenSUSE TumbleWeed). Therefore, I only ran the tests in single-node
mode. The results were the same between the patched code and the unpatched code.

ocfs2-test cases:

$ single_run-WIP.sh -f 1 -k /usr/local/ocfs2-test/tmp/linux-2.6.39.tar.gz -l \
/usr/local/ocfs2-test/log -m /mnt/ocfs2 -d /dev/vde -b 4096 -c 32768 -s pcmk \
-n hacluster -t create_and_open,directaio,fillverifyholes,renamewriterace,\
aiostress,filesizelimits,mmaptruncate,buildkernel,splice,sendfile,reserve_space,\
mmap,inline,xattr,reflink,mkfs,tunefs,backup_super

$ discontig_runner.sh -f 1 -d /dev/vde -b 4096 -c 32768 -s pcmk -n hacluster /mnt/ocfs2

- Heming
> 
> > v2:
> > 
> > Create 2 new patches:
> > - ocfs2: detect released suballocator bg for fh_to_[dentry|parent]
> > - ocfs2: adjust spinlock_t ip_lock protection scope
> > 
> > In ocfs2_read_hint_group_descriptor()
> > - bypass the validation of GD when the BH is already managed by jbd2.
> > 
> > In _ocfs2_free_suballoc_bits()
> > - Move up the position of the vars 'idx' & 'rec'.
> > - Move up the position of the ocfs2_journal_dirty.
> > - Use le[16|32]_to_cpu() to access cl/fe/rec vars.
> > - Add error handling for calling ocfs2_extend_trans().
> > - adjust spin_lock ->ip_lock protection scope.
> > - Follow Glass's review comments, add 'comment' & 'else-break' for the
> >   'for-loop'.
> > 
> > v1:
> > 
> > Only create patch:
> > - ocfs2: give ocfs2 the ability to reclaim suballoc free bg
> > 
> > Heming Zhao (2):
> >   ocfs2: give ocfs2 the ability to reclaim suballoc free bg
> >   ocfs2: detect released suballocator BG for fh_to_[dentry|parent]
> > 
> >  fs/ocfs2/suballoc.c | 336 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 317 insertions(+), 19 deletions(-)
> > 
>