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. --D > > Thanks! > Allison > > > > > Cheers, > > > > Dave. >
