| Message ID | 20220611094200.129502-17-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Return of the Parent Pointers | expand |
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.... > 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. */ 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. Also, rename can lock and modify 5 inodes, not 4, so the 4 inodes that get joined here need to be clearly documented somewhere. 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. Cheers, Dave.
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? Thanks! Allison > > Cheers, > > Dave.
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. >
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.
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 */ #define XFS_DEFER_OPS_NR_BUFS 2 /* join up to two buffers */ /* Resources that must be held across a transaction roll. */
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 Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_defer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)