Re: [PATCH 11/22] xfs: add a perag to the btree cursor

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, May 12, 2021 at 08:49:28AM -0400, Brian Foster wrote:
> On Wed, May 12, 2021 at 07:52:50AM +1000, Dave Chinner wrote:
> > On Tue, May 11, 2021 at 01:51:52PM -0700, Darrick J. Wong wrote:
> > > On Tue, May 11, 2021 at 08:30:22AM -0400, Brian Foster wrote:
> > > > On Thu, May 06, 2021 at 05:20:43PM +1000, Dave Chinner wrote:
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > 
> > > > > Which will eventually completely replace the agno in it.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  fs/xfs/libxfs/xfs_alloc.c          | 25 +++++++++++++++----------
> > > > >  fs/xfs/libxfs/xfs_alloc_btree.c    | 13 ++++++++++---
> > > > >  fs/xfs/libxfs/xfs_alloc_btree.h    |  3 ++-
> > > > >  fs/xfs/libxfs/xfs_btree.c          |  2 ++
> > > > >  fs/xfs/libxfs/xfs_btree.h          |  4 +++-
> > > > >  fs/xfs/libxfs/xfs_ialloc.c         | 16 ++++++++--------
> > > > >  fs/xfs/libxfs/xfs_ialloc_btree.c   | 15 +++++++++++----
> > > > >  fs/xfs/libxfs/xfs_ialloc_btree.h   |  7 ++++---
> > > > >  fs/xfs/libxfs/xfs_refcount.c       |  4 ++--
> > > > >  fs/xfs/libxfs/xfs_refcount_btree.c | 17 ++++++++++++-----
> > > > >  fs/xfs/libxfs/xfs_refcount_btree.h |  2 +-
> > > > >  fs/xfs/libxfs/xfs_rmap.c           |  6 +++---
> > > > >  fs/xfs/libxfs/xfs_rmap_btree.c     | 17 ++++++++++++-----
> > > > >  fs/xfs/libxfs/xfs_rmap_btree.h     |  2 +-
> > > > >  fs/xfs/scrub/agheader_repair.c     | 20 +++++++++++---------
> > > > >  fs/xfs/scrub/bmap.c                |  2 +-
> > > > >  fs/xfs/scrub/common.c              | 12 ++++++------
> > > > >  fs/xfs/scrub/repair.c              |  5 +++--
> > > > >  fs/xfs/xfs_discard.c               |  2 +-
> > > > >  fs/xfs/xfs_fsmap.c                 |  6 +++---
> > > > >  fs/xfs/xfs_reflink.c               |  2 +-
> > > > >  21 files changed, 112 insertions(+), 70 deletions(-)
> > > > > 
> > > > ...
> > > > > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > > > > index 0f12b885600d..44044317c0fb 100644
> > > > > --- a/fs/xfs/libxfs/xfs_btree.c
> > > > > +++ b/fs/xfs/libxfs/xfs_btree.c
> > > > > @@ -377,6 +377,8 @@ xfs_btree_del_cursor(
> > > > >  	       XFS_FORCED_SHUTDOWN(cur->bc_mp));
> > > > >  	if (unlikely(cur->bc_flags & XFS_BTREE_STAGING))
> > > > >  		kmem_free(cur->bc_ops);
> > > > > +	if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS) && cur->bc_ag.pag)
> > > > > +		xfs_perag_put(cur->bc_ag.pag);
> > > > 
> > > > What's the correlation with BTREE_LONG_PTRS?
> > 
> > Only the btrees that index agbnos within a specific AG use the
> > cur->bc_ag structure to store the agno the btree is rooted in.
> > These are all btrees that use short pointers.
> > 
> > IOWs, we need an agno to turn the agbno into a full fsbno, daddr,
> > inum or anything else with global scope. Translation of short
> > pointers to physical location is necessary just to walk the tree,
> > while long pointer trees already record physical location of the
> > blocks within the tree and hence do not need an agno for
> > translation.
> > 
> > Hence needing the agno is specific, at this point in
> > time, to a btree containing short pointers.
> > 
> > > maybe this should be:
> > > 
> > > 	if (!(cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
> > > 		xfs_perag_put(cur->bc_ag.pag);
> > 
> > Given that the only long pointer btree we have is also the only
> > btree we have rooted in an inode, this is just another way of saying
> > !BTREE_LONG_PTRS. But "root in inode" is less obvious, because then we
> > lose the context taht "short pointers need translation via agno to
> > calculate their physical location in the wider filesystem"...

Ok.  Can you add a comment to xfs_btree.h somewhere noting that
LONG_PTRS is what we use to switch betwen bc_ag and bc_ino, please?

--D

> > 
> 
> Got it, thanks. The bc_ag field is unioned with bc_ino, the latter of
> which is used by XFS_BTREE_ROOT_IN_INODE trees to track inode
> information rather than AG information. I think the flag usage just
> threw me off at a glance. With that cleared up:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > If we are going to put short ptrs in an inode, then at that point
> > we need to change the btree cursor, anyway, because then we are
> > going to need both an inode pointer and something else to turn those
> > short pointers into global scope pointers for IO....
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
>