On Mon, May 10, 2021 at 08:53:28AM -0400, Brian Foster wrote:

On Thu, May 06, 2021 at 05:20:34PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

The perag structures really need to be defined with the rest of the
AG support infrastructure. The struct xfs_perag and init/teardown
has been placed in xfs_mount.[ch] because there are differences in
the structure between kernel and userspace. Mainly that userspace
doesn't have a lot of the internal stuff that the kernel has for
caches and discard and other such structures.

However, it makes more sense to move this to libxfs than to keep
this separation because we are now moving to use struct perags
everywhere in the code instead of passing raw agnumber_t values
about. Hence we shoudl really move the support infrastructure to
libxfs/xfs_ag.[ch].

To do this without breaking userspace, first we need to rearrange
the structures and code so that all the kernel specific code is
located together. This makes it simple for userspace to ifdef out
the all the parts it does not need, minimising the code differences
between kernel and userspace. The next commit will do the move...

Signed-off-by: Dave Chinner <redacted>
---
 fs/xfs/xfs_mount.c | 50 ++++++++++++++++++++++++++--------------------
 fs/xfs/xfs_mount.h | 19 +++++++++---------
 2 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 21c630dde476..2e6d42014346 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c

...

quoted

@@ -229,13 +220,27 @@ xfs_initialize_perag(
 		}
 		spin_unlock(&mp->m_perag_lock);
 		radix_tree_preload_end();
-		/* first new pag is fully initialized */
-		if (first_initialised == NULLAGNUMBER)
-			first_initialised = index;
+
+		spin_lock_init(&pag->pag_ici_lock);
+		spin_lock_init(&pag->pagb_lock);
+		spin_lock_init(&pag->pag_state_lock);
+		INIT_DELAYED_WORK(&pag->pag_blockgc_work, xfs_blockgc_worker);
+		INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
+		init_waitqueue_head(&pag->pagb_wait);
+		pag->pagb_count = 0;
+		pag->pagb_tree = RB_ROOT;
+
+		error = xfs_buf_hash_init(pag);
+		if (error)
+			goto out_free_pag;
+

There's error handling code earlier up in this function that still lands
in out_hash_destroy, which is now before we get to the _hash_init()
call.

quoted

 		error = xfs_iunlink_init(pag);
 		if (error)
 			goto out_hash_destroy;
-		spin_lock_init(&pag->pag_state_lock);
+
+		/* first new pag is fully initialized */
+		if (first_initialised == NULLAGNUMBER)
+			first_initialised = index;
 	}
 
 	index = xfs_set_inode_alloc(mp, agcount);

@@ -249,6 +254,7 @@ xfs_initialize_perag(
 out_hash_destroy:
 	xfs_buf_hash_destroy(pag);
 out_free_pag:
+	pag = radix_tree_delete(&mp->m_perag_tree, index);

Now if we get here with an allocated pag that hasn't been inserted to
the tree, I suspect this call would assign pag = NULL..

Yup, error handling was fubar. Fixed now.

-- 
Dave Chinner
david@fromorbit.com