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@xxxxxxxxxx> > > > > --- > > > > 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.
