| Message ID | 20220216013713.1191082-2-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Log Attribute Replay | expand |
On 16 Feb 2022 at 07:06, Allison Henderson wrote: > The new deferred attr patch set uncovered a double unlock in the > recent port of the defer ops capture and continue code. During log > recovery, we're allowed to hold buffers to a transaction that's being > used to replay an intent item. When we capture the resources as part > of scheduling a continuation of an intent chain, we call xfs_buf_hold > to retain our reference to the buffer beyond the transaction commit, > but we do /not/ call xfs_trans_bhold to maintain the buffer lock. > This means that xfs_defer_ops_continue needs to relock the buffers > before xfs_defer_restore_resources joins then tothe new transaction. > > Additionally, the buffers should not be passed back via the dres > structure since they need to remain locked unlike the inodes. So > simply set dr_bufs to zero after populating the dres structure. > Looks good to me. Reviewed-by: Chandan Babu R <chandan.babu@oracle.com> > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_defer.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c > index 0805ade2d300..6dac8d6b8c21 100644 > --- a/fs/xfs/libxfs/xfs_defer.c > +++ b/fs/xfs/libxfs/xfs_defer.c > @@ -22,6 +22,7 @@ > #include "xfs_refcount.h" > #include "xfs_bmap.h" > #include "xfs_alloc.h" > +#include "xfs_buf.h" > > static struct kmem_cache *xfs_defer_pending_cache; > > @@ -774,17 +775,25 @@ xfs_defer_ops_continue( > struct xfs_trans *tp, > struct xfs_defer_resources *dres) > { > + unsigned int i; > + > ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES); > ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY)); > > - /* Lock and join the captured inode to the new transaction. */ > + /* Lock the captured resources to the new transaction. */ > if (dfc->dfc_held.dr_inos == 2) > xfs_lock_two_inodes(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL, > dfc->dfc_held.dr_ip[1], XFS_ILOCK_EXCL); > else if (dfc->dfc_held.dr_inos == 1) > xfs_ilock(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL); > + > + for (i = 0; i < dfc->dfc_held.dr_bufs; i++) > + xfs_buf_lock(dfc->dfc_held.dr_bp[i]); > + > + /* Join the captured resources to the new transaction. */ > xfs_defer_restore_resources(tp, &dfc->dfc_held); > memcpy(dres, &dfc->dfc_held, sizeof(struct xfs_defer_resources)); > + dres->dr_bufs = 0; > > /* Move captured dfops chain and state to the transaction. */ > list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c index 0805ade2d300..6dac8d6b8c21 100644 --- a/fs/xfs/libxfs/xfs_defer.c +++ b/fs/xfs/libxfs/xfs_defer.c @@ -22,6 +22,7 @@ #include "xfs_refcount.h" #include "xfs_bmap.h" #include "xfs_alloc.h" +#include "xfs_buf.h" static struct kmem_cache *xfs_defer_pending_cache; @@ -774,17 +775,25 @@ xfs_defer_ops_continue( struct xfs_trans *tp, struct xfs_defer_resources *dres) { + unsigned int i; + ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES); ASSERT(!(tp->t_flags & XFS_TRANS_DIRTY)); - /* Lock and join the captured inode to the new transaction. */ + /* Lock the captured resources to the new transaction. */ if (dfc->dfc_held.dr_inos == 2) xfs_lock_two_inodes(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL, dfc->dfc_held.dr_ip[1], XFS_ILOCK_EXCL); else if (dfc->dfc_held.dr_inos == 1) xfs_ilock(dfc->dfc_held.dr_ip[0], XFS_ILOCK_EXCL); + + for (i = 0; i < dfc->dfc_held.dr_bufs; i++) + xfs_buf_lock(dfc->dfc_held.dr_bp[i]); + + /* Join the captured resources to the new transaction. */ xfs_defer_restore_resources(tp, &dfc->dfc_held); memcpy(dres, &dfc->dfc_held, sizeof(struct xfs_defer_resources)); + dres->dr_bufs = 0; /* Move captured dfops chain and state to the transaction. */ list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);