Re: [PATCH v1 16/17] xfs: Increase XFS_DEFER_OPS_NR_INODES to 4

[Alli <allison.henderson@oracle.com>]

On Wed, 2022-06-29 at 11:43 -0700, Darrick J. Wong wrote:
> On Fri, Jun 17, 2022 at 05:32:45PM -0700, Alli wrote:
> > On Fri, 2022-06-17 at 07:54 +1000, Dave Chinner wrote:
> > > On Sat, Jun 11, 2022 at 02:41:59AM -0700, Allison Henderson
> > > wrote:
> > > > Renames that generate parent pointer updates will need to 2
> > > > extra
> > > > defer
> > > > operations. One for the rmap update and another for the parent
> > > > pointer
> > > > update
> > > 
> > > Not sure I follow this - defer operation counts are something
> > > tracked in the transaction reservations, whilst this is changing
> > > the
> > > number of inodes that are joined and held across defer
> > > operations.
> > > 
> > > These rmap updates already occur on the directory inodes in a
> > > rename
> > > (when the dir update changes the dir shape), so I'm guessing that
> > > you are now talking about changing parent attrs for the child
> > > inodes
> > > may require attr fork shape changes (hence rmap updates) due to
> > > the
> > > deferred parent pointer xattr update?
> > > 
> > > If so, this should be placed in the series before the
> > > modifications
> > > to the rename operation is modified to join 4 ops to it,
> > > preferably
> > > at the start of the series....
> > 
> > I see, sure, I can move this patch down to the beginning of the set
> > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_defer.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_defer.h
> > > > b/fs/xfs/libxfs/xfs_defer.h
> > > > index 114a3a4930a3..0c2a6e537016 100644
> > > > --- a/fs/xfs/libxfs/xfs_defer.h
> > > > +++ b/fs/xfs/libxfs/xfs_defer.h
> > > > @@ -70,7 +70,7 @@ extern const struct xfs_defer_op_type
> > > > xfs_attr_defer_type;
> > > >  /*
> > > >   * Deferred operation item relogging limits.
> > > >   */
> > > > -#define XFS_DEFER_OPS_NR_INODES	2	/* join up to
> > > > two inodes */
> > > > +#define XFS_DEFER_OPS_NR_INODES	4	/* join up to
> > > > four inodes
> > > > */
> > > 
> > > The comment is not useful  - it should desvribe what operation
> > > requires 4 inodes to be joined. e.g.
> > > 
> > > /*
> > >  * Rename w/ parent pointers requires 4 indoes with defered ops
> > > to
> > >  * be joined to the transaction.
> > >  */
> > Sure, will update
> > 
> > > Then, if we are changing the maximum number of inodes that are
> > > joined to a deferred operation, then we need to also update the
> > > locking code such as in xfs_defer_ops_continue() that has to
> > > order
> > > locking of multiple inodes correctly.
> > Ok, I see it, I will take a look at updating that
> > 
> > > Also, rename can lock and modify 5 inodes, not 4, so the 4 inodes
> > > that get joined here need to be clearly documented somewhere. 
> > Ok, I think its src dir, target dir, src inode, target inode, and
> > then
> > wip.  Do we want the documenting in xfs_defer_ops_continue?  Or
> > just
> > the commit description?
> > 
> > > Also,
> > > xfs_sort_for_rename() that orders all the inodes in rename into
> > > correct locking order in an array, and xfs_lock_inodes() that
> > > does
> > > the locking of the inodes in the array.
> > Yes, I see it.  You want a comment in xfs_defer_ops_continue
> > referring
> > to the order?
> 
> I wouldn't mind one somewhere, though it could probably live with the
> parent pointer helper functions or buried in xfs_rename somewhere.
Alrighty, sounds good then

Allison
> 
> --D
> 
> > Thanks!
> > Allison
> > 
> > > Cheers,
> > > 
> > > Dave.