On 1/26/22 10:38 PM, Chandan Babu R wrote:
On 24 Jan 2022 at 10:56, 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.
As part of recovering an intent item, xfs_defer_ops_capture_and_commit()
invokes xfs_defer_save_resources(). Here we save/capture those xfs_bufs which
have XFS_BLI_HOLD flag set. AFAICT, these xfs_bufs are already locked. When
the transaction is committed to the CIL, iop_committing()
(i.e. xfs_buf_item_committing()) routine is invoked. Here we refrain from
unlocking an xfs_buf if XFS_BLI_HOLD flag is set. Hence the xfs_buf continues
to be in locked state.
Later, When processing the captured list (via xlog_finish_defer_ops()),
wouldn't locking the same xfs_buf by xfs_defer_ops_continue() cause a
deadlock?
Well, currently the attr code may take the lock at some point during the
operation and then lets it go later when it no longer needs it. So that
is where the corresponding unlock comes from. Ideally, the delay replay
and the log replay should behave the same so that the underlying
operation doesn't need to know about it, or do anything different. I
think the attr operation is the first to use this lock hold over during
a journal replay though, so I suspect there just wasn't very much
testing to exercise it when the defer capture port went in. It comes up
pretty quickly with the new log attribute replay test though.
If it helps to see it, it's easy to reproduce:
Build/install both kernel and user space branches, as well as the test cases
https://github.com/allisonhenderson/xfs_work/tree/delayed_attrs_v26_extended
https://github.com/allisonhenderson/xfs_work/tree/delayed_attrs_xfsprogs_v26_extended
https://github.com/allisonhenderson/xfs_work/tree/pptr_xfstestsv5
Turn on the log attr replay feature:
echo 1 > /sys/fs/xfs/debug/larp
Run new journal replay test
./check xfs/542
Test should pass as it is. Reverse apply this patch to see the bug.
Hope this helps!
Allison
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.
Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
---
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);
