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

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

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.

--D

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