| Message ID | 20220922054458.40826-6-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Parent Pointers | expand |
On Wed, Sep 21, 2022 at 10:44:37PM -0700, allison.henderson@oracle.com wrote: > From: Allison Henderson <allison.henderson@oracle.com> > > Modify xfs_rename to hold all inode locks across a rename operation > We will need this later when we add parent pointers > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/xfs_inode.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 9a3174a8f895..4bfa4a1579f0 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2837,18 +2837,16 @@ xfs_rename( > xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL); > > /* > - * Join all the inodes to the transaction. From this point on, > - * we can rely on either trans_commit or trans_cancel to unlock > - * them. > + * Join all the inodes to the transaction. > */ > - xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL); > + xfs_trans_ijoin(tp, src_dp, 0); > if (new_parent) > - xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL); > - xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL); > + xfs_trans_ijoin(tp, target_dp, 0); > + xfs_trans_ijoin(tp, src_ip, 0); > if (target_ip) > - xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL); > + xfs_trans_ijoin(tp, target_ip, 0); > if (wip) > - xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL); > + xfs_trans_ijoin(tp, wip, 0); > > /* > * If we are using project inheritance, we only allow renames > @@ -2862,10 +2860,12 @@ xfs_rename( > } > > /* RENAME_EXCHANGE is unique from here on. */ > - if (flags & RENAME_EXCHANGE) > - return xfs_cross_rename(tp, src_dp, src_name, src_ip, > + if (flags & RENAME_EXCHANGE) { > + error = xfs_cross_rename(tp, src_dp, src_name, src_ip, > target_dp, target_name, target_ip, > spaceres); > + goto out_unlock; > + } > > /* > * Try to reserve quota to handle an expansion of the target directory. > @@ -3090,12 +3090,21 @@ xfs_rename( > xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE); > > error = xfs_finish_rename(tp); > - if (wip) > - xfs_irele(wip); > - return error; > + > + goto out_unlock; > > out_trans_cancel: > xfs_trans_cancel(tp); > +out_unlock: > + /* Unlock inodes in reverse order */ > + for (i = num_inodes - 1; i >= 0; i--) { > + if (inodes[i]) > + xfs_iunlock(inodes[i], XFS_ILOCK_EXCL); > + > + /* Skip duplicate inodes if src and target dps are the same */ > + if (i && (inodes[i] == inodes[i - 1])) > + i--; > + } Could you hoist this to a static inline xfs_iunlock_after_rename function that is adjacent to xfs_sort_for_rename, please? It's easier to verify that it does the right thing w.r.t. multiple array references pointing to the same incore inode when the two array management functions are right next to each other. static inline void xfs_iunlock_after_rename( struct xfs_inode **i_tab, int num_inodes) { for (i = num_inodes - 1; i >= 0; i--) { /* Skip duplicate inodes if src and target dps are the same */ if (!i_tab[i] || (i > 0 && i_tab[i] == i_tab[i - 1])) continue; xfs_iunlock(i_tab[i], XFS_ILOCK_EXCL); } } With that cleaned up, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > out_release_wip: > if (wip) > xfs_irele(wip); > -- > 2.25.1 >
On Fri, 2022-09-23 at 12:21 -0700, Darrick J. Wong wrote: > On Wed, Sep 21, 2022 at 10:44:37PM -0700, > allison.henderson@oracle.com wrote: > > From: Allison Henderson <allison.henderson@oracle.com> > > > > Modify xfs_rename to hold all inode locks across a rename operation > > We will need this later when we add parent pointers > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > > --- > > fs/xfs/xfs_inode.c | 35 ++++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index 9a3174a8f895..4bfa4a1579f0 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -2837,18 +2837,16 @@ xfs_rename( > > xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL); > > > > /* > > - * Join all the inodes to the transaction. From this point > > on, > > - * we can rely on either trans_commit or trans_cancel to > > unlock > > - * them. > > + * Join all the inodes to the transaction. > > */ > > - xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL); > > + xfs_trans_ijoin(tp, src_dp, 0); > > if (new_parent) > > - xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL); > > - xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL); > > + xfs_trans_ijoin(tp, target_dp, 0); > > + xfs_trans_ijoin(tp, src_ip, 0); > > if (target_ip) > > - xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL); > > + xfs_trans_ijoin(tp, target_ip, 0); > > if (wip) > > - xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL); > > + xfs_trans_ijoin(tp, wip, 0); > > > > /* > > * If we are using project inheritance, we only allow > > renames > > @@ -2862,10 +2860,12 @@ xfs_rename( > > } > > > > /* RENAME_EXCHANGE is unique from here on. */ > > - if (flags & RENAME_EXCHANGE) > > - return xfs_cross_rename(tp, src_dp, src_name, > > src_ip, > > + if (flags & RENAME_EXCHANGE) { > > + error = xfs_cross_rename(tp, src_dp, src_name, > > src_ip, > > target_dp, target_name, > > target_ip, > > spaceres); > > + goto out_unlock; > > + } > > > > /* > > * Try to reserve quota to handle an expansion of the > > target directory. > > @@ -3090,12 +3090,21 @@ xfs_rename( > > xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE); > > > > error = xfs_finish_rename(tp); > > - if (wip) > > - xfs_irele(wip); > > - return error; > > + > > + goto out_unlock; > > > > out_trans_cancel: > > xfs_trans_cancel(tp); > > +out_unlock: > > + /* Unlock inodes in reverse order */ > > + for (i = num_inodes - 1; i >= 0; i--) { > > + if (inodes[i]) > > + xfs_iunlock(inodes[i], XFS_ILOCK_EXCL); > > + > > + /* Skip duplicate inodes if src and target dps are > > the same */ > > + if (i && (inodes[i] == inodes[i - 1])) > > + i--; > > + } > > Could you hoist this to a static inline xfs_iunlock_after_rename > function that is adjacent to xfs_sort_for_rename, please? It's > easier > to verify that it does the right thing w.r.t. multiple array > references > pointing to the same incore inode when the two array management > functions are right next to each other. > > static inline void > xfs_iunlock_after_rename( > struct xfs_inode **i_tab, > int num_inodes) > { > for (i = num_inodes - 1; i >= 0; i--) { > /* Skip duplicate inodes if src and target dps are > the same */ > if (!i_tab[i] || (i > 0 && i_tab[i] == i_tab[i - 1])) > continue; > xfs_iunlock(i_tab[i], XFS_ILOCK_EXCL); > } > } > Sure, that looks fine. Will do. Thanks! Allison > With that cleaned up, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D > > > out_release_wip: > > if (wip) > > xfs_irele(wip); > > -- > > 2.25.1 > >
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 9a3174a8f895..4bfa4a1579f0 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2837,18 +2837,16 @@ xfs_rename( xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL); /* - * Join all the inodes to the transaction. From this point on, - * we can rely on either trans_commit or trans_cancel to unlock - * them. + * Join all the inodes to the transaction. */ - xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, src_dp, 0); if (new_parent) - xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL); - xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, target_dp, 0); + xfs_trans_ijoin(tp, src_ip, 0); if (target_ip) - xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, target_ip, 0); if (wip) - xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL); + xfs_trans_ijoin(tp, wip, 0); /* * If we are using project inheritance, we only allow renames @@ -2862,10 +2860,12 @@ xfs_rename( } /* RENAME_EXCHANGE is unique from here on. */ - if (flags & RENAME_EXCHANGE) - return xfs_cross_rename(tp, src_dp, src_name, src_ip, + if (flags & RENAME_EXCHANGE) { + error = xfs_cross_rename(tp, src_dp, src_name, src_ip, target_dp, target_name, target_ip, spaceres); + goto out_unlock; + } /* * Try to reserve quota to handle an expansion of the target directory. @@ -3090,12 +3090,21 @@ xfs_rename( xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE); error = xfs_finish_rename(tp); - if (wip) - xfs_irele(wip); - return error; + + goto out_unlock; out_trans_cancel: xfs_trans_cancel(tp); +out_unlock: + /* Unlock inodes in reverse order */ + for (i = num_inodes - 1; i >= 0; i--) { + if (inodes[i]) + xfs_iunlock(inodes[i], XFS_ILOCK_EXCL); + + /* Skip duplicate inodes if src and target dps are the same */ + if (i && (inodes[i] == inodes[i - 1])) + i--; + } out_release_wip: if (wip) xfs_irele(wip);