Re: [PATCH v8 3/5] xfs: introduce xfs_ag_shrink_space()

On Mon, Mar 22, 2021 at 08:25:55AM -0400, Brian Foster wrote:

On Mon, Mar 22, 2021 at 08:03:10PM +0800, Gao Xiang wrote:

quoted

Hi Brian,

On Mon, Mar 22, 2021 at 07:30:03AM -0400, Brian Foster wrote:

quoted

On Fri, Mar 05, 2021 at 10:57:01AM +0800, Gao Xiang wrote:

quoted

This patch introduces a helper to shrink unused space in the last AG
by fixing up the freespace btree.

Also make sure that the per-AG reservation works under the new AG
size. If such per-AG reservation or extent allocation fails, roll
the transaction so the new transaction could cancel without any side
effects.

Signed-off-by: Gao Xiang <redacted>
---

Looks mostly good to me. Some nits..

quoted

 fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h |   4 +-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9331f3516afa..1f6f9e70e1cb 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c

...

quoted

@@ -485,6 +490,112 @@ xfs_ag_init_headers(
 	return error;
 }
 
+int
+xfs_ag_shrink_space(
+	struct xfs_mount	*mp,
+	struct xfs_trans	**tpp,
+	xfs_agnumber_t		agno,
+	xfs_extlen_t		delta)
+{
+	struct xfs_alloc_arg	args = {
+		.tp	= *tpp,
+		.mp	= mp,
+		.type	= XFS_ALLOCTYPE_THIS_BNO,
+		.minlen = delta,
+		.maxlen = delta,
+		.oinfo	= XFS_RMAP_OINFO_SKIP_UPDATE,
+		.resv	= XFS_AG_RESV_NONE,
+		.prod	= 1
+	};
+	struct xfs_buf		*agibp, *agfbp;
+	struct xfs_agi		*agi;
+	struct xfs_agf		*agf;
+	int			error, err2;
+
+	ASSERT(agno == mp->m_sb.sb_agcount - 1);
+	error = xfs_ialloc_read_agi(mp, *tpp, agno, &agibp);
+	if (error)
+		return error;
+
+	agi = agibp->b_addr;
+
+	error = xfs_alloc_read_agf(mp, *tpp, agno, 0, &agfbp);
+	if (error)
+		return error;
+
+	agf = agfbp->b_addr;
+	if (XFS_IS_CORRUPT(mp, agf->agf_length != agi->agi_length))
+		return -EFSCORRUPTED;

Is this check here for a reason? It seems a bit random, so I wonder if
we should just leave the extra verification to buffer verifiers.

It came from Darrick's thought. I'm fine with either way, but I feel
confused if different conflict opinions here:
https://lore.kernel.org/linux-xfs/20210303181931.GB3419940@magnolia/ (local)

Darrick's comment seems to refer to the check below. I'm referring to
the check above that agi_length and agf_length match. Are they intended
to go together? The check above seems to preexist the one below.

Sorry, update link of this:
https://lore.kernel.org/r/20210111181753.GC1164246@magnolia/ (local)

Anyways, if so, maybe just bunch them together and add a comment:

	/* some extra paranoid checks before we shrink the ag */
	if (XFS_IS_CORRUPT(...))
		return -EFSCORRUPTED;
	if (delta >= agf->agf_length)
		return -EVINAL; 

ok, will update this.

quoted

quoted

quoted

+
+	if (delta >= agi->agi_length)
+		return -EINVAL;
+
+	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
+				    be32_to_cpu(agi->agi_length) - delta);
+
+	/* remove the preallocations before allocation and re-establish then */

...

quoted

quoted

quoted

diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 5166322807e7..41293ebde8da 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h

@@ -24,8 +24,10 @@ struct aghdr_init_data {
 };
 
 int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
+int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans **tpp,
+			xfs_agnumber_t agno, xfs_extlen_t len);
 int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
-			struct aghdr_init_data *id, xfs_extlen_t len);
+			struct aghdr_init_data *id, xfs_extlen_t delta);

This looks misplaced..?

Or maybe this is trying to make the APIs consistent, but the function
definition still uses len as well as the declaration for
_ag_shrink_space() (while the definition of that function uses delta).

FWIW, the name delta tends to suggest a signed value to me based on our
pattern of usage, whereas here it seems like these helpers always want a
positive value (i.e. a length).

Yeah, it's just misplaced, thanks for pointing out, sorry about that.
`delta' name came from, `len' is confusing to Darrick.
https://lore.kernel.org/r/20210303182527.GC3419940@magnolia/ (local)

Fair enough. I'm not worried about the name, just pointing out some
potential inconsistencies.

Thanks for pointing out!

Thanks,
Gao Xiang

Brian

quoted

Thanks,
Gao Xiang

quoted

Brian

quoted

 int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
 			struct xfs_ag_geometry *ageo);
 
-- 
2.27.0