* [PATCH 0/9 v2] xfs: log item and transaction cleanups
@ 2018-05-08 3:41 Dave Chinner
2018-05-08 3:41 ` [PATCH 1/9] xfs: log item flags are racy Dave Chinner
` (8 more replies)
0 siblings, 9 replies; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
Hi folks,
This is an updated version of the patchset that emerged from trying
to remove the log item descriptors. First version was here:
https://marc.info/?l=linux-xfs&m=152524812914200&w=2
This version addresses the initial review comments.
Version 2:
- rebased on current for-next tree
- get rid of XFS_LI_TRANS (was patch 2)
- rework AIL assert failure patch with Brian's suggestions
- reworked removal of log item descriptor patch to add checks that
XFS_LI_TRANS was providing.
Cheers,
Dave.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/9] xfs: log item flags are racy
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-09 14:51 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 2/9] xfs: add tracing to high level transaction operations Dave Chinner
` (7 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
The log item flags contain a field that is protected by the AIL
lock - the XFS_LI_IN_AIL flag. We use non-atomic RMW operations to
set and clear these flags, but most of the updates and checks are
not done with the AIL lock held and so are susceptible to update
races.
Fix this by changing the log item flags to use atomic bitops rather
than be reliant on the AIL lock for update serialisation.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_bmap_item.c | 4 ++--
fs/xfs/xfs_buf_item.c | 4 +++-
fs/xfs/xfs_dquot.c | 7 +++----
fs/xfs/xfs_dquot_item.c | 2 +-
fs/xfs/xfs_extfree_item.c | 4 ++--
fs/xfs/xfs_icache.c | 3 ++-
fs/xfs/xfs_icreate_item.c | 2 +-
fs/xfs/xfs_inode.c | 4 ++--
fs/xfs/xfs_inode_item.c | 8 ++++----
fs/xfs/xfs_log.c | 2 +-
fs/xfs/xfs_qm.c | 2 +-
fs/xfs/xfs_refcount_item.c | 4 ++--
fs/xfs/xfs_rmap_item.c | 4 ++--
fs/xfs/xfs_trace.h | 6 +++---
fs/xfs/xfs_trans.c | 4 ++--
fs/xfs/xfs_trans.h | 19 ++++++++++++-------
fs/xfs/xfs_trans_ail.c | 9 ++++-----
fs/xfs/xfs_trans_buf.c | 2 +-
fs/xfs/xfs_trans_priv.h | 10 ++++------
19 files changed, 52 insertions(+), 48 deletions(-)
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2203465e63ea..618bb71535c8 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -160,7 +160,7 @@ STATIC void
xfs_bui_item_unlock(
struct xfs_log_item *lip)
{
- if (lip->li_flags & XFS_LI_ABORTED)
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
xfs_bui_release(BUI_ITEM(lip));
}
@@ -305,7 +305,7 @@ xfs_bud_item_unlock(
{
struct xfs_bud_log_item *budp = BUD_ITEM(lip);
- if (lip->li_flags & XFS_LI_ABORTED) {
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
xfs_bui_release(budp->bud_buip);
kmem_zone_free(xfs_bud_zone, budp);
}
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 82ad270e390e..df62082f2204 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -568,13 +568,15 @@ xfs_buf_item_unlock(
{
struct xfs_buf_log_item *bip = BUF_ITEM(lip);
struct xfs_buf *bp = bip->bli_buf;
- bool aborted = !!(lip->li_flags & XFS_LI_ABORTED);
+ bool aborted;
bool hold = !!(bip->bli_flags & XFS_BLI_HOLD);
bool dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
#if defined(DEBUG) || defined(XFS_WARN)
bool ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
#endif
+ aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
+
/* Clear the buffer's association with this transaction. */
bp->b_transp = NULL;
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index a7daef9e16bf..d0b65679baae 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -913,9 +913,9 @@ xfs_qm_dqflush_done(
* since it's cheaper, and then we recheck while
* holding the lock before removing the dquot from the AIL.
*/
- if ((lip->li_flags & XFS_LI_IN_AIL) &&
+ if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) &&
((lip->li_lsn == qip->qli_flush_lsn) ||
- (lip->li_flags & XFS_LI_FAILED))) {
+ test_bit(XFS_LI_FAILED, &lip->li_flags))) {
/* xfs_trans_ail_delete() drops the AIL lock. */
spin_lock(&ailp->ail_lock);
@@ -926,8 +926,7 @@ xfs_qm_dqflush_done(
* Clear the failed state since we are about to drop the
* flush lock
*/
- if (lip->li_flags & XFS_LI_FAILED)
- xfs_clear_li_failed(lip);
+ xfs_clear_li_failed(lip);
spin_unlock(&ailp->ail_lock);
}
}
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 4b331e354da7..57df98122156 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -173,7 +173,7 @@ xfs_qm_dquot_logitem_push(
* The buffer containing this item failed to be written back
* previously. Resubmit the buffer for IO
*/
- if (lip->li_flags & XFS_LI_FAILED) {
+ if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
if (!xfs_buf_trylock(bp))
return XFS_ITEM_LOCKED;
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index b5b1e567b9f4..70b7d48af6d6 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -168,7 +168,7 @@ STATIC void
xfs_efi_item_unlock(
struct xfs_log_item *lip)
{
- if (lip->li_flags & XFS_LI_ABORTED)
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
xfs_efi_release(EFI_ITEM(lip));
}
@@ -402,7 +402,7 @@ xfs_efd_item_unlock(
{
struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
- if (lip->li_flags & XFS_LI_ABORTED) {
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
xfs_efi_release(efdp->efd_efip);
xfs_efd_item_free(efdp);
}
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 9a18f69f6e96..98b7a4ae15e4 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -107,7 +107,8 @@ xfs_inode_free_callback(
xfs_idestroy_fork(ip, XFS_COW_FORK);
if (ip->i_itemp) {
- ASSERT(!(ip->i_itemp->ili_item.li_flags & XFS_LI_IN_AIL));
+ ASSERT(!test_bit(XFS_LI_IN_AIL,
+ &ip->i_itemp->ili_item.li_flags));
xfs_inode_item_destroy(ip);
ip->i_itemp = NULL;
}
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index 865ad1373e5e..a99a0f8aa528 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -91,7 +91,7 @@ xfs_icreate_item_unlock(
{
struct xfs_icreate_item *icp = ICR_ITEM(lip);
- if (icp->ic_item.li_flags & XFS_LI_ABORTED)
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
kmem_zone_free(xfs_icreate_zone, icp);
return;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2b70c8b4cee2..16a43ba884cc 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -498,7 +498,7 @@ xfs_lock_inodes(
if (!try_lock) {
for (j = (i - 1); j >= 0 && !try_lock; j--) {
lp = (xfs_log_item_t *)ips[j]->i_itemp;
- if (lp && (lp->li_flags & XFS_LI_IN_AIL))
+ if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags))
try_lock++;
}
}
@@ -598,7 +598,7 @@ xfs_lock_two_inodes(
* and try again.
*/
lp = (xfs_log_item_t *)ip0->i_itemp;
- if (lp && (lp->li_flags & XFS_LI_IN_AIL)) {
+ if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags)) {
if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(ip1_mode, 1))) {
xfs_iunlock(ip0, ip0_mode);
if ((++attempts % 5) == 0)
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 34b91b789702..3e5b8574818e 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -518,7 +518,7 @@ xfs_inode_item_push(
* The buffer containing this item failed to be written back
* previously. Resubmit the buffer for IO.
*/
- if (lip->li_flags & XFS_LI_FAILED) {
+ if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
if (!xfs_buf_trylock(bp))
return XFS_ITEM_LOCKED;
@@ -729,14 +729,14 @@ xfs_iflush_done(
*/
iip = INODE_ITEM(blip);
if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
- (blip->li_flags & XFS_LI_FAILED))
+ test_bit(XFS_LI_FAILED, &blip->li_flags))
need_ail++;
}
/* make sure we capture the state of the initial inode. */
iip = INODE_ITEM(lip);
if ((iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) ||
- lip->li_flags & XFS_LI_FAILED)
+ test_bit(XFS_LI_FAILED, &lip->li_flags))
need_ail++;
/*
@@ -803,7 +803,7 @@ xfs_iflush_abort(
xfs_inode_log_item_t *iip = ip->i_itemp;
if (iip) {
- if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
+ if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
xfs_trans_ail_remove(&iip->ili_item,
stale ? SHUTDOWN_LOG_IO_ERROR :
SHUTDOWN_CORRUPT_INCORE);
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 2fcd9ed5d075..e427864434c1 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2132,7 +2132,7 @@ xlog_print_trans(
xfs_warn(mp, "log item: ");
xfs_warn(mp, " type = 0x%x", lip->li_type);
- xfs_warn(mp, " flags = 0x%x", lip->li_flags);
+ xfs_warn(mp, " flags = 0x%lx", lip->li_flags);
if (!lv)
continue;
xfs_warn(mp, " niovecs = %d", lv->lv_niovecs);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index ec39ae274c78..4438fc446d18 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -173,7 +173,7 @@ xfs_qm_dqpurge(
ASSERT(atomic_read(&dqp->q_pincount) == 0);
ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
- !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
+ !test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
xfs_dqfunlock(dqp);
xfs_dqunlock(dqp);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 15c9393dd7a7..e5866b714d5f 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -159,7 +159,7 @@ STATIC void
xfs_cui_item_unlock(
struct xfs_log_item *lip)
{
- if (lip->li_flags & XFS_LI_ABORTED)
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
xfs_cui_release(CUI_ITEM(lip));
}
@@ -310,7 +310,7 @@ xfs_cud_item_unlock(
{
struct xfs_cud_log_item *cudp = CUD_ITEM(lip);
- if (lip->li_flags & XFS_LI_ABORTED) {
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
xfs_cui_release(cudp->cud_cuip);
kmem_zone_free(xfs_cud_zone, cudp);
}
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 06a07846c9b3..e5b5b3e7ef82 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -158,7 +158,7 @@ STATIC void
xfs_rui_item_unlock(
struct xfs_log_item *lip)
{
- if (lip->li_flags & XFS_LI_ABORTED)
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
xfs_rui_release(RUI_ITEM(lip));
}
@@ -331,7 +331,7 @@ xfs_rud_item_unlock(
{
struct xfs_rud_log_item *rudp = RUD_ITEM(lip);
- if (lip->li_flags & XFS_LI_ABORTED) {
+ if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
xfs_rui_release(rudp->rud_ruip);
kmem_zone_free(xfs_rud_zone, rudp);
}
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8955254b900e..7f4d961fae12 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -442,7 +442,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
__field(int, bli_refcount)
__field(unsigned, bli_flags)
__field(void *, li_desc)
- __field(unsigned, li_flags)
+ __field(unsigned long, li_flags)
),
TP_fast_assign(
__entry->dev = bip->bli_buf->b_target->bt_dev;
@@ -1018,7 +1018,7 @@ DECLARE_EVENT_CLASS(xfs_log_item_class,
__field(dev_t, dev)
__field(void *, lip)
__field(uint, type)
- __field(uint, flags)
+ __field(unsigned long, flags)
__field(xfs_lsn_t, lsn)
),
TP_fast_assign(
@@ -1070,7 +1070,7 @@ DECLARE_EVENT_CLASS(xfs_ail_class,
__field(dev_t, dev)
__field(void *, lip)
__field(uint, type)
- __field(uint, flags)
+ __field(unsigned long, flags)
__field(xfs_lsn_t, old_lsn)
__field(xfs_lsn_t, new_lsn)
),
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index d6d8f9d129a7..c6a2a3cb9df5 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -790,7 +790,7 @@ xfs_trans_free_items(
if (commit_lsn != NULLCOMMITLSN)
lip->li_ops->iop_committing(lip, commit_lsn);
if (abort)
- lip->li_flags |= XFS_LI_ABORTED;
+ set_bit(XFS_LI_ABORTED, &lip->li_flags);
lip->li_ops->iop_unlock(lip);
xfs_trans_free_item_desc(lidp);
@@ -861,7 +861,7 @@ xfs_trans_committed_bulk(
xfs_lsn_t item_lsn;
if (aborted)
- lip->li_flags |= XFS_LI_ABORTED;
+ set_bit(XFS_LI_ABORTED, &lip->li_flags);
item_lsn = lip->li_ops->iop_committed(lip, commit_lsn);
/* item_lsn of -1 means the item needs no further processing */
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 9d542dfe0052..51145ddf7e5b 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -48,7 +48,7 @@ typedef struct xfs_log_item {
struct xfs_mount *li_mountp; /* ptr to fs mount */
struct xfs_ail *li_ailp; /* ptr to AIL */
uint li_type; /* item type */
- uint li_flags; /* misc flags */
+ unsigned long li_flags; /* misc flags */
struct xfs_buf *li_buf; /* real buffer pointer */
struct list_head li_bio_list; /* buffer item list */
void (*li_cb)(struct xfs_buf *,
@@ -64,14 +64,19 @@ typedef struct xfs_log_item {
xfs_lsn_t li_seq; /* CIL commit seq */
} xfs_log_item_t;
-#define XFS_LI_IN_AIL 0x1
-#define XFS_LI_ABORTED 0x2
-#define XFS_LI_FAILED 0x4
+/*
+ * li_flags use the (set/test/clear)_bit atomic interfaces because updates can
+ * race with each other and we don't want to have to use the AIL lock to
+ * serialise all updates.
+ */
+#define XFS_LI_IN_AIL 0
+#define XFS_LI_ABORTED 1
+#define XFS_LI_FAILED 2
#define XFS_LI_FLAGS \
- { XFS_LI_IN_AIL, "IN_AIL" }, \
- { XFS_LI_ABORTED, "ABORTED" }, \
- { XFS_LI_FAILED, "FAILED" }
+ { (1 << XFS_LI_IN_AIL), "IN_AIL" }, \
+ { (1 << XFS_LI_ABORTED), "ABORTED" }, \
+ { (1 << XFS_LI_FAILED), "FAILED" }
struct xfs_item_ops {
void (*iop_size)(xfs_log_item_t *, int *, int *);
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index d4a2445215e6..50611d2bcbc2 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -46,7 +46,7 @@ xfs_ail_check(
/*
* Check the next and previous entries are valid.
*/
- ASSERT((lip->li_flags & XFS_LI_IN_AIL) != 0);
+ ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
if (&prev_lip->li_ail != &ailp->ail_head)
ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
@@ -684,7 +684,7 @@ xfs_trans_ail_update_bulk(
for (i = 0; i < nr_items; i++) {
struct xfs_log_item *lip = log_items[i];
- if (lip->li_flags & XFS_LI_IN_AIL) {
+ if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
/* check if we really need to move the item */
if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
continue;
@@ -694,7 +694,6 @@ xfs_trans_ail_update_bulk(
if (mlip == lip)
mlip_changed = 1;
} else {
- lip->li_flags |= XFS_LI_IN_AIL;
trace_xfs_ail_insert(lip, 0, lsn);
}
lip->li_lsn = lsn;
@@ -725,7 +724,7 @@ xfs_ail_delete_one(
trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
xfs_ail_delete(ailp, lip);
xfs_clear_li_failed(lip);
- lip->li_flags &= ~XFS_LI_IN_AIL;
+ clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
lip->li_lsn = 0;
return mlip == lip;
@@ -761,7 +760,7 @@ xfs_trans_ail_delete(
struct xfs_mount *mp = ailp->ail_mount;
bool mlip_changed;
- if (!(lip->li_flags & XFS_LI_IN_AIL)) {
+ if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
spin_unlock(&ailp->ail_lock);
if (!XFS_FORCED_SHUTDOWN(mp)) {
xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index a5d9dfc45d98..0081e9b3decf 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -442,7 +442,7 @@ xfs_trans_brelse(
ASSERT(bp->b_pincount == 0);
***/
ASSERT(atomic_read(&bip->bli_refcount) == 0);
- ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
+ ASSERT(!test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags));
ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
xfs_buf_item_relse(bp);
}
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index be24b0c8a332..43f773297b9d 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -119,7 +119,7 @@ xfs_trans_ail_remove(
spin_lock(&ailp->ail_lock);
/* xfs_trans_ail_delete() drops the AIL lock */
- if (lip->li_flags & XFS_LI_IN_AIL)
+ if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
xfs_trans_ail_delete(ailp, lip, shutdown_type);
else
spin_unlock(&ailp->ail_lock);
@@ -171,11 +171,10 @@ xfs_clear_li_failed(
{
struct xfs_buf *bp = lip->li_buf;
- ASSERT(lip->li_flags & XFS_LI_IN_AIL);
+ ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
lockdep_assert_held(&lip->li_ailp->ail_lock);
- if (lip->li_flags & XFS_LI_FAILED) {
- lip->li_flags &= ~XFS_LI_FAILED;
+ if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) {
lip->li_buf = NULL;
xfs_buf_rele(bp);
}
@@ -188,9 +187,8 @@ xfs_set_li_failed(
{
lockdep_assert_held(&lip->li_ailp->ail_lock);
- if (!(lip->li_flags & XFS_LI_FAILED)) {
+ if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) {
xfs_buf_hold(bp);
- lip->li_flags |= XFS_LI_FAILED;
lip->li_buf = bp;
}
}
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/9] xfs: add tracing to high level transaction operations
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
2018-05-08 3:41 ` [PATCH 1/9] xfs: log item flags are racy Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-09 14:51 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints Dave Chinner
` (6 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Because currently we have no idea what the transaction context we
are operating in is, and I need to know that information to track
down bugs in multiple log item joins to transactions.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_log_cil.c | 1 +
fs/xfs/xfs_trace.h | 37 +++++++++++++++++++++++++++++++++++++
fs/xfs/xfs_trans.c | 20 +++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 4668403b1741..a8675c631438 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1013,6 +1013,7 @@ xfs_log_commit_cil(
*commit_lsn = xc_commit_lsn;
xfs_log_done(mp, tp->t_ticket, NULL, regrant);
+ tp->t_ticket = NULL;
xfs_trans_unreserve_and_mod_sb(tp);
/*
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 7f4d961fae12..8136f2280173 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -3346,6 +3346,43 @@ TRACE_EVENT(xfs_trans_resv_calc,
__entry->logflags)
);
+DECLARE_EVENT_CLASS(xfs_trans_class,
+ TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip),
+ TP_ARGS(tp, caller_ip),
+ TP_STRUCT__entry(
+ __field(dev_t, dev)
+ __field(uint32_t, tid)
+ __field(uint32_t, flags)
+ __field(unsigned long, caller_ip)
+ ),
+ TP_fast_assign(
+ __entry->dev = tp->t_mountp->m_super->s_dev;
+ __entry->tid = 0;
+ if (tp->t_ticket)
+ __entry->tid = tp->t_ticket->t_tid;
+ __entry->flags = tp->t_flags;
+ __entry->caller_ip = caller_ip;
+ ),
+ TP_printk("dev %d:%d trans %x flags 0x%x caller %pS",
+ MAJOR(__entry->dev), MINOR(__entry->dev),
+ __entry->tid,
+ __entry->flags,
+ (char *)__entry->caller_ip)
+)
+
+#define DEFINE_TRANS_EVENT(name) \
+DEFINE_EVENT(xfs_trans_class, name, \
+ TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip), \
+ TP_ARGS(tp, caller_ip))
+DEFINE_TRANS_EVENT(xfs_trans_alloc);
+DEFINE_TRANS_EVENT(xfs_trans_cancel);
+DEFINE_TRANS_EVENT(xfs_trans_commit);
+DEFINE_TRANS_EVENT(xfs_trans_dup);
+DEFINE_TRANS_EVENT(xfs_trans_free);
+DEFINE_TRANS_EVENT(xfs_trans_roll);
+DEFINE_TRANS_EVENT(xfs_trans_add_item);
+DEFINE_TRANS_EVENT(xfs_trans_free_items);
+
#endif /* _TRACE_XFS_H */
#undef TRACE_INCLUDE_PATH
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index c6a2a3cb9df5..24744915357b 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -79,6 +79,7 @@ xfs_trans_free(
xfs_extent_busy_sort(&tp->t_busy);
xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
+ trace_xfs_trans_free(tp, _RET_IP_);
atomic_dec(&tp->t_mountp->m_active_trans);
if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT))
sb_end_intwrite(tp->t_mountp->m_super);
@@ -100,6 +101,8 @@ xfs_trans_dup(
{
xfs_trans_t *ntp;
+ trace_xfs_trans_dup(tp, _RET_IP_);
+
ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);
/*
@@ -283,6 +286,8 @@ xfs_trans_alloc(
return error;
}
+ trace_xfs_trans_alloc(tp, _RET_IP_);
+
*tpp = tp;
return 0;
}
@@ -749,6 +754,8 @@ xfs_trans_add_item(
list_add_tail(&lidp->lid_trans, &tp->t_items);
lip->li_desc = lidp;
+
+ trace_xfs_trans_add_item(tp, _RET_IP_);
}
STATIC void
@@ -782,6 +789,8 @@ xfs_trans_free_items(
{
struct xfs_log_item_desc *lidp, *next;
+ trace_xfs_trans_free_items(tp, _RET_IP_);
+
list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
struct xfs_log_item *lip = lidp->lid_item;
@@ -936,6 +945,8 @@ __xfs_trans_commit(
int error = 0;
int sync = tp->t_flags & XFS_TRANS_SYNC;
+ trace_xfs_trans_commit(tp, _RET_IP_);
+
/*
* If there is nothing to be logged by the transaction,
* then unlock all of the items associated with the
@@ -991,6 +1002,7 @@ __xfs_trans_commit(
commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
if (commit_lsn == -1 && !error)
error = -EIO;
+ tp->t_ticket = NULL;
}
current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
xfs_trans_free_items(tp, NULLCOMMITLSN, !!error);
@@ -1022,6 +1034,8 @@ xfs_trans_cancel(
struct xfs_mount *mp = tp->t_mountp;
bool dirty = (tp->t_flags & XFS_TRANS_DIRTY);
+ trace_xfs_trans_cancel(tp, _RET_IP_);
+
/*
* See if the caller is relying on us to shut down the
* filesystem. This happens in paths where we detect
@@ -1042,8 +1056,10 @@ xfs_trans_cancel(
xfs_trans_unreserve_and_mod_sb(tp);
xfs_trans_unreserve_and_mod_dquots(tp);
- if (tp->t_ticket)
+ if (tp->t_ticket) {
xfs_log_done(mp, tp->t_ticket, NULL, false);
+ tp->t_ticket = NULL;
+ }
/* mark this thread as no longer being in a transaction */
current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
@@ -1067,6 +1083,8 @@ xfs_trans_roll(
struct xfs_trans_res tres;
int error;
+ trace_xfs_trans_roll(trans, _RET_IP_);
+
/*
* Copy the critical parameters from one trans to the next.
*/
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
2018-05-08 3:41 ` [PATCH 1/9] xfs: log item flags are racy Dave Chinner
2018-05-08 3:41 ` [PATCH 2/9] xfs: add tracing to high level transaction operations Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-09 14:52 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
` (5 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
So it's clear in the trace where they are being called from.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_defer.c | 12 ++++++------
fs/xfs/xfs_trace.h | 17 +++++++++++------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 087fea02c389..51821af5d653 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -220,7 +220,7 @@ xfs_defer_trans_abort(
{
struct xfs_defer_pending *dfp;
- trace_xfs_defer_trans_abort(tp->t_mountp, dop);
+ trace_xfs_defer_trans_abort(tp->t_mountp, dop, _RET_IP_);
/* Abort intent items that don't have a done item. */
list_for_each_entry(dfp, &dop->dop_pending, dfp_list) {
@@ -253,7 +253,7 @@ xfs_defer_trans_roll(
for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
- trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
+ trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
/* Roll the transaction. */
error = xfs_trans_roll(tp);
@@ -355,7 +355,7 @@ xfs_defer_finish(
ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
- trace_xfs_defer_finish((*tp)->t_mountp, dop);
+ trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
/* Until we run out of pending work to finish... */
while (xfs_defer_has_unfinished_work(dop)) {
@@ -431,7 +431,7 @@ xfs_defer_finish(
if (error)
trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
else
- trace_xfs_defer_finish_done((*tp)->t_mountp, dop);
+ trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
return error;
}
@@ -447,7 +447,7 @@ xfs_defer_cancel(
struct list_head *pwi;
struct list_head *n;
- trace_xfs_defer_cancel(NULL, dop);
+ trace_xfs_defer_cancel(NULL, dop, _RET_IP_);
/*
* Free the pending items. Caller should already have arranged
@@ -532,5 +532,5 @@ xfs_defer_init(
*fbp = NULLFSBLOCK;
INIT_LIST_HEAD(&dop->dop_intake);
INIT_LIST_HEAD(&dop->dop_pending);
- trace_xfs_defer_init(NULL, dop);
+ trace_xfs_defer_init(NULL, dop, _RET_IP_);
}
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 8136f2280173..947ab6e32c18 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -2243,30 +2243,35 @@ struct xfs_defer_pending;
struct xfs_defer_ops;
DECLARE_EVENT_CLASS(xfs_defer_class,
- TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop),
- TP_ARGS(mp, dop),
+ TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop,
+ unsigned long caller_ip),
+ TP_ARGS(mp, dop, caller_ip),
TP_STRUCT__entry(
__field(dev_t, dev)
__field(void *, dop)
__field(char, committed)
__field(char, low)
+ __field(unsigned long, caller_ip)
),
TP_fast_assign(
__entry->dev = mp ? mp->m_super->s_dev : 0;
__entry->dop = dop;
__entry->committed = dop->dop_committed;
__entry->low = dop->dop_low;
+ __entry->caller_ip = caller_ip;
),
- TP_printk("dev %d:%d ops %p committed %d low %d",
+ TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
MAJOR(__entry->dev), MINOR(__entry->dev),
__entry->dop,
__entry->committed,
- __entry->low)
+ __entry->low,
+ (char *)__entry->caller_ip)
)
#define DEFINE_DEFER_EVENT(name) \
DEFINE_EVENT(xfs_defer_class, name, \
- TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop), \
- TP_ARGS(mp, dop))
+ TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, \
+ unsigned long caller_ip), \
+ TP_ARGS(mp, dop, caller_ip))
DECLARE_EVENT_CLASS(xfs_defer_error_class,
TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, int error),
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/9] xfs: don't assert fail with AIL lock held
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (2 preceding siblings ...)
2018-05-08 3:41 ` [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
` (2 more replies)
2018-05-08 3:41 ` [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt() Dave Chinner
` (4 subsequent siblings)
8 siblings, 3 replies; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Been hitting AIL ordering assert failures recently, but been unable
to trace them down because the system immediately hangs up onteh
spinlock that was held when this assert fires:
XFS: Assertion failed: XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0, file: fs/xfs/xfs_trans_ail.c, line: 52
Move the assertions outside of the spinlock so the corpse can
be dissected. Thanks to Brian Foster for supplying a clean
way of doing this.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_trans_ail.c | 43 +++++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 50611d2bcbc2..41e280ef1483 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -32,30 +32,51 @@
#ifdef DEBUG
/*
* Check that the list is sorted as it should be.
+ *
+ * Called with the ail lock held, but we don't want to assert fail with it
+ * held otherwise we'll lock everything up and won't be able to debug the
+ * cause. Hence we sample and check the state under the AIL lock and return if
+ * everything is fine, otherwise we drop the lock and run the ASSERT checks.
+ * Asserts may not be fatal, so pick the lock back up and continue onwards.
*/
STATIC void
xfs_ail_check(
- struct xfs_ail *ailp,
- xfs_log_item_t *lip)
+ struct xfs_ail *ailp,
+ struct xfs_log_item *lip)
{
- xfs_log_item_t *prev_lip;
+ struct xfs_log_item *prev_lip;
+ struct xfs_log_item *next_lip;
+ xfs_lsn_t prev_lsn = NULLCOMMITLSN;
+ xfs_lsn_t next_lsn = NULLCOMMITLSN;
+ xfs_lsn_t lsn;
+ bool in_ail;
+
if (list_empty(&ailp->ail_head))
return;
/*
- * Check the next and previous entries are valid.
+ * Sample then check the next and previous entries are valid.
*/
- ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
- prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
+ in_ail = test_bit(XFS_LI_IN_AIL, &lip->li_flags);
+ prev_lip = list_entry(lip->li_ail.prev, struct xfs_log_item, li_ail);
if (&prev_lip->li_ail != &ailp->ail_head)
- ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
-
- prev_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
- if (&prev_lip->li_ail != &ailp->ail_head)
- ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) >= 0);
+ prev_lsn = prev_lip->li_lsn;
+ next_lip = list_entry(lip->li_ail.next, struct xfs_log_item, li_ail);
+ if (&next_lip->li_ail != &ailp->ail_head)
+ next_lsn = next_lip->li_lsn;
+ lsn = lip->li_lsn;
+ if (in_ail &&
+ (prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0) &&
+ (next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0))
+ return;
+ spin_unlock(&ailp->ail_lock);
+ ASSERT(in_ail);
+ ASSERT(prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0);
+ ASSERT(next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0);
+ spin_lock(&ailp->ail_lock);
}
#else /* !DEBUG */
#define xfs_ail_check(a,l)
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (3 preceding siblings ...)
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-08 3:41 ` [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range Dave Chinner
` (3 subsequent siblings)
8 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
xfs_inactive_symlink_rmt() does something nasty - it joins an inode
into a transaction it is already joined to. This means the inode can
have multiple log item descriptors attached to the transaction for
it. This breaks teh 1:1 mapping that is supposed to exist
between the log item and log item descriptor.
This results in the log item being processed twice during
transaction commit and CIL formatting, and there are lots of other
potential issues tha arise from double processing of log items in
the transaction commit state machine.
In this case, the inode is already held by the rolling transaction
returned from xfs_defer_finish(), so there's no need to join it
again.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_symlink.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 5b66ac12913c..27870e5cd259 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
error = xfs_defer_finish(&tp, &dfops);
if (error)
goto error_bmap_cancel;
- /*
- * The first xact was committed, so add the inode to the new one.
- * Mark it dirty so it will be logged and moved forward in the log as
- * part of every commit.
- */
- xfs_trans_ijoin(tp, ip, 0);
- xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
/*
* Commit the transaction containing extent freeing and EFDs.
*/
+ xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
error = xfs_trans_commit(tp);
if (error) {
ASSERT(XFS_FORCED_SHUTDOWN(mp));
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (4 preceding siblings ...)
2018-05-08 3:41 ` [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt() Dave Chinner
@ 2018-05-08 3:41 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 15:17 ` Darrick J. Wong
2018-05-08 3:42 ` [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag() Dave Chinner
` (2 subsequent siblings)
8 siblings, 2 replies; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:41 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
AN inode is joined to teh same transaction twice in
xfs_reflink_cancel_cow_range() resulting in the following assert
failure:
[ 30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
[ 30.183435] ------------[ cut here ]------------
......
[ 30.209264] Call Trace:
[ 30.209935] xfs_trans_add_item+0xcc/0xe0
[ 30.210968] xfs_reflink_cancel_cow_blocks+0xab/0x290
[ 30.212249] ? xfs_trans_reserve+0x1b4/0x2b0
[ 30.213320] ? kmem_zone_alloc+0x61/0xe0
[ 30.214321] xfs_reflink_cancel_cow_range+0xb2/0x1f0
[ 30.215616] xfs_fs_destroy_inode+0x1bd/0x280
[ 30.216757] dispose_list+0x35/0x40
[ 30.217656] evict_inodes+0x132/0x160
[ 30.218620] generic_shutdown_super+0x3a/0x110
[ 30.219771] kill_block_super+0x21/0x50
[ 30.220762] deactivate_locked_super+0x39/0x70
[ 30.221909] cleanup_mnt+0x3b/0x70
[ 30.222819] task_work_run+0x7f/0xa0
[ 30.223762] exit_to_usermode_loop+0x9b/0xa0
[ 30.224884] do_syscall_64+0x18f/0x1a0
Fix it and document that the callers of
xfs_reflink_cancel_cow_blocks() must have already joined the inode
to the permanent transaction passed in.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cdbd342a5249..bce2b5351d64 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
*
* If cancel_real is true this function cancels all COW fork extents for the
* inode; if cancel_real is false, real extents are not cleared.
+ *
+ * Caller must have already joined the inode to the current transaction. The
+ * inode will be joined to the transaction returned to the caller.
*/
int
xfs_reflink_cancel_cow_blocks(
@@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
if (error)
break;
} else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
- xfs_trans_ijoin(*tpp, ip, 0);
xfs_defer_init(&dfops, &firstfsb);
/* Free the CoW orphan record. */
@@ -1570,6 +1572,7 @@ xfs_reflink_clear_inode_flag(
* We didn't find any shared blocks so turn off the reflink flag.
* First, get rid of any leftover CoW mappings.
*/
+ xfs_trans_ijoin(*tpp, ip, 0);
error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
if (error)
return error;
@@ -1578,7 +1581,6 @@ xfs_reflink_clear_inode_flag(
trace_xfs_reflink_unset_inode_flag(ip);
ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
xfs_inode_clear_cowblocks_tag(ip);
- xfs_trans_ijoin(*tpp, ip, 0);
xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
return error;
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (5 preceding siblings ...)
2018-05-08 3:41 ` [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range Dave Chinner
@ 2018-05-08 3:42 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-08 3:42 ` [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse Dave Chinner
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
8 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:42 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Another assert failure:
XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
....
xfs_trans_add_item+0xcc/0xe0
xfs_reflink_clear_inode_flag+0x53/0x120
xfs_reflink_try_clear_inode_flag+0x5b/0xa0
? filemap_write_and_wait+0x4f/0x70
xfs_reflink_unshare+0x18e/0x19d
xfs_file_fallocate+0x241/0x310
? selinux_file_permission+0xd4/0x140
vfs_fallocate+0x13d/0x260
SyS_fallocate+0x43/0x80
Another fix.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_reflink.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index bce2b5351d64..12d441a73b53 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
return 0;
}
-/* Clear the inode reflink flag if there are no shared extents. */
+/*
+ * Clear the inode reflink flag if there are no shared extents.
+ *
+ * The caller is responsible for joining the inode to the transaction passed in.
+ * The inode will be joined to the transaction that is returned to the caller.
+ */
int
xfs_reflink_clear_inode_flag(
struct xfs_inode *ip,
@@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
* We didn't find any shared blocks so turn off the reflink flag.
* First, get rid of any leftover CoW mappings.
*/
- xfs_trans_ijoin(*tpp, ip, 0);
error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
if (error)
return error;
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (6 preceding siblings ...)
2018-05-08 3:42 ` [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag() Dave Chinner
@ 2018-05-08 3:42 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 15:19 ` Darrick J. Wong
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
8 siblings, 2 replies; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:42 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Just to make sure the item isn't associated with another
transaction when we try to reuse it.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_buf_item.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index df62082f2204..8d6ed045b643 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -745,8 +745,10 @@ xfs_buf_item_init(
* nothing to do here so return.
*/
ASSERT(bp->b_target->bt_mount == mp);
- if (bip != NULL) {
+ if (bip) {
ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
+ ASSERT(!bp->b_transp);
+ ASSERT(bip->bli_buf == bp);
return 0;
}
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 9/9] xfs: get rid of the log item descriptor
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
` (7 preceding siblings ...)
2018-05-08 3:42 ` [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse Dave Chinner
@ 2018-05-08 3:42 ` Dave Chinner
2018-05-08 14:18 ` Brian Foster
` (2 more replies)
8 siblings, 3 replies; 35+ messages in thread
From: Dave Chinner @ 2018-05-08 3:42 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
It's just a connector between a transaction and a log item. There's
a 1:1 relationship between a log item descriptor and a log item,
and a 1:1 relationship between a log item descriptor and a
transaction. Both relationships are created and terminated at the
same time, so why do we even have the descriptor?
Replace it with a specific list_head in the log item and a new
log item dirtied flag to replace the XFS_LID_DIRTY flag.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/libxfs/xfs_bmap.c | 8 ++---
fs/xfs/libxfs/xfs_shared.h | 15 ----------
fs/xfs/xfs_buf_item.c | 2 +-
fs/xfs/xfs_icreate_item.c | 2 +-
fs/xfs/xfs_log.c | 10 +++----
fs/xfs/xfs_log_cil.c | 21 ++++++--------
fs/xfs/xfs_super.c | 10 +------
fs/xfs/xfs_trace.h | 5 +---
fs/xfs/xfs_trans.c | 58 ++++++++++---------------------------
fs/xfs/xfs_trans.h | 8 ++---
fs/xfs/xfs_trans_bmap.c | 4 +--
fs/xfs/xfs_trans_buf.c | 22 ++++++--------
fs/xfs/xfs_trans_dquot.c | 4 +--
fs/xfs/xfs_trans_extfree.c | 4 +--
fs/xfs/xfs_trans_inode.c | 3 +-
fs/xfs/xfs_trans_priv.h | 1 -
fs/xfs/xfs_trans_refcount.c | 4 +--
fs/xfs/xfs_trans_rmap.c | 4 +--
18 files changed, 62 insertions(+), 123 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 040eeda8426f..ab90998c1867 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -246,7 +246,7 @@ xfs_bmap_get_bp(
struct xfs_btree_cur *cur,
xfs_fsblock_t bno)
{
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
int i;
if (!cur)
@@ -260,9 +260,9 @@ xfs_bmap_get_bp(
}
/* Chase down all the log items to see if the bp is there */
- list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
- struct xfs_buf_log_item *bip;
- bip = (struct xfs_buf_log_item *)lidp->lid_item;
+ list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
+ struct xfs_buf_log_item *bip = (struct xfs_buf_log_item *)lip;
+
if (bip->bli_item.li_type == XFS_LI_BUF &&
XFS_BUF_ADDR(bip->bli_buf) == bno)
return bip->bli_buf;
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index d0b84da0cb1e..8efc06e62b13 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -57,21 +57,6 @@ extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
extern const struct xfs_buf_ops xfs_symlink_buf_ops;
extern const struct xfs_buf_ops xfs_rtbuf_ops;
-/*
- * This structure is used to track log items associated with
- * a transaction. It points to the log item and keeps some
- * flags to track the state of the log item. It also tracks
- * the amount of space needed to log the item it describes
- * once we get to commit processing (see xfs_trans_commit()).
- */
-struct xfs_log_item_desc {
- struct xfs_log_item *lid_item;
- struct list_head lid_trans;
- unsigned char lid_flags;
-};
-
-#define XFS_LID_DIRTY 0x1
-
/* log size calculation functions */
int xfs_log_calc_unit_res(struct xfs_mount *mp, int unit_bytes);
int xfs_log_calc_minimum_size(struct xfs_mount *);
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 8d6ed045b643..c2311379d1c3 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -438,7 +438,7 @@ xfs_buf_item_unpin(
* xfs_trans_uncommit() will try to reference the
* buffer which we no longer have a hold on.
*/
- if (lip->li_desc)
+ if (!list_empty(&lip->li_trans))
xfs_trans_del_item(lip);
/*
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index a99a0f8aa528..5da9599156ed 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -184,5 +184,5 @@ xfs_icreate_log(
xfs_trans_add_item(tp, &icp->ic_item);
tp->t_flags |= XFS_TRANS_DIRTY;
- icp->ic_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
}
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index e427864434c1..c21039f27e39 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1047,6 +1047,7 @@ xfs_log_item_init(
INIT_LIST_HEAD(&item->li_ail);
INIT_LIST_HEAD(&item->li_cil);
INIT_LIST_HEAD(&item->li_bio_list);
+ INIT_LIST_HEAD(&item->li_trans);
}
/*
@@ -2110,10 +2111,10 @@ xlog_print_tic_res(
*/
void
xlog_print_trans(
- struct xfs_trans *tp)
+ struct xfs_trans *tp)
{
- struct xfs_mount *mp = tp->t_mountp;
- struct xfs_log_item_desc *lidp;
+ struct xfs_mount *mp = tp->t_mountp;
+ struct xfs_log_item *lip;
/* dump core transaction and ticket info */
xfs_warn(mp, "transaction summary:");
@@ -2124,8 +2125,7 @@ xlog_print_trans(
xlog_print_tic_res(mp, tp->t_ticket);
/* dump each log item */
- list_for_each_entry(lidp, &tp->t_items, lid_trans) {
- struct xfs_log_item *lip = lidp->lid_item;
+ list_for_each_entry(lip, &tp->t_items, li_trans) {
struct xfs_log_vec *lv = lip->li_lv;
struct xfs_log_iovec *vec;
int i;
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index a8675c631438..c15687724728 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -141,10 +141,9 @@ xlog_cil_alloc_shadow_bufs(
struct xlog *log,
struct xfs_trans *tp)
{
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
- list_for_each_entry(lidp, &tp->t_items, lid_trans) {
- struct xfs_log_item *lip = lidp->lid_item;
+ list_for_each_entry(lip, &tp->t_items, li_trans) {
struct xfs_log_vec *lv;
int niovecs = 0;
int nbytes = 0;
@@ -152,7 +151,7 @@ xlog_cil_alloc_shadow_bufs(
bool ordered = false;
/* Skip items which aren't dirty in this transaction. */
- if (!(lidp->lid_flags & XFS_LID_DIRTY))
+ if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
continue;
/* get number of vecs and size of data to be stored */
@@ -317,7 +316,7 @@ xlog_cil_insert_format_items(
int *diff_len,
int *diff_iovecs)
{
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
/* Bail out if we didn't find a log item. */
@@ -326,15 +325,14 @@ xlog_cil_insert_format_items(
return;
}
- list_for_each_entry(lidp, &tp->t_items, lid_trans) {
- struct xfs_log_item *lip = lidp->lid_item;
+ list_for_each_entry(lip, &tp->t_items, li_trans) {
struct xfs_log_vec *lv;
struct xfs_log_vec *old_lv = NULL;
struct xfs_log_vec *shadow;
bool ordered = false;
/* Skip items which aren't dirty in this transaction. */
- if (!(lidp->lid_flags & XFS_LID_DIRTY))
+ if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
continue;
/*
@@ -406,7 +404,7 @@ xlog_cil_insert_items(
{
struct xfs_cil *cil = log->l_cilp;
struct xfs_cil_ctx *ctx = cil->xc_ctx;
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
int len = 0;
int diff_iovecs = 0;
int iclog_space;
@@ -479,11 +477,10 @@ xlog_cil_insert_items(
* We do this here so we only need to take the CIL lock once during
* the transaction commit.
*/
- list_for_each_entry(lidp, &tp->t_items, lid_trans) {
- struct xfs_log_item *lip = lidp->lid_item;
+ list_for_each_entry(lip, &tp->t_items, li_trans) {
/* Skip items which aren't dirty in this transaction. */
- if (!(lidp->lid_flags & XFS_LID_DIRTY))
+ if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
continue;
/*
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d71424052917..5726ef496980 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1880,11 +1880,6 @@ xfs_init_zones(void)
if (!xfs_trans_zone)
goto out_destroy_ifork_zone;
- xfs_log_item_desc_zone =
- kmem_zone_init(sizeof(struct xfs_log_item_desc),
- "xfs_log_item_desc");
- if (!xfs_log_item_desc_zone)
- goto out_destroy_trans_zone;
/*
* The size of the zone allocated buf log item is the maximum
@@ -1894,7 +1889,7 @@ xfs_init_zones(void)
xfs_buf_item_zone = kmem_zone_init(sizeof(struct xfs_buf_log_item),
"xfs_buf_item");
if (!xfs_buf_item_zone)
- goto out_destroy_log_item_desc_zone;
+ goto out_destroy_trans_zone;
xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
((XFS_EFD_MAX_FAST_EXTENTS - 1) *
@@ -1982,8 +1977,6 @@ xfs_init_zones(void)
kmem_zone_destroy(xfs_efd_zone);
out_destroy_buf_item_zone:
kmem_zone_destroy(xfs_buf_item_zone);
- out_destroy_log_item_desc_zone:
- kmem_zone_destroy(xfs_log_item_desc_zone);
out_destroy_trans_zone:
kmem_zone_destroy(xfs_trans_zone);
out_destroy_ifork_zone:
@@ -2022,7 +2015,6 @@ xfs_destroy_zones(void)
kmem_zone_destroy(xfs_efi_zone);
kmem_zone_destroy(xfs_efd_zone);
kmem_zone_destroy(xfs_buf_item_zone);
- kmem_zone_destroy(xfs_log_item_desc_zone);
kmem_zone_destroy(xfs_trans_zone);
kmem_zone_destroy(xfs_ifork_zone);
kmem_zone_destroy(xfs_da_state_zone);
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 947ab6e32c18..28a5b3f876ad 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -441,7 +441,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
__field(unsigned, bli_recur)
__field(int, bli_refcount)
__field(unsigned, bli_flags)
- __field(void *, li_desc)
__field(unsigned long, li_flags)
),
TP_fast_assign(
@@ -455,12 +454,11 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
__entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
__entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
__entry->buf_lockval = bip->bli_buf->b_sema.count;
- __entry->li_desc = bip->bli_item.li_desc;
__entry->li_flags = bip->bli_item.li_flags;
),
TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
"lock %d flags %s recur %d refcount %d bliflags %s "
- "lidesc %p liflags %s",
+ "liflags %s",
MAJOR(__entry->dev), MINOR(__entry->dev),
(unsigned long long)__entry->buf_bno,
__entry->buf_len,
@@ -471,7 +469,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
__entry->bli_recur,
__entry->bli_refcount,
__print_flags(__entry->bli_flags, "|", XFS_BLI_FLAGS),
- __entry->li_desc,
__print_flags(__entry->li_flags, "|", XFS_LI_FLAGS))
)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 24744915357b..477c12412d31 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -33,7 +33,6 @@
#include "xfs_error.h"
kmem_zone_t *xfs_trans_zone;
-kmem_zone_t *xfs_log_item_desc_zone;
#if defined(CONFIG_TRACEPOINTS)
static void
@@ -732,77 +731,52 @@ xfs_trans_unreserve_and_mod_sb(
return;
}
-/*
- * Add the given log item to the transaction's list of log items.
- *
- * The log item will now point to its new descriptor with its li_desc field.
- */
+/* Add the given log item to the transaction's list of log items. */
void
xfs_trans_add_item(
struct xfs_trans *tp,
struct xfs_log_item *lip)
{
- struct xfs_log_item_desc *lidp;
-
ASSERT(lip->li_mountp == tp->t_mountp);
ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
+ ASSERT(list_empty(&lip->li_trans));
+ ASSERT(!test_bit(XFS_LI_DIRTY, &lip->li_flags));
- lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
-
- lidp->lid_item = lip;
- lidp->lid_flags = 0;
- list_add_tail(&lidp->lid_trans, &tp->t_items);
-
- lip->li_desc = lidp;
-
+ list_add_tail(&lip->li_trans, &tp->t_items);
trace_xfs_trans_add_item(tp, _RET_IP_);
}
-STATIC void
-xfs_trans_free_item_desc(
- struct xfs_log_item_desc *lidp)
-{
- list_del_init(&lidp->lid_trans);
- kmem_zone_free(xfs_log_item_desc_zone, lidp);
-}
-
/*
- * Unlink and free the given descriptor.
+ * Unlink the log item from the transaction. the log item is no longer
+ * considered dirty in this transaction, as the linked transaction has
+ * finished, either by abort or commit completion.
*/
void
xfs_trans_del_item(
struct xfs_log_item *lip)
{
- xfs_trans_free_item_desc(lip->li_desc);
- lip->li_desc = NULL;
+ clear_bit(XFS_LI_DIRTY, &lip->li_flags);
+ list_del_init(&lip->li_trans);
}
-/*
- * Unlock all of the items of a transaction and free all the descriptors
- * of that transaction.
- */
+/* Detach and unlock all of the items in a transaction */
void
xfs_trans_free_items(
struct xfs_trans *tp,
xfs_lsn_t commit_lsn,
bool abort)
{
- struct xfs_log_item_desc *lidp, *next;
+ struct xfs_log_item *lip, *next;
trace_xfs_trans_free_items(tp, _RET_IP_);
- list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
- struct xfs_log_item *lip = lidp->lid_item;
-
- lip->li_desc = NULL;
-
+ list_for_each_entry_safe(lip, next, &tp->t_items, li_trans) {
+ xfs_trans_del_item(lip);
if (commit_lsn != NULLCOMMITLSN)
lip->li_ops->iop_committing(lip, commit_lsn);
if (abort)
set_bit(XFS_LI_ABORTED, &lip->li_flags);
lip->li_ops->iop_unlock(lip);
-
- xfs_trans_free_item_desc(lidp);
}
}
@@ -1047,10 +1021,10 @@ xfs_trans_cancel(
}
#ifdef DEBUG
if (!dirty && !XFS_FORCED_SHUTDOWN(mp)) {
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
- list_for_each_entry(lidp, &tp->t_items, lid_trans)
- ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
+ list_for_each_entry(lip, &tp->t_items, li_trans)
+ ASSERT(!(lip->li_type == XFS_LI_EFD));
}
#endif
xfs_trans_unreserve_and_mod_sb(tp);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 51145ddf7e5b..a32da7d7ea97 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -27,7 +27,6 @@ struct xfs_efi_log_item;
struct xfs_inode;
struct xfs_item_ops;
struct xfs_log_iovec;
-struct xfs_log_item_desc;
struct xfs_mount;
struct xfs_trans;
struct xfs_trans_res;
@@ -43,8 +42,8 @@ struct xfs_bud_log_item;
typedef struct xfs_log_item {
struct list_head li_ail; /* AIL pointers */
+ struct list_head li_trans; /* transaction list */
xfs_lsn_t li_lsn; /* last on-disk lsn */
- struct xfs_log_item_desc *li_desc; /* ptr to current desc*/
struct xfs_mount *li_mountp; /* ptr to fs mount */
struct xfs_ail *li_ailp; /* ptr to AIL */
uint li_type; /* item type */
@@ -72,11 +71,13 @@ typedef struct xfs_log_item {
#define XFS_LI_IN_AIL 0
#define XFS_LI_ABORTED 1
#define XFS_LI_FAILED 2
+#define XFS_LI_DIRTY 3 /* log item dirty in transaction */
#define XFS_LI_FLAGS \
{ (1 << XFS_LI_IN_AIL), "IN_AIL" }, \
{ (1 << XFS_LI_ABORTED), "ABORTED" }, \
- { (1 << XFS_LI_FAILED), "FAILED" }
+ { (1 << XFS_LI_FAILED), "FAILED" }, \
+ { (1 << XFS_LI_DIRTY), "DIRTY" }
struct xfs_item_ops {
void (*iop_size)(xfs_log_item_t *, int *, int *);
@@ -247,7 +248,6 @@ void xfs_trans_buf_copy_type(struct xfs_buf *dst_bp,
struct xfs_buf *src_bp);
extern kmem_zone_t *xfs_trans_zone;
-extern kmem_zone_t *xfs_log_item_desc_zone;
/* rmap updates */
enum xfs_rmap_intent_type;
diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
index 14543d93cd4b..230a21df4b12 100644
--- a/fs/xfs/xfs_trans_bmap.c
+++ b/fs/xfs/xfs_trans_bmap.c
@@ -79,7 +79,7 @@ xfs_trans_log_finish_bmap_update(
* 2.) shuts down the filesystem
*/
tp->t_flags |= XFS_TRANS_DIRTY;
- budp->bud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
return error;
}
@@ -158,7 +158,7 @@ xfs_bmap_update_log_item(
bmap = container_of(item, struct xfs_bmap_intent, bi_list);
tp->t_flags |= XFS_TRANS_DIRTY;
- buip->bui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
/*
* atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 0081e9b3decf..a8ddb4eed279 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -40,7 +40,7 @@ xfs_trans_buf_item_match(
struct xfs_buf_map *map,
int nmaps)
{
- struct xfs_log_item_desc *lidp;
+ struct xfs_log_item *lip;
struct xfs_buf_log_item *blip;
int len = 0;
int i;
@@ -48,8 +48,8 @@ xfs_trans_buf_item_match(
for (i = 0; i < nmaps; i++)
len += map[i].bm_len;
- list_for_each_entry(lidp, &tp->t_items, lid_trans) {
- blip = (struct xfs_buf_log_item *)lidp->lid_item;
+ list_for_each_entry(lip, &tp->t_items, li_trans) {
+ blip = (struct xfs_buf_log_item *)lip;
if (blip->bli_item.li_type == XFS_LI_BUF &&
blip->bli_buf->b_target == target &&
XFS_BUF_ADDR(blip->bli_buf) == map[0].bm_bn &&
@@ -100,14 +100,10 @@ _xfs_trans_bjoin(
atomic_inc(&bip->bli_refcount);
/*
- * Get a log_item_desc to point at the new item.
+ * Attach the item to the transaction so we can find it in
+ * xfs_trans_get_buf() and friends.
*/
xfs_trans_add_item(tp, &bip->bli_item);
-
- /*
- * Initialize b_fsprivate2 so we can find it with incore_match()
- * in xfs_trans_get_buf() and friends above.
- */
bp->b_transp = tp;
}
@@ -391,7 +387,7 @@ xfs_trans_brelse(
* If the buffer is dirty within this transaction, we can't
* release it until we commit.
*/
- if (bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY)
+ if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
return;
/*
@@ -542,7 +538,7 @@ xfs_trans_dirty_buf(
bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
tp->t_flags |= XFS_TRANS_DIRTY;
- bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
}
/*
@@ -626,7 +622,7 @@ xfs_trans_binval(
ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
ASSERT(!(bip->__bli_format.blf_flags & XFS_BLFT_MASK));
ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
- ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
+ ASSERT(test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags));
ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
return;
}
@@ -642,7 +638,7 @@ xfs_trans_binval(
memset(bip->bli_formats[i].blf_data_map, 0,
(bip->bli_formats[i].blf_map_size * sizeof(uint)));
}
- bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
tp->t_flags |= XFS_TRANS_DIRTY;
}
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index c3d547211d16..c381c02cca45 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -77,7 +77,7 @@ xfs_trans_log_dquot(
ASSERT(XFS_DQ_IS_LOCKED(dqp));
tp->t_flags |= XFS_TRANS_DIRTY;
- dqp->q_logitem.qli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &dqp->q_logitem.qli_item.li_flags);
}
/*
@@ -879,7 +879,7 @@ xfs_trans_log_quotaoff_item(
xfs_qoff_logitem_t *qlp)
{
tp->t_flags |= XFS_TRANS_DIRTY;
- qlp->qql_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags);
}
STATIC void
diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
index ab438647592a..fb631081d829 100644
--- a/fs/xfs/xfs_trans_extfree.c
+++ b/fs/xfs/xfs_trans_extfree.c
@@ -90,7 +90,7 @@ xfs_trans_free_extent(
* 2.) shuts down the filesystem
*/
tp->t_flags |= XFS_TRANS_DIRTY;
- efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
next_extent = efdp->efd_next_extent;
ASSERT(next_extent < efdp->efd_format.efd_nextents);
@@ -155,7 +155,7 @@ xfs_extent_free_log_item(
free = container_of(item, struct xfs_extent_free_item, xefi_list);
tp->t_flags |= XFS_TRANS_DIRTY;
- efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
/*
* atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
index 07cea592dc01..f7bd7960a90f 100644
--- a/fs/xfs/xfs_trans_inode.c
+++ b/fs/xfs/xfs_trans_inode.c
@@ -133,14 +133,13 @@ xfs_trans_log_inode(
* set however, then go ahead and bump the i_version counter
* unconditionally.
*/
- if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) &&
+ if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
IS_I_VERSION(VFS_I(ip))) {
if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
flags |= XFS_ILOG_CORE;
}
tp->t_flags |= XFS_TRANS_DIRTY;
- ip->i_itemp->ili_item.li_desc->lid_flags |= XFS_LID_DIRTY;
/*
* Always OR in the bits from the ili_last_fields field.
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 43f773297b9d..9717ae74b36d 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -19,7 +19,6 @@
#define __XFS_TRANS_PRIV_H__
struct xfs_log_item;
-struct xfs_log_item_desc;
struct xfs_mount;
struct xfs_trans;
struct xfs_ail;
diff --git a/fs/xfs/xfs_trans_refcount.c b/fs/xfs/xfs_trans_refcount.c
index 94c1877af834..c7f8e82f5bda 100644
--- a/fs/xfs/xfs_trans_refcount.c
+++ b/fs/xfs/xfs_trans_refcount.c
@@ -77,7 +77,7 @@ xfs_trans_log_finish_refcount_update(
* 2.) shuts down the filesystem
*/
tp->t_flags |= XFS_TRANS_DIRTY;
- cudp->cud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &cudp->cud_item.li_flags);
return error;
}
@@ -154,7 +154,7 @@ xfs_refcount_update_log_item(
refc = container_of(item, struct xfs_refcount_intent, ri_list);
tp->t_flags |= XFS_TRANS_DIRTY;
- cuip->cui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
/*
* atomic_inc_return gives us the value after the increment;
diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
index 9b577beb43d7..5831ca0c270b 100644
--- a/fs/xfs/xfs_trans_rmap.c
+++ b/fs/xfs/xfs_trans_rmap.c
@@ -117,7 +117,7 @@ xfs_trans_log_finish_rmap_update(
* 2.) shuts down the filesystem
*/
tp->t_flags |= XFS_TRANS_DIRTY;
- rudp->rud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &rudp->rud_item.li_flags);
return error;
}
@@ -175,7 +175,7 @@ xfs_rmap_update_log_item(
rmap = container_of(item, struct xfs_rmap_intent, ri_list);
tp->t_flags |= XFS_TRANS_DIRTY;
- ruip->rui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+ set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
/*
* atomic_inc_return gives us the value after the increment;
--
2.17.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 4/9] xfs: don't assert fail with AIL lock held
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 6:13 ` Christoph Hellwig
2018-05-09 14:52 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:57PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Been hitting AIL ordering assert failures recently, but been unable
> to trace them down because the system immediately hangs up onteh
> spinlock that was held when this assert fires:
>
> XFS: Assertion failed: XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0, file: fs/xfs/xfs_trans_ail.c, line: 52
>
> Move the assertions outside of the spinlock so the corpse can
> be dissected. Thanks to Brian Foster for supplying a clean
> way of doing this.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_trans_ail.c | 43 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 50611d2bcbc2..41e280ef1483 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -32,30 +32,51 @@
> #ifdef DEBUG
> /*
> * Check that the list is sorted as it should be.
> + *
> + * Called with the ail lock held, but we don't want to assert fail with it
> + * held otherwise we'll lock everything up and won't be able to debug the
> + * cause. Hence we sample and check the state under the AIL lock and return if
> + * everything is fine, otherwise we drop the lock and run the ASSERT checks.
> + * Asserts may not be fatal, so pick the lock back up and continue onwards.
> */
> STATIC void
> xfs_ail_check(
> - struct xfs_ail *ailp,
> - xfs_log_item_t *lip)
> + struct xfs_ail *ailp,
> + struct xfs_log_item *lip)
> {
> - xfs_log_item_t *prev_lip;
> + struct xfs_log_item *prev_lip;
> + struct xfs_log_item *next_lip;
> + xfs_lsn_t prev_lsn = NULLCOMMITLSN;
> + xfs_lsn_t next_lsn = NULLCOMMITLSN;
> + xfs_lsn_t lsn;
> + bool in_ail;
> +
>
> if (list_empty(&ailp->ail_head))
> return;
>
> /*
> - * Check the next and previous entries are valid.
> + * Sample then check the next and previous entries are valid.
> */
> - ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
> - prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
> + in_ail = test_bit(XFS_LI_IN_AIL, &lip->li_flags);
> + prev_lip = list_entry(lip->li_ail.prev, struct xfs_log_item, li_ail);
> if (&prev_lip->li_ail != &ailp->ail_head)
> - ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> -
> - prev_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
> - if (&prev_lip->li_ail != &ailp->ail_head)
> - ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) >= 0);
> + prev_lsn = prev_lip->li_lsn;
> + next_lip = list_entry(lip->li_ail.next, struct xfs_log_item, li_ail);
> + if (&next_lip->li_ail != &ailp->ail_head)
> + next_lsn = next_lip->li_lsn;
> + lsn = lip->li_lsn;
>
> + if (in_ail &&
> + (prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0) &&
> + (next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0))
> + return;
>
> + spin_unlock(&ailp->ail_lock);
> + ASSERT(in_ail);
> + ASSERT(prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0);
> + ASSERT(next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0);
> + spin_lock(&ailp->ail_lock);
> }
> #else /* !DEBUG */
> #define xfs_ail_check(a,l)
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-08 3:41 ` [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt() Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 0:24 ` Dave Chinner
0 siblings, 1 reply; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> into a transaction it is already joined to. This means the inode can
> have multiple log item descriptors attached to the transaction for
> it. This breaks teh 1:1 mapping that is supposed to exist
> between the log item and log item descriptor.
>
> This results in the log item being processed twice during
> transaction commit and CIL formatting, and there are lots of other
> potential issues tha arise from double processing of log items in
> the transaction commit state machine.
>
> In this case, the inode is already held by the rolling transaction
> returned from xfs_defer_finish(), so there's no need to join it
> again.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_symlink.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 5b66ac12913c..27870e5cd259 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> error = xfs_defer_finish(&tp, &dfops);
> if (error)
> goto error_bmap_cancel;
> - /*
> - * The first xact was committed, so add the inode to the new one.
> - * Mark it dirty so it will be logged and moved forward in the log as
> - * part of every commit.
> - */
> - xfs_trans_ijoin(tp, ip, 0);
> - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
> /*
> * Commit the transaction containing extent freeing and EFDs.
> */
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
Seems fine.. but do we even need this call? We're about to commit the
transaction and unlock the inode...
Brian
> error = xfs_trans_commit(tp);
> if (error) {
> ASSERT(XFS_FORCED_SHUTDOWN(mp));
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
2018-05-08 3:41 ` [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 15:17 ` Darrick J. Wong
1 sibling, 0 replies; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:59PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> AN inode is joined to teh same transaction twice in
> xfs_reflink_cancel_cow_range() resulting in the following assert
> failure:
>
> [ 30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> [ 30.183435] ------------[ cut here ]------------
This assert report is kind of irrelevant if the whole LI_TRANS thing was
dropped. Might as well just simplify or generalize the commit log. With
that, looks fine:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> ......
> [ 30.209264] Call Trace:
> [ 30.209935] xfs_trans_add_item+0xcc/0xe0
> [ 30.210968] xfs_reflink_cancel_cow_blocks+0xab/0x290
> [ 30.212249] ? xfs_trans_reserve+0x1b4/0x2b0
> [ 30.213320] ? kmem_zone_alloc+0x61/0xe0
> [ 30.214321] xfs_reflink_cancel_cow_range+0xb2/0x1f0
> [ 30.215616] xfs_fs_destroy_inode+0x1bd/0x280
> [ 30.216757] dispose_list+0x35/0x40
> [ 30.217656] evict_inodes+0x132/0x160
> [ 30.218620] generic_shutdown_super+0x3a/0x110
> [ 30.219771] kill_block_super+0x21/0x50
> [ 30.220762] deactivate_locked_super+0x39/0x70
> [ 30.221909] cleanup_mnt+0x3b/0x70
> [ 30.222819] task_work_run+0x7f/0xa0
> [ 30.223762] exit_to_usermode_loop+0x9b/0xa0
> [ 30.224884] do_syscall_64+0x18f/0x1a0
>
> Fix it and document that the callers of
> xfs_reflink_cancel_cow_blocks() must have already joined the inode
> to the permanent transaction passed in.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_reflink.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index cdbd342a5249..bce2b5351d64 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
> *
> * If cancel_real is true this function cancels all COW fork extents for the
> * inode; if cancel_real is false, real extents are not cleared.
> + *
> + * Caller must have already joined the inode to the current transaction. The
> + * inode will be joined to the transaction returned to the caller.
> */
> int
> xfs_reflink_cancel_cow_blocks(
> @@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
> if (error)
> break;
> } else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
> - xfs_trans_ijoin(*tpp, ip, 0);
> xfs_defer_init(&dfops, &firstfsb);
>
> /* Free the CoW orphan record. */
> @@ -1570,6 +1572,7 @@ xfs_reflink_clear_inode_flag(
> * We didn't find any shared blocks so turn off the reflink flag.
> * First, get rid of any leftover CoW mappings.
> */
> + xfs_trans_ijoin(*tpp, ip, 0);
> error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
> if (error)
> return error;
> @@ -1578,7 +1581,6 @@ xfs_reflink_clear_inode_flag(
> trace_xfs_reflink_unset_inode_flag(ip);
> ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> xfs_inode_clear_cowblocks_tag(ip);
> - xfs_trans_ijoin(*tpp, ip, 0);
> xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
>
> return error;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
2018-05-08 3:42 ` [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag() Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 0:40 ` Dave Chinner
0 siblings, 1 reply; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:42:00PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Another assert failure:
>
> XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
Same assert comment...
> ....
> xfs_trans_add_item+0xcc/0xe0
> xfs_reflink_clear_inode_flag+0x53/0x120
> xfs_reflink_try_clear_inode_flag+0x5b/0xa0
> ? filemap_write_and_wait+0x4f/0x70
> xfs_reflink_unshare+0x18e/0x19d
> xfs_file_fallocate+0x241/0x310
> ? selinux_file_permission+0xd4/0x140
> vfs_fallocate+0x13d/0x260
> SyS_fallocate+0x43/0x80
>
> Another fix.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_reflink.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index bce2b5351d64..12d441a73b53 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
> return 0;
> }
>
> -/* Clear the inode reflink flag if there are no shared extents. */
> +/*
> + * Clear the inode reflink flag if there are no shared extents.
> + *
> + * The caller is responsible for joining the inode to the transaction passed in.
> + * The inode will be joined to the transaction that is returned to the caller.
> + */
> int
> xfs_reflink_clear_inode_flag(
> struct xfs_inode *ip,
> @@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
> * We didn't find any shared blocks so turn off the reflink flag.
> * First, get rid of any leftover CoW mappings.
> */
> - xfs_trans_ijoin(*tpp, ip, 0);
Wasn't this just added in the previous patch? This seems a bit
superfluous. If the inode was already joined by the one caller of this
function, why not just remove this call in the previous patch rather
than move it and remove it?
Brian
> error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
> if (error)
> return error;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse
2018-05-08 3:42 ` [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 15:19 ` Darrick J. Wong
1 sibling, 0 replies; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:42:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Just to make sure the item isn't associated with another
> transaction when we try to reuse it.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_buf_item.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index df62082f2204..8d6ed045b643 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -745,8 +745,10 @@ xfs_buf_item_init(
> * nothing to do here so return.
> */
> ASSERT(bp->b_target->bt_mount == mp);
> - if (bip != NULL) {
> + if (bip) {
> ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> + ASSERT(!bp->b_transp);
> + ASSERT(bip->bli_buf == bp);
> return 0;
> }
>
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] xfs: get rid of the log item descriptor
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
@ 2018-05-08 14:18 ` Brian Foster
2018-05-09 6:27 ` Christoph Hellwig
2018-05-09 15:19 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Brian Foster @ 2018-05-08 14:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:42:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> It's just a connector between a transaction and a log item. There's
> a 1:1 relationship between a log item descriptor and a log item,
> and a 1:1 relationship between a log item descriptor and a
> transaction. Both relationships are created and terminated at the
> same time, so why do we even have the descriptor?
>
> Replace it with a specific list_head in the log item and a new
> log item dirtied flag to replace the XFS_LID_DIRTY flag.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_bmap.c | 8 ++---
> fs/xfs/libxfs/xfs_shared.h | 15 ----------
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_icreate_item.c | 2 +-
> fs/xfs/xfs_log.c | 10 +++----
> fs/xfs/xfs_log_cil.c | 21 ++++++--------
> fs/xfs/xfs_super.c | 10 +------
> fs/xfs/xfs_trace.h | 5 +---
> fs/xfs/xfs_trans.c | 58 ++++++++++---------------------------
> fs/xfs/xfs_trans.h | 8 ++---
> fs/xfs/xfs_trans_bmap.c | 4 +--
> fs/xfs/xfs_trans_buf.c | 22 ++++++--------
> fs/xfs/xfs_trans_dquot.c | 4 +--
> fs/xfs/xfs_trans_extfree.c | 4 +--
> fs/xfs/xfs_trans_inode.c | 3 +-
> fs/xfs/xfs_trans_priv.h | 1 -
> fs/xfs/xfs_trans_refcount.c | 4 +--
> fs/xfs/xfs_trans_rmap.c | 4 +--
> 18 files changed, 62 insertions(+), 123 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 040eeda8426f..ab90998c1867 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -246,7 +246,7 @@ xfs_bmap_get_bp(
> struct xfs_btree_cur *cur,
> xfs_fsblock_t bno)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> int i;
>
> if (!cur)
> @@ -260,9 +260,9 @@ xfs_bmap_get_bp(
> }
>
> /* Chase down all the log items to see if the bp is there */
> - list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
> - struct xfs_buf_log_item *bip;
> - bip = (struct xfs_buf_log_item *)lidp->lid_item;
> + list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
> + struct xfs_buf_log_item *bip = (struct xfs_buf_log_item *)lip;
> +
> if (bip->bli_item.li_type == XFS_LI_BUF &&
> XFS_BUF_ADDR(bip->bli_buf) == bno)
> return bip->bli_buf;
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index d0b84da0cb1e..8efc06e62b13 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -57,21 +57,6 @@ extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
> extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> extern const struct xfs_buf_ops xfs_rtbuf_ops;
>
> -/*
> - * This structure is used to track log items associated with
> - * a transaction. It points to the log item and keeps some
> - * flags to track the state of the log item. It also tracks
> - * the amount of space needed to log the item it describes
> - * once we get to commit processing (see xfs_trans_commit()).
> - */
> -struct xfs_log_item_desc {
> - struct xfs_log_item *lid_item;
> - struct list_head lid_trans;
> - unsigned char lid_flags;
> -};
> -
> -#define XFS_LID_DIRTY 0x1
> -
> /* log size calculation functions */
> int xfs_log_calc_unit_res(struct xfs_mount *mp, int unit_bytes);
> int xfs_log_calc_minimum_size(struct xfs_mount *);
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 8d6ed045b643..c2311379d1c3 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -438,7 +438,7 @@ xfs_buf_item_unpin(
> * xfs_trans_uncommit() will try to reference the
> * buffer which we no longer have a hold on.
> */
> - if (lip->li_desc)
> + if (!list_empty(&lip->li_trans))
> xfs_trans_del_item(lip);
>
> /*
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index a99a0f8aa528..5da9599156ed 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -184,5 +184,5 @@ xfs_icreate_log(
>
> xfs_trans_add_item(tp, &icp->ic_item);
> tp->t_flags |= XFS_TRANS_DIRTY;
> - icp->ic_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
> }
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e427864434c1..c21039f27e39 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1047,6 +1047,7 @@ xfs_log_item_init(
> INIT_LIST_HEAD(&item->li_ail);
> INIT_LIST_HEAD(&item->li_cil);
> INIT_LIST_HEAD(&item->li_bio_list);
> + INIT_LIST_HEAD(&item->li_trans);
> }
>
> /*
> @@ -2110,10 +2111,10 @@ xlog_print_tic_res(
> */
> void
> xlog_print_trans(
> - struct xfs_trans *tp)
> + struct xfs_trans *tp)
> {
> - struct xfs_mount *mp = tp->t_mountp;
> - struct xfs_log_item_desc *lidp;
> + struct xfs_mount *mp = tp->t_mountp;
> + struct xfs_log_item *lip;
>
> /* dump core transaction and ticket info */
> xfs_warn(mp, "transaction summary:");
> @@ -2124,8 +2125,7 @@ xlog_print_trans(
> xlog_print_tic_res(mp, tp->t_ticket);
>
> /* dump each log item */
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv = lip->li_lv;
> struct xfs_log_iovec *vec;
> int i;
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index a8675c631438..c15687724728 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -141,10 +141,9 @@ xlog_cil_alloc_shadow_bufs(
> struct xlog *log,
> struct xfs_trans *tp)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv;
> int niovecs = 0;
> int nbytes = 0;
> @@ -152,7 +151,7 @@ xlog_cil_alloc_shadow_bufs(
> bool ordered = false;
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /* get number of vecs and size of data to be stored */
> @@ -317,7 +316,7 @@ xlog_cil_insert_format_items(
> int *diff_len,
> int *diff_iovecs)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
>
> /* Bail out if we didn't find a log item. */
> @@ -326,15 +325,14 @@ xlog_cil_insert_format_items(
> return;
> }
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv;
> struct xfs_log_vec *old_lv = NULL;
> struct xfs_log_vec *shadow;
> bool ordered = false;
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /*
> @@ -406,7 +404,7 @@ xlog_cil_insert_items(
> {
> struct xfs_cil *cil = log->l_cilp;
> struct xfs_cil_ctx *ctx = cil->xc_ctx;
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> int len = 0;
> int diff_iovecs = 0;
> int iclog_space;
> @@ -479,11 +477,10 @@ xlog_cil_insert_items(
> * We do this here so we only need to take the CIL lock once during
> * the transaction commit.
> */
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /*
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d71424052917..5726ef496980 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1880,11 +1880,6 @@ xfs_init_zones(void)
> if (!xfs_trans_zone)
> goto out_destroy_ifork_zone;
>
> - xfs_log_item_desc_zone =
> - kmem_zone_init(sizeof(struct xfs_log_item_desc),
> - "xfs_log_item_desc");
> - if (!xfs_log_item_desc_zone)
> - goto out_destroy_trans_zone;
>
> /*
> * The size of the zone allocated buf log item is the maximum
> @@ -1894,7 +1889,7 @@ xfs_init_zones(void)
> xfs_buf_item_zone = kmem_zone_init(sizeof(struct xfs_buf_log_item),
> "xfs_buf_item");
> if (!xfs_buf_item_zone)
> - goto out_destroy_log_item_desc_zone;
> + goto out_destroy_trans_zone;
>
> xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
> ((XFS_EFD_MAX_FAST_EXTENTS - 1) *
> @@ -1982,8 +1977,6 @@ xfs_init_zones(void)
> kmem_zone_destroy(xfs_efd_zone);
> out_destroy_buf_item_zone:
> kmem_zone_destroy(xfs_buf_item_zone);
> - out_destroy_log_item_desc_zone:
> - kmem_zone_destroy(xfs_log_item_desc_zone);
> out_destroy_trans_zone:
> kmem_zone_destroy(xfs_trans_zone);
> out_destroy_ifork_zone:
> @@ -2022,7 +2015,6 @@ xfs_destroy_zones(void)
> kmem_zone_destroy(xfs_efi_zone);
> kmem_zone_destroy(xfs_efd_zone);
> kmem_zone_destroy(xfs_buf_item_zone);
> - kmem_zone_destroy(xfs_log_item_desc_zone);
> kmem_zone_destroy(xfs_trans_zone);
> kmem_zone_destroy(xfs_ifork_zone);
> kmem_zone_destroy(xfs_da_state_zone);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 947ab6e32c18..28a5b3f876ad 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -441,7 +441,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __field(unsigned, bli_recur)
> __field(int, bli_refcount)
> __field(unsigned, bli_flags)
> - __field(void *, li_desc)
> __field(unsigned long, li_flags)
> ),
> TP_fast_assign(
> @@ -455,12 +454,11 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
> __entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
> __entry->buf_lockval = bip->bli_buf->b_sema.count;
> - __entry->li_desc = bip->bli_item.li_desc;
> __entry->li_flags = bip->bli_item.li_flags;
> ),
> TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
> "lock %d flags %s recur %d refcount %d bliflags %s "
> - "lidesc %p liflags %s",
> + "liflags %s",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> (unsigned long long)__entry->buf_bno,
> __entry->buf_len,
> @@ -471,7 +469,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __entry->bli_recur,
> __entry->bli_refcount,
> __print_flags(__entry->bli_flags, "|", XFS_BLI_FLAGS),
> - __entry->li_desc,
> __print_flags(__entry->li_flags, "|", XFS_LI_FLAGS))
> )
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 24744915357b..477c12412d31 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -33,7 +33,6 @@
> #include "xfs_error.h"
>
> kmem_zone_t *xfs_trans_zone;
> -kmem_zone_t *xfs_log_item_desc_zone;
>
> #if defined(CONFIG_TRACEPOINTS)
> static void
> @@ -732,77 +731,52 @@ xfs_trans_unreserve_and_mod_sb(
> return;
> }
>
> -/*
> - * Add the given log item to the transaction's list of log items.
> - *
> - * The log item will now point to its new descriptor with its li_desc field.
> - */
> +/* Add the given log item to the transaction's list of log items. */
> void
> xfs_trans_add_item(
> struct xfs_trans *tp,
> struct xfs_log_item *lip)
> {
> - struct xfs_log_item_desc *lidp;
> -
> ASSERT(lip->li_mountp == tp->t_mountp);
> ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> + ASSERT(list_empty(&lip->li_trans));
> + ASSERT(!test_bit(XFS_LI_DIRTY, &lip->li_flags));
>
> - lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
> -
> - lidp->lid_item = lip;
> - lidp->lid_flags = 0;
> - list_add_tail(&lidp->lid_trans, &tp->t_items);
> -
> - lip->li_desc = lidp;
> -
> + list_add_tail(&lip->li_trans, &tp->t_items);
> trace_xfs_trans_add_item(tp, _RET_IP_);
> }
>
> -STATIC void
> -xfs_trans_free_item_desc(
> - struct xfs_log_item_desc *lidp)
> -{
> - list_del_init(&lidp->lid_trans);
> - kmem_zone_free(xfs_log_item_desc_zone, lidp);
> -}
> -
> /*
> - * Unlink and free the given descriptor.
> + * Unlink the log item from the transaction. the log item is no longer
> + * considered dirty in this transaction, as the linked transaction has
> + * finished, either by abort or commit completion.
> */
> void
> xfs_trans_del_item(
> struct xfs_log_item *lip)
> {
> - xfs_trans_free_item_desc(lip->li_desc);
> - lip->li_desc = NULL;
> + clear_bit(XFS_LI_DIRTY, &lip->li_flags);
> + list_del_init(&lip->li_trans);
> }
>
> -/*
> - * Unlock all of the items of a transaction and free all the descriptors
> - * of that transaction.
> - */
> +/* Detach and unlock all of the items in a transaction */
> void
> xfs_trans_free_items(
> struct xfs_trans *tp,
> xfs_lsn_t commit_lsn,
> bool abort)
> {
> - struct xfs_log_item_desc *lidp, *next;
> + struct xfs_log_item *lip, *next;
>
> trace_xfs_trans_free_items(tp, _RET_IP_);
>
> - list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> -
> - lip->li_desc = NULL;
> -
> + list_for_each_entry_safe(lip, next, &tp->t_items, li_trans) {
> + xfs_trans_del_item(lip);
> if (commit_lsn != NULLCOMMITLSN)
> lip->li_ops->iop_committing(lip, commit_lsn);
> if (abort)
> set_bit(XFS_LI_ABORTED, &lip->li_flags);
> lip->li_ops->iop_unlock(lip);
> -
> - xfs_trans_free_item_desc(lidp);
> }
> }
>
> @@ -1047,10 +1021,10 @@ xfs_trans_cancel(
> }
> #ifdef DEBUG
> if (!dirty && !XFS_FORCED_SHUTDOWN(mp)) {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans)
> - ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
> + list_for_each_entry(lip, &tp->t_items, li_trans)
> + ASSERT(!(lip->li_type == XFS_LI_EFD));
> }
> #endif
> xfs_trans_unreserve_and_mod_sb(tp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 51145ddf7e5b..a32da7d7ea97 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -27,7 +27,6 @@ struct xfs_efi_log_item;
> struct xfs_inode;
> struct xfs_item_ops;
> struct xfs_log_iovec;
> -struct xfs_log_item_desc;
> struct xfs_mount;
> struct xfs_trans;
> struct xfs_trans_res;
> @@ -43,8 +42,8 @@ struct xfs_bud_log_item;
>
> typedef struct xfs_log_item {
> struct list_head li_ail; /* AIL pointers */
> + struct list_head li_trans; /* transaction list */
> xfs_lsn_t li_lsn; /* last on-disk lsn */
> - struct xfs_log_item_desc *li_desc; /* ptr to current desc*/
> struct xfs_mount *li_mountp; /* ptr to fs mount */
> struct xfs_ail *li_ailp; /* ptr to AIL */
> uint li_type; /* item type */
> @@ -72,11 +71,13 @@ typedef struct xfs_log_item {
> #define XFS_LI_IN_AIL 0
> #define XFS_LI_ABORTED 1
> #define XFS_LI_FAILED 2
> +#define XFS_LI_DIRTY 3 /* log item dirty in transaction */
>
> #define XFS_LI_FLAGS \
> { (1 << XFS_LI_IN_AIL), "IN_AIL" }, \
> { (1 << XFS_LI_ABORTED), "ABORTED" }, \
> - { (1 << XFS_LI_FAILED), "FAILED" }
> + { (1 << XFS_LI_FAILED), "FAILED" }, \
> + { (1 << XFS_LI_DIRTY), "DIRTY" }
>
> struct xfs_item_ops {
> void (*iop_size)(xfs_log_item_t *, int *, int *);
> @@ -247,7 +248,6 @@ void xfs_trans_buf_copy_type(struct xfs_buf *dst_bp,
> struct xfs_buf *src_bp);
>
> extern kmem_zone_t *xfs_trans_zone;
> -extern kmem_zone_t *xfs_log_item_desc_zone;
>
> /* rmap updates */
> enum xfs_rmap_intent_type;
> diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
> index 14543d93cd4b..230a21df4b12 100644
> --- a/fs/xfs/xfs_trans_bmap.c
> +++ b/fs/xfs/xfs_trans_bmap.c
> @@ -79,7 +79,7 @@ xfs_trans_log_finish_bmap_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - budp->bud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
>
> return error;
> }
> @@ -158,7 +158,7 @@ xfs_bmap_update_log_item(
> bmap = container_of(item, struct xfs_bmap_intent, bi_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - buip->bui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 0081e9b3decf..a8ddb4eed279 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -40,7 +40,7 @@ xfs_trans_buf_item_match(
> struct xfs_buf_map *map,
> int nmaps)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> struct xfs_buf_log_item *blip;
> int len = 0;
> int i;
> @@ -48,8 +48,8 @@ xfs_trans_buf_item_match(
> for (i = 0; i < nmaps; i++)
> len += map[i].bm_len;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - blip = (struct xfs_buf_log_item *)lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> + blip = (struct xfs_buf_log_item *)lip;
> if (blip->bli_item.li_type == XFS_LI_BUF &&
> blip->bli_buf->b_target == target &&
> XFS_BUF_ADDR(blip->bli_buf) == map[0].bm_bn &&
> @@ -100,14 +100,10 @@ _xfs_trans_bjoin(
> atomic_inc(&bip->bli_refcount);
>
> /*
> - * Get a log_item_desc to point at the new item.
> + * Attach the item to the transaction so we can find it in
> + * xfs_trans_get_buf() and friends.
> */
> xfs_trans_add_item(tp, &bip->bli_item);
> -
> - /*
> - * Initialize b_fsprivate2 so we can find it with incore_match()
> - * in xfs_trans_get_buf() and friends above.
> - */
> bp->b_transp = tp;
>
> }
> @@ -391,7 +387,7 @@ xfs_trans_brelse(
> * If the buffer is dirty within this transaction, we can't
> * release it until we commit.
> */
> - if (bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY)
> + if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
> return;
>
> /*
> @@ -542,7 +538,7 @@ xfs_trans_dirty_buf(
> bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> }
>
> /*
> @@ -626,7 +622,7 @@ xfs_trans_binval(
> ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
> ASSERT(!(bip->__bli_format.blf_flags & XFS_BLFT_MASK));
> ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
> - ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
> + ASSERT(test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags));
> ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
> return;
> }
> @@ -642,7 +638,7 @@ xfs_trans_binval(
> memset(bip->bli_formats[i].blf_data_map, 0,
> (bip->bli_formats[i].blf_map_size * sizeof(uint)));
> }
> - bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> tp->t_flags |= XFS_TRANS_DIRTY;
> }
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index c3d547211d16..c381c02cca45 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -77,7 +77,7 @@ xfs_trans_log_dquot(
> ASSERT(XFS_DQ_IS_LOCKED(dqp));
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - dqp->q_logitem.qli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &dqp->q_logitem.qli_item.li_flags);
> }
>
> /*
> @@ -879,7 +879,7 @@ xfs_trans_log_quotaoff_item(
> xfs_qoff_logitem_t *qlp)
> {
> tp->t_flags |= XFS_TRANS_DIRTY;
> - qlp->qql_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags);
> }
>
> STATIC void
> diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> index ab438647592a..fb631081d829 100644
> --- a/fs/xfs/xfs_trans_extfree.c
> +++ b/fs/xfs/xfs_trans_extfree.c
> @@ -90,7 +90,7 @@ xfs_trans_free_extent(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
>
> next_extent = efdp->efd_next_extent;
> ASSERT(next_extent < efdp->efd_format.efd_nextents);
> @@ -155,7 +155,7 @@ xfs_extent_free_log_item(
> free = container_of(item, struct xfs_extent_free_item, xefi_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 07cea592dc01..f7bd7960a90f 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -133,14 +133,13 @@ xfs_trans_log_inode(
> * set however, then go ahead and bump the i_version counter
> * unconditionally.
> */
> - if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) &&
> + if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
> IS_I_VERSION(VFS_I(ip))) {
> if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
> flags |= XFS_ILOG_CORE;
> }
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - ip->i_itemp->ili_item.li_desc->lid_flags |= XFS_LID_DIRTY;
>
> /*
> * Always OR in the bits from the ili_last_fields field.
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 43f773297b9d..9717ae74b36d 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -19,7 +19,6 @@
> #define __XFS_TRANS_PRIV_H__
>
> struct xfs_log_item;
> -struct xfs_log_item_desc;
> struct xfs_mount;
> struct xfs_trans;
> struct xfs_ail;
> diff --git a/fs/xfs/xfs_trans_refcount.c b/fs/xfs/xfs_trans_refcount.c
> index 94c1877af834..c7f8e82f5bda 100644
> --- a/fs/xfs/xfs_trans_refcount.c
> +++ b/fs/xfs/xfs_trans_refcount.c
> @@ -77,7 +77,7 @@ xfs_trans_log_finish_refcount_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - cudp->cud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &cudp->cud_item.li_flags);
>
> return error;
> }
> @@ -154,7 +154,7 @@ xfs_refcount_update_log_item(
> refc = container_of(item, struct xfs_refcount_intent, ri_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - cuip->cui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
> index 9b577beb43d7..5831ca0c270b 100644
> --- a/fs/xfs/xfs_trans_rmap.c
> +++ b/fs/xfs/xfs_trans_rmap.c
> @@ -117,7 +117,7 @@ xfs_trans_log_finish_rmap_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - rudp->rud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &rudp->rud_item.li_flags);
>
> return error;
> }
> @@ -175,7 +175,7 @@ xfs_rmap_update_log_item(
> rmap = container_of(item, struct xfs_rmap_intent, ri_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - ruip->rui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 0:24 ` Dave Chinner
2018-05-09 10:10 ` Brian Foster
0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-09 0:24 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > into a transaction it is already joined to. This means the inode can
> > have multiple log item descriptors attached to the transaction for
> > it. This breaks teh 1:1 mapping that is supposed to exist
> > between the log item and log item descriptor.
> >
> > This results in the log item being processed twice during
> > transaction commit and CIL formatting, and there are lots of other
> > potential issues tha arise from double processing of log items in
> > the transaction commit state machine.
> >
> > In this case, the inode is already held by the rolling transaction
> > returned from xfs_defer_finish(), so there's no need to join it
> > again.
> >
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/xfs_symlink.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > index 5b66ac12913c..27870e5cd259 100644
> > --- a/fs/xfs/xfs_symlink.c
> > +++ b/fs/xfs/xfs_symlink.c
> > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > error = xfs_defer_finish(&tp, &dfops);
> > if (error)
> > goto error_bmap_cancel;
> > - /*
> > - * The first xact was committed, so add the inode to the new one.
> > - * Mark it dirty so it will be logged and moved forward in the log as
> > - * part of every commit.
> > - */
> > - xfs_trans_ijoin(tp, ip, 0);
> > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > +
> > /*
> > * Commit the transaction containing extent freeing and EFDs.
> > */
> > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>
> Seems fine.. but do we even need this call? We're about to commit the
> transaction and unlock the inode...
Yes, I think we do. We need it to be committed in each of the
rolling transactions so that the inode doesn't get written/replayed
before any of the other dependent metadata changes in this final
transaction.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 0:40 ` Dave Chinner
2018-05-09 10:12 ` Brian Foster
0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-09 0:40 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, May 08, 2018 at 10:18:26AM -0400, Brian Foster wrote:
> On Tue, May 08, 2018 at 01:42:00PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Another assert failure:
> >
> > XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
>
> Same assert comment...
>
> > ....
> > xfs_trans_add_item+0xcc/0xe0
> > xfs_reflink_clear_inode_flag+0x53/0x120
> > xfs_reflink_try_clear_inode_flag+0x5b/0xa0
> > ? filemap_write_and_wait+0x4f/0x70
> > xfs_reflink_unshare+0x18e/0x19d
> > xfs_file_fallocate+0x241/0x310
> > ? selinux_file_permission+0xd4/0x140
> > vfs_fallocate+0x13d/0x260
> > SyS_fallocate+0x43/0x80
> >
> > Another fix.
> >
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/xfs_reflink.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index bce2b5351d64..12d441a73b53 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
> > return 0;
> > }
> >
> > -/* Clear the inode reflink flag if there are no shared extents. */
> > +/*
> > + * Clear the inode reflink flag if there are no shared extents.
> > + *
> > + * The caller is responsible for joining the inode to the transaction passed in.
> > + * The inode will be joined to the transaction that is returned to the caller.
> > + */
> > int
> > xfs_reflink_clear_inode_flag(
> > struct xfs_inode *ip,
> > @@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
> > * We didn't find any shared blocks so turn off the reflink flag.
> > * First, get rid of any leftover CoW mappings.
> > */
> > - xfs_trans_ijoin(*tpp, ip, 0);
>
> Wasn't this just added in the previous patch?
No, it was /moved/ in the previous patch from lower in the same
function to ensure that the join was done before the call to
xfs_reflink_cancel_cow_blocks().
i.e. the previous patch fixed this path:
xfs_reflink_cancel_cow_range
xfs_trans_ijoin()
xfs_reflink_cancel_cow_blocks()
xfs_trans_ijoin()
This patch fixes this path:
xfs_reflink_unshare
xfs_reflink_try_clear_inode_flag()
xfs_trans_ijoin()
xfs_reflink_clear_inode_flag
xfs_trans_ijoin()
So even if I didn't change xfs_reflink_clear_inode_flag() in the
previous patch, there's still a separate double ijoin bug in the
code, and hence this patch would still be necessary.
> This seems a bit
> superfluous. If the inode was already joined by the one caller of this
> function, why not just remove this call in the previous patch rather
> than move it and remove it?
Because every time I put multiple independent fixes into a single
patch I get told to separate them into individual patches.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/9] xfs: don't assert fail with AIL lock held
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 6:13 ` Christoph Hellwig
2018-05-09 14:52 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Christoph Hellwig @ 2018-05-09 6:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
Not really excited about this, but if it helps you debugging the AIL
code:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] xfs: get rid of the log item descriptor
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 6:27 ` Christoph Hellwig
2018-05-09 15:19 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Christoph Hellwig @ 2018-05-09 6:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
> /* Chase down all the log items to see if the bp is there */
> - list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
> - struct xfs_buf_log_item *bip;
> - bip = (struct xfs_buf_log_item *)lidp->lid_item;
> + list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
> + struct xfs_buf_log_item *bip = (struct xfs_buf_log_item *)lip;
> +
> if (bip->bli_item.li_type == XFS_LI_BUF &&
This isn't really new in this patch, but I think this code needs to
be fixed to respect typing. That is:
- do the li_type check on the original log item we iterate over
- use containe_of to get at the buf_log_item
- possible move this code into a helper function in xfs_buf_item.c,
where it logically belongs and can use the BUF_ITEM() helper.
but that can be done in a separate fixup patch.
Otherwise looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-09 0:24 ` Dave Chinner
@ 2018-05-09 10:10 ` Brian Foster
2018-05-09 15:02 ` Darrick J. Wong
0 siblings, 1 reply; 35+ messages in thread
From: Brian Foster @ 2018-05-09 10:10 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > into a transaction it is already joined to. This means the inode can
> > > have multiple log item descriptors attached to the transaction for
> > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > between the log item and log item descriptor.
> > >
> > > This results in the log item being processed twice during
> > > transaction commit and CIL formatting, and there are lots of other
> > > potential issues tha arise from double processing of log items in
> > > the transaction commit state machine.
> > >
> > > In this case, the inode is already held by the rolling transaction
> > > returned from xfs_defer_finish(), so there's no need to join it
> > > again.
> > >
> > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > index 5b66ac12913c..27870e5cd259 100644
> > > --- a/fs/xfs/xfs_symlink.c
> > > +++ b/fs/xfs/xfs_symlink.c
> > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > error = xfs_defer_finish(&tp, &dfops);
> > > if (error)
> > > goto error_bmap_cancel;
> > > - /*
> > > - * The first xact was committed, so add the inode to the new one.
> > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > - * part of every commit.
> > > - */
> > > - xfs_trans_ijoin(tp, ip, 0);
> > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > +
> > > /*
> > > * Commit the transaction containing extent freeing and EFDs.
> > > */
> > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >
> > Seems fine.. but do we even need this call? We're about to commit the
> > transaction and unlock the inode...
>
> Yes, I think we do. We need it to be committed in each of the
> rolling transactions so that the inode doesn't get written/replayed
> before any of the other dependent metadata changes in this final
> transaction.
>
Hmm, I don't follow what that means. IIUC the act of logging it again
simply moves it forward in the log. That makes sense down in the dfops
code but seems unecessary here given that we are about to complete the
chain of transactions.
xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
remote bufs, joins the inode to the dfops and finishes the dfops. We
return from xfs_defer_finish() having committed the (still locked) inode
modifications and have a new/rolled transaction that covers the free of
the associated blocks (EFDs). I could certainly be missing something,
but from that point what difference does it make whether the final
transaction relogs the inode before it commits? The in-core inode is
still locked and the previous inode modifications may very well already
be in the on-disk log.
That said, it seems harmless despite not understanding what it's for. So
with or without the xfs_trans_log_inode() call:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
2018-05-09 0:40 ` Dave Chinner
@ 2018-05-09 10:12 ` Brian Foster
2018-05-09 15:19 ` Darrick J. Wong
0 siblings, 1 reply; 35+ messages in thread
From: Brian Foster @ 2018-05-09 10:12 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Wed, May 09, 2018 at 10:40:59AM +1000, Dave Chinner wrote:
> On Tue, May 08, 2018 at 10:18:26AM -0400, Brian Foster wrote:
> > On Tue, May 08, 2018 at 01:42:00PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > Another assert failure:
> > >
> > > XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> >
> > Same assert comment...
> >
> > > ....
> > > xfs_trans_add_item+0xcc/0xe0
> > > xfs_reflink_clear_inode_flag+0x53/0x120
> > > xfs_reflink_try_clear_inode_flag+0x5b/0xa0
> > > ? filemap_write_and_wait+0x4f/0x70
> > > xfs_reflink_unshare+0x18e/0x19d
> > > xfs_file_fallocate+0x241/0x310
> > > ? selinux_file_permission+0xd4/0x140
> > > vfs_fallocate+0x13d/0x260
> > > SyS_fallocate+0x43/0x80
> > >
> > > Another fix.
> > >
> > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > > fs/xfs/xfs_reflink.c | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > index bce2b5351d64..12d441a73b53 100644
> > > --- a/fs/xfs/xfs_reflink.c
> > > +++ b/fs/xfs/xfs_reflink.c
> > > @@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
> > > return 0;
> > > }
> > >
> > > -/* Clear the inode reflink flag if there are no shared extents. */
> > > +/*
> > > + * Clear the inode reflink flag if there are no shared extents.
> > > + *
> > > + * The caller is responsible for joining the inode to the transaction passed in.
> > > + * The inode will be joined to the transaction that is returned to the caller.
> > > + */
> > > int
> > > xfs_reflink_clear_inode_flag(
> > > struct xfs_inode *ip,
> > > @@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
> > > * We didn't find any shared blocks so turn off the reflink flag.
> > > * First, get rid of any leftover CoW mappings.
> > > */
> > > - xfs_trans_ijoin(*tpp, ip, 0);
> >
...
>
> > This seems a bit
> > superfluous. If the inode was already joined by the one caller of this
> > function, why not just remove this call in the previous patch rather
> > than move it and remove it?
>
> Because every time I put multiple independent fixes into a single
> patch I get told to separate them into individual patches.
>
Ok, fair enough:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/9] xfs: log item flags are racy
2018-05-08 3:41 ` [PATCH 1/9] xfs: log item flags are racy Dave Chinner
@ 2018-05-09 14:51 ` Darrick J. Wong
0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 14:51 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:54PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The log item flags contain a field that is protected by the AIL
> lock - the XFS_LI_IN_AIL flag. We use non-atomic RMW operations to
> set and clear these flags, but most of the updates and checks are
> not done with the AIL lock held and so are susceptible to update
> races.
>
> Fix this by changing the log item flags to use atomic bitops rather
> than be reliant on the AIL lock for update serialisation.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks ok, will test,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_bmap_item.c | 4 ++--
> fs/xfs/xfs_buf_item.c | 4 +++-
> fs/xfs/xfs_dquot.c | 7 +++----
> fs/xfs/xfs_dquot_item.c | 2 +-
> fs/xfs/xfs_extfree_item.c | 4 ++--
> fs/xfs/xfs_icache.c | 3 ++-
> fs/xfs/xfs_icreate_item.c | 2 +-
> fs/xfs/xfs_inode.c | 4 ++--
> fs/xfs/xfs_inode_item.c | 8 ++++----
> fs/xfs/xfs_log.c | 2 +-
> fs/xfs/xfs_qm.c | 2 +-
> fs/xfs/xfs_refcount_item.c | 4 ++--
> fs/xfs/xfs_rmap_item.c | 4 ++--
> fs/xfs/xfs_trace.h | 6 +++---
> fs/xfs/xfs_trans.c | 4 ++--
> fs/xfs/xfs_trans.h | 19 ++++++++++++-------
> fs/xfs/xfs_trans_ail.c | 9 ++++-----
> fs/xfs/xfs_trans_buf.c | 2 +-
> fs/xfs/xfs_trans_priv.h | 10 ++++------
> 19 files changed, 52 insertions(+), 48 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index 2203465e63ea..618bb71535c8 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -160,7 +160,7 @@ STATIC void
> xfs_bui_item_unlock(
> struct xfs_log_item *lip)
> {
> - if (lip->li_flags & XFS_LI_ABORTED)
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> xfs_bui_release(BUI_ITEM(lip));
> }
>
> @@ -305,7 +305,7 @@ xfs_bud_item_unlock(
> {
> struct xfs_bud_log_item *budp = BUD_ITEM(lip);
>
> - if (lip->li_flags & XFS_LI_ABORTED) {
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> xfs_bui_release(budp->bud_buip);
> kmem_zone_free(xfs_bud_zone, budp);
> }
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 82ad270e390e..df62082f2204 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -568,13 +568,15 @@ xfs_buf_item_unlock(
> {
> struct xfs_buf_log_item *bip = BUF_ITEM(lip);
> struct xfs_buf *bp = bip->bli_buf;
> - bool aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> + bool aborted;
> bool hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> bool dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> #if defined(DEBUG) || defined(XFS_WARN)
> bool ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> #endif
>
> + aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
> +
> /* Clear the buffer's association with this transaction. */
> bp->b_transp = NULL;
>
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index a7daef9e16bf..d0b65679baae 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -913,9 +913,9 @@ xfs_qm_dqflush_done(
> * since it's cheaper, and then we recheck while
> * holding the lock before removing the dquot from the AIL.
> */
> - if ((lip->li_flags & XFS_LI_IN_AIL) &&
> + if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) &&
> ((lip->li_lsn == qip->qli_flush_lsn) ||
> - (lip->li_flags & XFS_LI_FAILED))) {
> + test_bit(XFS_LI_FAILED, &lip->li_flags))) {
>
> /* xfs_trans_ail_delete() drops the AIL lock. */
> spin_lock(&ailp->ail_lock);
> @@ -926,8 +926,7 @@ xfs_qm_dqflush_done(
> * Clear the failed state since we are about to drop the
> * flush lock
> */
> - if (lip->li_flags & XFS_LI_FAILED)
> - xfs_clear_li_failed(lip);
> + xfs_clear_li_failed(lip);
> spin_unlock(&ailp->ail_lock);
> }
> }
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 4b331e354da7..57df98122156 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -173,7 +173,7 @@ xfs_qm_dquot_logitem_push(
> * The buffer containing this item failed to be written back
> * previously. Resubmit the buffer for IO
> */
> - if (lip->li_flags & XFS_LI_FAILED) {
> + if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
> if (!xfs_buf_trylock(bp))
> return XFS_ITEM_LOCKED;
>
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index b5b1e567b9f4..70b7d48af6d6 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -168,7 +168,7 @@ STATIC void
> xfs_efi_item_unlock(
> struct xfs_log_item *lip)
> {
> - if (lip->li_flags & XFS_LI_ABORTED)
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> xfs_efi_release(EFI_ITEM(lip));
> }
>
> @@ -402,7 +402,7 @@ xfs_efd_item_unlock(
> {
> struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
>
> - if (lip->li_flags & XFS_LI_ABORTED) {
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> xfs_efi_release(efdp->efd_efip);
> xfs_efd_item_free(efdp);
> }
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 9a18f69f6e96..98b7a4ae15e4 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -107,7 +107,8 @@ xfs_inode_free_callback(
> xfs_idestroy_fork(ip, XFS_COW_FORK);
>
> if (ip->i_itemp) {
> - ASSERT(!(ip->i_itemp->ili_item.li_flags & XFS_LI_IN_AIL));
> + ASSERT(!test_bit(XFS_LI_IN_AIL,
> + &ip->i_itemp->ili_item.li_flags));
> xfs_inode_item_destroy(ip);
> ip->i_itemp = NULL;
> }
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index 865ad1373e5e..a99a0f8aa528 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -91,7 +91,7 @@ xfs_icreate_item_unlock(
> {
> struct xfs_icreate_item *icp = ICR_ITEM(lip);
>
> - if (icp->ic_item.li_flags & XFS_LI_ABORTED)
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> kmem_zone_free(xfs_icreate_zone, icp);
> return;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2b70c8b4cee2..16a43ba884cc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -498,7 +498,7 @@ xfs_lock_inodes(
> if (!try_lock) {
> for (j = (i - 1); j >= 0 && !try_lock; j--) {
> lp = (xfs_log_item_t *)ips[j]->i_itemp;
> - if (lp && (lp->li_flags & XFS_LI_IN_AIL))
> + if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags))
> try_lock++;
> }
> }
> @@ -598,7 +598,7 @@ xfs_lock_two_inodes(
> * and try again.
> */
> lp = (xfs_log_item_t *)ip0->i_itemp;
> - if (lp && (lp->li_flags & XFS_LI_IN_AIL)) {
> + if (lp && test_bit(XFS_LI_IN_AIL, &lp->li_flags)) {
> if (!xfs_ilock_nowait(ip1, xfs_lock_inumorder(ip1_mode, 1))) {
> xfs_iunlock(ip0, ip0_mode);
> if ((++attempts % 5) == 0)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 34b91b789702..3e5b8574818e 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -518,7 +518,7 @@ xfs_inode_item_push(
> * The buffer containing this item failed to be written back
> * previously. Resubmit the buffer for IO.
> */
> - if (lip->li_flags & XFS_LI_FAILED) {
> + if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
> if (!xfs_buf_trylock(bp))
> return XFS_ITEM_LOCKED;
>
> @@ -729,14 +729,14 @@ xfs_iflush_done(
> */
> iip = INODE_ITEM(blip);
> if ((iip->ili_logged && blip->li_lsn == iip->ili_flush_lsn) ||
> - (blip->li_flags & XFS_LI_FAILED))
> + test_bit(XFS_LI_FAILED, &blip->li_flags))
> need_ail++;
> }
>
> /* make sure we capture the state of the initial inode. */
> iip = INODE_ITEM(lip);
> if ((iip->ili_logged && lip->li_lsn == iip->ili_flush_lsn) ||
> - lip->li_flags & XFS_LI_FAILED)
> + test_bit(XFS_LI_FAILED, &lip->li_flags))
> need_ail++;
>
> /*
> @@ -803,7 +803,7 @@ xfs_iflush_abort(
> xfs_inode_log_item_t *iip = ip->i_itemp;
>
> if (iip) {
> - if (iip->ili_item.li_flags & XFS_LI_IN_AIL) {
> + if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags)) {
> xfs_trans_ail_remove(&iip->ili_item,
> stale ? SHUTDOWN_LOG_IO_ERROR :
> SHUTDOWN_CORRUPT_INCORE);
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 2fcd9ed5d075..e427864434c1 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2132,7 +2132,7 @@ xlog_print_trans(
>
> xfs_warn(mp, "log item: ");
> xfs_warn(mp, " type = 0x%x", lip->li_type);
> - xfs_warn(mp, " flags = 0x%x", lip->li_flags);
> + xfs_warn(mp, " flags = 0x%lx", lip->li_flags);
> if (!lv)
> continue;
> xfs_warn(mp, " niovecs = %d", lv->lv_niovecs);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index ec39ae274c78..4438fc446d18 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -173,7 +173,7 @@ xfs_qm_dqpurge(
>
> ASSERT(atomic_read(&dqp->q_pincount) == 0);
> ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
> - !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
> + !test_bit(XFS_LI_IN_AIL, &dqp->q_logitem.qli_item.li_flags));
>
> xfs_dqfunlock(dqp);
> xfs_dqunlock(dqp);
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index 15c9393dd7a7..e5866b714d5f 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -159,7 +159,7 @@ STATIC void
> xfs_cui_item_unlock(
> struct xfs_log_item *lip)
> {
> - if (lip->li_flags & XFS_LI_ABORTED)
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> xfs_cui_release(CUI_ITEM(lip));
> }
>
> @@ -310,7 +310,7 @@ xfs_cud_item_unlock(
> {
> struct xfs_cud_log_item *cudp = CUD_ITEM(lip);
>
> - if (lip->li_flags & XFS_LI_ABORTED) {
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> xfs_cui_release(cudp->cud_cuip);
> kmem_zone_free(xfs_cud_zone, cudp);
> }
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index 06a07846c9b3..e5b5b3e7ef82 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -158,7 +158,7 @@ STATIC void
> xfs_rui_item_unlock(
> struct xfs_log_item *lip)
> {
> - if (lip->li_flags & XFS_LI_ABORTED)
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
> xfs_rui_release(RUI_ITEM(lip));
> }
>
> @@ -331,7 +331,7 @@ xfs_rud_item_unlock(
> {
> struct xfs_rud_log_item *rudp = RUD_ITEM(lip);
>
> - if (lip->li_flags & XFS_LI_ABORTED) {
> + if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> xfs_rui_release(rudp->rud_ruip);
> kmem_zone_free(xfs_rud_zone, rudp);
> }
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8955254b900e..7f4d961fae12 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -442,7 +442,7 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __field(int, bli_refcount)
> __field(unsigned, bli_flags)
> __field(void *, li_desc)
> - __field(unsigned, li_flags)
> + __field(unsigned long, li_flags)
> ),
> TP_fast_assign(
> __entry->dev = bip->bli_buf->b_target->bt_dev;
> @@ -1018,7 +1018,7 @@ DECLARE_EVENT_CLASS(xfs_log_item_class,
> __field(dev_t, dev)
> __field(void *, lip)
> __field(uint, type)
> - __field(uint, flags)
> + __field(unsigned long, flags)
> __field(xfs_lsn_t, lsn)
> ),
> TP_fast_assign(
> @@ -1070,7 +1070,7 @@ DECLARE_EVENT_CLASS(xfs_ail_class,
> __field(dev_t, dev)
> __field(void *, lip)
> __field(uint, type)
> - __field(uint, flags)
> + __field(unsigned long, flags)
> __field(xfs_lsn_t, old_lsn)
> __field(xfs_lsn_t, new_lsn)
> ),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index d6d8f9d129a7..c6a2a3cb9df5 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -790,7 +790,7 @@ xfs_trans_free_items(
> if (commit_lsn != NULLCOMMITLSN)
> lip->li_ops->iop_committing(lip, commit_lsn);
> if (abort)
> - lip->li_flags |= XFS_LI_ABORTED;
> + set_bit(XFS_LI_ABORTED, &lip->li_flags);
> lip->li_ops->iop_unlock(lip);
>
> xfs_trans_free_item_desc(lidp);
> @@ -861,7 +861,7 @@ xfs_trans_committed_bulk(
> xfs_lsn_t item_lsn;
>
> if (aborted)
> - lip->li_flags |= XFS_LI_ABORTED;
> + set_bit(XFS_LI_ABORTED, &lip->li_flags);
> item_lsn = lip->li_ops->iop_committed(lip, commit_lsn);
>
> /* item_lsn of -1 means the item needs no further processing */
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 9d542dfe0052..51145ddf7e5b 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -48,7 +48,7 @@ typedef struct xfs_log_item {
> struct xfs_mount *li_mountp; /* ptr to fs mount */
> struct xfs_ail *li_ailp; /* ptr to AIL */
> uint li_type; /* item type */
> - uint li_flags; /* misc flags */
> + unsigned long li_flags; /* misc flags */
> struct xfs_buf *li_buf; /* real buffer pointer */
> struct list_head li_bio_list; /* buffer item list */
> void (*li_cb)(struct xfs_buf *,
> @@ -64,14 +64,19 @@ typedef struct xfs_log_item {
> xfs_lsn_t li_seq; /* CIL commit seq */
> } xfs_log_item_t;
>
> -#define XFS_LI_IN_AIL 0x1
> -#define XFS_LI_ABORTED 0x2
> -#define XFS_LI_FAILED 0x4
> +/*
> + * li_flags use the (set/test/clear)_bit atomic interfaces because updates can
> + * race with each other and we don't want to have to use the AIL lock to
> + * serialise all updates.
> + */
> +#define XFS_LI_IN_AIL 0
> +#define XFS_LI_ABORTED 1
> +#define XFS_LI_FAILED 2
>
> #define XFS_LI_FLAGS \
> - { XFS_LI_IN_AIL, "IN_AIL" }, \
> - { XFS_LI_ABORTED, "ABORTED" }, \
> - { XFS_LI_FAILED, "FAILED" }
> + { (1 << XFS_LI_IN_AIL), "IN_AIL" }, \
> + { (1 << XFS_LI_ABORTED), "ABORTED" }, \
> + { (1 << XFS_LI_FAILED), "FAILED" }
>
> struct xfs_item_ops {
> void (*iop_size)(xfs_log_item_t *, int *, int *);
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index d4a2445215e6..50611d2bcbc2 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -46,7 +46,7 @@ xfs_ail_check(
> /*
> * Check the next and previous entries are valid.
> */
> - ASSERT((lip->li_flags & XFS_LI_IN_AIL) != 0);
> + ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
> prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
> if (&prev_lip->li_ail != &ailp->ail_head)
> ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> @@ -684,7 +684,7 @@ xfs_trans_ail_update_bulk(
>
> for (i = 0; i < nr_items; i++) {
> struct xfs_log_item *lip = log_items[i];
> - if (lip->li_flags & XFS_LI_IN_AIL) {
> + if (test_and_set_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> /* check if we really need to move the item */
> if (XFS_LSN_CMP(lsn, lip->li_lsn) <= 0)
> continue;
> @@ -694,7 +694,6 @@ xfs_trans_ail_update_bulk(
> if (mlip == lip)
> mlip_changed = 1;
> } else {
> - lip->li_flags |= XFS_LI_IN_AIL;
> trace_xfs_ail_insert(lip, 0, lsn);
> }
> lip->li_lsn = lsn;
> @@ -725,7 +724,7 @@ xfs_ail_delete_one(
> trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> xfs_ail_delete(ailp, lip);
> xfs_clear_li_failed(lip);
> - lip->li_flags &= ~XFS_LI_IN_AIL;
> + clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
> lip->li_lsn = 0;
>
> return mlip == lip;
> @@ -761,7 +760,7 @@ xfs_trans_ail_delete(
> struct xfs_mount *mp = ailp->ail_mount;
> bool mlip_changed;
>
> - if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> + if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
> spin_unlock(&ailp->ail_lock);
> if (!XFS_FORCED_SHUTDOWN(mp)) {
> xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index a5d9dfc45d98..0081e9b3decf 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -442,7 +442,7 @@ xfs_trans_brelse(
> ASSERT(bp->b_pincount == 0);
> ***/
> ASSERT(atomic_read(&bip->bli_refcount) == 0);
> - ASSERT(!(bip->bli_item.li_flags & XFS_LI_IN_AIL));
> + ASSERT(!test_bit(XFS_LI_IN_AIL, &bip->bli_item.li_flags));
> ASSERT(!(bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF));
> xfs_buf_item_relse(bp);
> }
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index be24b0c8a332..43f773297b9d 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -119,7 +119,7 @@ xfs_trans_ail_remove(
>
> spin_lock(&ailp->ail_lock);
> /* xfs_trans_ail_delete() drops the AIL lock */
> - if (lip->li_flags & XFS_LI_IN_AIL)
> + if (test_bit(XFS_LI_IN_AIL, &lip->li_flags))
> xfs_trans_ail_delete(ailp, lip, shutdown_type);
> else
> spin_unlock(&ailp->ail_lock);
> @@ -171,11 +171,10 @@ xfs_clear_li_failed(
> {
> struct xfs_buf *bp = lip->li_buf;
>
> - ASSERT(lip->li_flags & XFS_LI_IN_AIL);
> + ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
> lockdep_assert_held(&lip->li_ailp->ail_lock);
>
> - if (lip->li_flags & XFS_LI_FAILED) {
> - lip->li_flags &= ~XFS_LI_FAILED;
> + if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) {
> lip->li_buf = NULL;
> xfs_buf_rele(bp);
> }
> @@ -188,9 +187,8 @@ xfs_set_li_failed(
> {
> lockdep_assert_held(&lip->li_ailp->ail_lock);
>
> - if (!(lip->li_flags & XFS_LI_FAILED)) {
> + if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) {
> xfs_buf_hold(bp);
> - lip->li_flags |= XFS_LI_FAILED;
> lip->li_buf = bp;
> }
> }
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/9] xfs: add tracing to high level transaction operations
2018-05-08 3:41 ` [PATCH 2/9] xfs: add tracing to high level transaction operations Dave Chinner
@ 2018-05-09 14:51 ` Darrick J. Wong
0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 14:51 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:55PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Because currently we have no idea what the transaction context we
> are operating in is, and I need to know that information to track
> down bugs in multiple log item joins to transactions.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_log_cil.c | 1 +
> fs/xfs/xfs_trace.h | 37 +++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_trans.c | 20 +++++++++++++++++++-
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index 4668403b1741..a8675c631438 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -1013,6 +1013,7 @@ xfs_log_commit_cil(
> *commit_lsn = xc_commit_lsn;
>
> xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> + tp->t_ticket = NULL;
> xfs_trans_unreserve_and_mod_sb(tp);
>
> /*
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 7f4d961fae12..8136f2280173 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3346,6 +3346,43 @@ TRACE_EVENT(xfs_trans_resv_calc,
> __entry->logflags)
> );
>
> +DECLARE_EVENT_CLASS(xfs_trans_class,
> + TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip),
> + TP_ARGS(tp, caller_ip),
> + TP_STRUCT__entry(
> + __field(dev_t, dev)
> + __field(uint32_t, tid)
> + __field(uint32_t, flags)
> + __field(unsigned long, caller_ip)
> + ),
> + TP_fast_assign(
> + __entry->dev = tp->t_mountp->m_super->s_dev;
> + __entry->tid = 0;
> + if (tp->t_ticket)
> + __entry->tid = tp->t_ticket->t_tid;
> + __entry->flags = tp->t_flags;
> + __entry->caller_ip = caller_ip;
> + ),
> + TP_printk("dev %d:%d trans %x flags 0x%x caller %pS",
> + MAJOR(__entry->dev), MINOR(__entry->dev),
> + __entry->tid,
> + __entry->flags,
> + (char *)__entry->caller_ip)
> +)
> +
> +#define DEFINE_TRANS_EVENT(name) \
> +DEFINE_EVENT(xfs_trans_class, name, \
> + TP_PROTO(struct xfs_trans *tp, unsigned long caller_ip), \
> + TP_ARGS(tp, caller_ip))
> +DEFINE_TRANS_EVENT(xfs_trans_alloc);
> +DEFINE_TRANS_EVENT(xfs_trans_cancel);
> +DEFINE_TRANS_EVENT(xfs_trans_commit);
> +DEFINE_TRANS_EVENT(xfs_trans_dup);
> +DEFINE_TRANS_EVENT(xfs_trans_free);
> +DEFINE_TRANS_EVENT(xfs_trans_roll);
> +DEFINE_TRANS_EVENT(xfs_trans_add_item);
> +DEFINE_TRANS_EVENT(xfs_trans_free_items);
> +
> #endif /* _TRACE_XFS_H */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index c6a2a3cb9df5..24744915357b 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -79,6 +79,7 @@ xfs_trans_free(
> xfs_extent_busy_sort(&tp->t_busy);
> xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
>
> + trace_xfs_trans_free(tp, _RET_IP_);
> atomic_dec(&tp->t_mountp->m_active_trans);
> if (!(tp->t_flags & XFS_TRANS_NO_WRITECOUNT))
> sb_end_intwrite(tp->t_mountp->m_super);
> @@ -100,6 +101,8 @@ xfs_trans_dup(
> {
> xfs_trans_t *ntp;
>
> + trace_xfs_trans_dup(tp, _RET_IP_);
> +
> ntp = kmem_zone_zalloc(xfs_trans_zone, KM_SLEEP);
>
> /*
> @@ -283,6 +286,8 @@ xfs_trans_alloc(
> return error;
> }
>
> + trace_xfs_trans_alloc(tp, _RET_IP_);
> +
> *tpp = tp;
> return 0;
> }
> @@ -749,6 +754,8 @@ xfs_trans_add_item(
> list_add_tail(&lidp->lid_trans, &tp->t_items);
>
> lip->li_desc = lidp;
> +
> + trace_xfs_trans_add_item(tp, _RET_IP_);
> }
>
> STATIC void
> @@ -782,6 +789,8 @@ xfs_trans_free_items(
> {
> struct xfs_log_item_desc *lidp, *next;
>
> + trace_xfs_trans_free_items(tp, _RET_IP_);
> +
> list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
> struct xfs_log_item *lip = lidp->lid_item;
>
> @@ -936,6 +945,8 @@ __xfs_trans_commit(
> int error = 0;
> int sync = tp->t_flags & XFS_TRANS_SYNC;
>
> + trace_xfs_trans_commit(tp, _RET_IP_);
> +
> /*
> * If there is nothing to be logged by the transaction,
> * then unlock all of the items associated with the
> @@ -991,6 +1002,7 @@ __xfs_trans_commit(
> commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, regrant);
> if (commit_lsn == -1 && !error)
> error = -EIO;
> + tp->t_ticket = NULL;
> }
> current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> xfs_trans_free_items(tp, NULLCOMMITLSN, !!error);
> @@ -1022,6 +1034,8 @@ xfs_trans_cancel(
> struct xfs_mount *mp = tp->t_mountp;
> bool dirty = (tp->t_flags & XFS_TRANS_DIRTY);
>
> + trace_xfs_trans_cancel(tp, _RET_IP_);
> +
> /*
> * See if the caller is relying on us to shut down the
> * filesystem. This happens in paths where we detect
> @@ -1042,8 +1056,10 @@ xfs_trans_cancel(
> xfs_trans_unreserve_and_mod_sb(tp);
> xfs_trans_unreserve_and_mod_dquots(tp);
>
> - if (tp->t_ticket)
> + if (tp->t_ticket) {
> xfs_log_done(mp, tp->t_ticket, NULL, false);
> + tp->t_ticket = NULL;
> + }
>
> /* mark this thread as no longer being in a transaction */
> current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
> @@ -1067,6 +1083,8 @@ xfs_trans_roll(
> struct xfs_trans_res tres;
> int error;
>
> + trace_xfs_trans_roll(trans, _RET_IP_);
> +
> /*
> * Copy the critical parameters from one trans to the next.
> */
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints
2018-05-08 3:41 ` [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints Dave Chinner
@ 2018-05-09 14:52 ` Darrick J. Wong
0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 14:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:56PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> So it's clear in the trace where they are being called from.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_defer.c | 12 ++++++------
> fs/xfs/xfs_trace.h | 17 +++++++++++------
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 087fea02c389..51821af5d653 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -220,7 +220,7 @@ xfs_defer_trans_abort(
> {
> struct xfs_defer_pending *dfp;
>
> - trace_xfs_defer_trans_abort(tp->t_mountp, dop);
> + trace_xfs_defer_trans_abort(tp->t_mountp, dop, _RET_IP_);
>
> /* Abort intent items that don't have a done item. */
> list_for_each_entry(dfp, &dop->dop_pending, dfp_list) {
> @@ -253,7 +253,7 @@ xfs_defer_trans_roll(
> for (i = 0; i < XFS_DEFER_OPS_NR_BUFS && dop->dop_bufs[i]; i++)
> xfs_trans_dirty_buf(*tp, dop->dop_bufs[i]);
>
> - trace_xfs_defer_trans_roll((*tp)->t_mountp, dop);
> + trace_xfs_defer_trans_roll((*tp)->t_mountp, dop, _RET_IP_);
>
> /* Roll the transaction. */
> error = xfs_trans_roll(tp);
> @@ -355,7 +355,7 @@ xfs_defer_finish(
>
> ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
>
> - trace_xfs_defer_finish((*tp)->t_mountp, dop);
> + trace_xfs_defer_finish((*tp)->t_mountp, dop, _RET_IP_);
>
> /* Until we run out of pending work to finish... */
> while (xfs_defer_has_unfinished_work(dop)) {
> @@ -431,7 +431,7 @@ xfs_defer_finish(
> if (error)
> trace_xfs_defer_finish_error((*tp)->t_mountp, dop, error);
> else
> - trace_xfs_defer_finish_done((*tp)->t_mountp, dop);
> + trace_xfs_defer_finish_done((*tp)->t_mountp, dop, _RET_IP_);
> return error;
> }
>
> @@ -447,7 +447,7 @@ xfs_defer_cancel(
> struct list_head *pwi;
> struct list_head *n;
>
> - trace_xfs_defer_cancel(NULL, dop);
> + trace_xfs_defer_cancel(NULL, dop, _RET_IP_);
>
> /*
> * Free the pending items. Caller should already have arranged
> @@ -532,5 +532,5 @@ xfs_defer_init(
> *fbp = NULLFSBLOCK;
> INIT_LIST_HEAD(&dop->dop_intake);
> INIT_LIST_HEAD(&dop->dop_pending);
> - trace_xfs_defer_init(NULL, dop);
> + trace_xfs_defer_init(NULL, dop, _RET_IP_);
> }
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8136f2280173..947ab6e32c18 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2243,30 +2243,35 @@ struct xfs_defer_pending;
> struct xfs_defer_ops;
>
> DECLARE_EVENT_CLASS(xfs_defer_class,
> - TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop),
> - TP_ARGS(mp, dop),
> + TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop,
> + unsigned long caller_ip),
> + TP_ARGS(mp, dop, caller_ip),
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(void *, dop)
> __field(char, committed)
> __field(char, low)
> + __field(unsigned long, caller_ip)
> ),
> TP_fast_assign(
> __entry->dev = mp ? mp->m_super->s_dev : 0;
> __entry->dop = dop;
> __entry->committed = dop->dop_committed;
> __entry->low = dop->dop_low;
> + __entry->caller_ip = caller_ip;
> ),
> - TP_printk("dev %d:%d ops %p committed %d low %d",
> + TP_printk("dev %d:%d ops %p committed %d low %d, caller %pS",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->dop,
> __entry->committed,
> - __entry->low)
> + __entry->low,
> + (char *)__entry->caller_ip)
> )
> #define DEFINE_DEFER_EVENT(name) \
> DEFINE_EVENT(xfs_defer_class, name, \
> - TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop), \
> - TP_ARGS(mp, dop))
> + TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, \
> + unsigned long caller_ip), \
> + TP_ARGS(mp, dop, caller_ip))
>
> DECLARE_EVENT_CLASS(xfs_defer_error_class,
> TP_PROTO(struct xfs_mount *mp, struct xfs_defer_ops *dop, int error),
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/9] xfs: don't assert fail with AIL lock held
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 6:13 ` Christoph Hellwig
@ 2018-05-09 14:52 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 14:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:57PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Been hitting AIL ordering assert failures recently, but been unable
> to trace them down because the system immediately hangs up onteh
> spinlock that was held when this assert fires:
>
> XFS: Assertion failed: XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0, file: fs/xfs/xfs_trans_ail.c, line: 52
>
> Move the assertions outside of the spinlock so the corpse can
> be dissected. Thanks to Brian Foster for supplying a clean
> way of doing this.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_trans_ail.c | 43 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 50611d2bcbc2..41e280ef1483 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -32,30 +32,51 @@
> #ifdef DEBUG
> /*
> * Check that the list is sorted as it should be.
> + *
> + * Called with the ail lock held, but we don't want to assert fail with it
> + * held otherwise we'll lock everything up and won't be able to debug the
> + * cause. Hence we sample and check the state under the AIL lock and return if
> + * everything is fine, otherwise we drop the lock and run the ASSERT checks.
> + * Asserts may not be fatal, so pick the lock back up and continue onwards.
> */
> STATIC void
> xfs_ail_check(
> - struct xfs_ail *ailp,
> - xfs_log_item_t *lip)
> + struct xfs_ail *ailp,
> + struct xfs_log_item *lip)
> {
> - xfs_log_item_t *prev_lip;
> + struct xfs_log_item *prev_lip;
> + struct xfs_log_item *next_lip;
> + xfs_lsn_t prev_lsn = NULLCOMMITLSN;
> + xfs_lsn_t next_lsn = NULLCOMMITLSN;
> + xfs_lsn_t lsn;
> + bool in_ail;
> +
>
> if (list_empty(&ailp->ail_head))
> return;
>
> /*
> - * Check the next and previous entries are valid.
> + * Sample then check the next and previous entries are valid.
> */
> - ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags));
> - prev_lip = list_entry(lip->li_ail.prev, xfs_log_item_t, li_ail);
> + in_ail = test_bit(XFS_LI_IN_AIL, &lip->li_flags);
> + prev_lip = list_entry(lip->li_ail.prev, struct xfs_log_item, li_ail);
> if (&prev_lip->li_ail != &ailp->ail_head)
> - ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) <= 0);
> -
> - prev_lip = list_entry(lip->li_ail.next, xfs_log_item_t, li_ail);
> - if (&prev_lip->li_ail != &ailp->ail_head)
> - ASSERT(XFS_LSN_CMP(prev_lip->li_lsn, lip->li_lsn) >= 0);
> + prev_lsn = prev_lip->li_lsn;
> + next_lip = list_entry(lip->li_ail.next, struct xfs_log_item, li_ail);
> + if (&next_lip->li_ail != &ailp->ail_head)
> + next_lsn = next_lip->li_lsn;
> + lsn = lip->li_lsn;
>
> + if (in_ail &&
> + (prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0) &&
> + (next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0))
> + return;
>
> + spin_unlock(&ailp->ail_lock);
> + ASSERT(in_ail);
> + ASSERT(prev_lsn == NULLCOMMITLSN || XFS_LSN_CMP(prev_lsn, lsn) <= 0);
> + ASSERT(next_lsn == NULLCOMMITLSN || XFS_LSN_CMP(next_lsn, lsn) >= 0);
> + spin_lock(&ailp->ail_lock);
> }
> #else /* !DEBUG */
> #define xfs_ail_check(a,l)
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-09 10:10 ` Brian Foster
@ 2018-05-09 15:02 ` Darrick J. Wong
2018-05-11 2:04 ` Dave Chinner
0 siblings, 1 reply; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 15:02 UTC (permalink / raw)
To: Brian Foster; +Cc: Dave Chinner, linux-xfs
On Wed, May 09, 2018 at 06:10:42AM -0400, Brian Foster wrote:
> On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> > On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > > into a transaction it is already joined to. This means the inode can
> > > > have multiple log item descriptors attached to the transaction for
> > > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > > between the log item and log item descriptor.
> > > >
> > > > This results in the log item being processed twice during
> > > > transaction commit and CIL formatting, and there are lots of other
> > > > potential issues tha arise from double processing of log items in
> > > > the transaction commit state machine.
> > > >
> > > > In this case, the inode is already held by the rolling transaction
> > > > returned from xfs_defer_finish(), so there's no need to join it
> > > > again.
> > > >
> > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > > index 5b66ac12913c..27870e5cd259 100644
> > > > --- a/fs/xfs/xfs_symlink.c
> > > > +++ b/fs/xfs/xfs_symlink.c
> > > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > > error = xfs_defer_finish(&tp, &dfops);
> > > > if (error)
> > > > goto error_bmap_cancel;
> > > > - /*
> > > > - * The first xact was committed, so add the inode to the new one.
> > > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > > - * part of every commit.
> > > > - */
> > > > - xfs_trans_ijoin(tp, ip, 0);
> > > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > +
> > > > /*
> > > > * Commit the transaction containing extent freeing and EFDs.
> > > > */
> > > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > >
> > > Seems fine.. but do we even need this call? We're about to commit the
> > > transaction and unlock the inode...
> >
> > Yes, I think we do. We need it to be committed in each of the
> > rolling transactions so that the inode doesn't get written/replayed
> > before any of the other dependent metadata changes in this final
> > transaction.
> >
>
> Hmm, I don't follow what that means. IIUC the act of logging it again
> simply moves it forward in the log. That makes sense down in the dfops
> code but seems unecessary here given that we are about to complete the
> chain of transactions.
>
> xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
> remote bufs, joins the inode to the dfops and finishes the dfops. We
> return from xfs_defer_finish() having committed the (still locked) inode
> modifications and have a new/rolled transaction that covers the free of
> the associated blocks (EFDs). I could certainly be missing something,
> but from that point what difference does it make whether the final
> transaction relogs the inode before it commits? The in-core inode is
> still locked and the previous inode modifications may very well already
> be in the on-disk log.
>
> That said, it seems harmless despite not understanding what it's for. So
> with or without the xfs_trans_log_inode() call:
<shrug> I interpreted this as Dave being fastidious about relogging
inodes after every transaction roll even if it's not strictly necessary
(but not otherwise harmful) just in case someone else stumbles in later
and dirties something in this transaction.
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@fromorbit.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range
2018-05-08 3:41 ` [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range Dave Chinner
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 15:17 ` Darrick J. Wong
1 sibling, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 15:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:41:59PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> AN inode is joined to teh same transaction twice in
> xfs_reflink_cancel_cow_range() resulting in the following assert
> failure:
>
> [ 30.180485] XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> [ 30.183435] ------------[ cut here ]------------
> ......
> [ 30.209264] Call Trace:
> [ 30.209935] xfs_trans_add_item+0xcc/0xe0
> [ 30.210968] xfs_reflink_cancel_cow_blocks+0xab/0x290
> [ 30.212249] ? xfs_trans_reserve+0x1b4/0x2b0
> [ 30.213320] ? kmem_zone_alloc+0x61/0xe0
> [ 30.214321] xfs_reflink_cancel_cow_range+0xb2/0x1f0
> [ 30.215616] xfs_fs_destroy_inode+0x1bd/0x280
> [ 30.216757] dispose_list+0x35/0x40
> [ 30.217656] evict_inodes+0x132/0x160
> [ 30.218620] generic_shutdown_super+0x3a/0x110
> [ 30.219771] kill_block_super+0x21/0x50
> [ 30.220762] deactivate_locked_super+0x39/0x70
> [ 30.221909] cleanup_mnt+0x3b/0x70
> [ 30.222819] task_work_run+0x7f/0xa0
> [ 30.223762] exit_to_usermode_loop+0x9b/0xa0
> [ 30.224884] do_syscall_64+0x18f/0x1a0
>
> Fix it and document that the callers of
> xfs_reflink_cancel_cow_blocks() must have already joined the inode
> to the permanent transaction passed in.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok, though I agree that the commit message should be changed now
that the ASSERT is gone:
"xfs_reflink_cancel_cow_range joins an inode twice to the same
transaction. This is not allowed, so fix it and document that the
callers of xfs_reflink_cancel_cow_blocks() must have already joined the
inode to the permanent transaction passed in."
Will change on the way in,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_reflink.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index cdbd342a5249..bce2b5351d64 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -552,6 +552,9 @@ xfs_reflink_trim_irec_to_next_cow(
> *
> * If cancel_real is true this function cancels all COW fork extents for the
> * inode; if cancel_real is false, real extents are not cleared.
> + *
> + * Caller must have already joined the inode to the current transaction. The
> + * inode will be joined to the transaction returned to the caller.
> */
> int
> xfs_reflink_cancel_cow_blocks(
> @@ -592,7 +595,6 @@ xfs_reflink_cancel_cow_blocks(
> if (error)
> break;
> } else if (del.br_state == XFS_EXT_UNWRITTEN || cancel_real) {
> - xfs_trans_ijoin(*tpp, ip, 0);
> xfs_defer_init(&dfops, &firstfsb);
>
> /* Free the CoW orphan record. */
> @@ -1570,6 +1572,7 @@ xfs_reflink_clear_inode_flag(
> * We didn't find any shared blocks so turn off the reflink flag.
> * First, get rid of any leftover CoW mappings.
> */
> + xfs_trans_ijoin(*tpp, ip, 0);
> error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true);
> if (error)
> return error;
> @@ -1578,7 +1581,6 @@ xfs_reflink_clear_inode_flag(
> trace_xfs_reflink_unset_inode_flag(ip);
> ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> xfs_inode_clear_cowblocks_tag(ip);
> - xfs_trans_ijoin(*tpp, ip, 0);
> xfs_trans_log_inode(*tpp, ip, XFS_ILOG_CORE);
>
> return error;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag()
2018-05-09 10:12 ` Brian Foster
@ 2018-05-09 15:19 ` Darrick J. Wong
0 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 15:19 UTC (permalink / raw)
To: Brian Foster; +Cc: Dave Chinner, linux-xfs
On Wed, May 09, 2018 at 06:12:04AM -0400, Brian Foster wrote:
> On Wed, May 09, 2018 at 10:40:59AM +1000, Dave Chinner wrote:
> > On Tue, May 08, 2018 at 10:18:26AM -0400, Brian Foster wrote:
> > > On Tue, May 08, 2018 at 01:42:00PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > Another assert failure:
> > > >
> > > > XFS: Assertion failed: !(lip->li_flags & XFS_LI_TRANS), file: fs/xfs/xfs_trans.c, line: 740
> > >
> > > Same assert comment...
> > >
> > > > ....
> > > > xfs_trans_add_item+0xcc/0xe0
> > > > xfs_reflink_clear_inode_flag+0x53/0x120
> > > > xfs_reflink_try_clear_inode_flag+0x5b/0xa0
> > > > ? filemap_write_and_wait+0x4f/0x70
> > > > xfs_reflink_unshare+0x18e/0x19d
> > > > xfs_file_fallocate+0x241/0x310
> > > > ? selinux_file_permission+0xd4/0x140
> > > > vfs_fallocate+0x13d/0x260
> > > > SyS_fallocate+0x43/0x80
> > > >
> > > > Another fix.
> > > >
> > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > > fs/xfs/xfs_reflink.c | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > > index bce2b5351d64..12d441a73b53 100644
> > > > --- a/fs/xfs/xfs_reflink.c
> > > > +++ b/fs/xfs/xfs_reflink.c
> > > > @@ -1553,7 +1553,12 @@ xfs_reflink_inode_has_shared_extents(
> > > > return 0;
> > > > }
> > > >
> > > > -/* Clear the inode reflink flag if there are no shared extents. */
> > > > +/*
> > > > + * Clear the inode reflink flag if there are no shared extents.
> > > > + *
> > > > + * The caller is responsible for joining the inode to the transaction passed in.
> > > > + * The inode will be joined to the transaction that is returned to the caller.
> > > > + */
> > > > int
> > > > xfs_reflink_clear_inode_flag(
> > > > struct xfs_inode *ip,
> > > > @@ -1572,7 +1577,6 @@ xfs_reflink_clear_inode_flag(
> > > > * We didn't find any shared blocks so turn off the reflink flag.
> > > > * First, get rid of any leftover CoW mappings.
> > > > */
> > > > - xfs_trans_ijoin(*tpp, ip, 0);
> > >
> ...
> >
> > > This seems a bit
> > > superfluous. If the inode was already joined by the one caller of this
> > > function, why not just remove this call in the previous patch rather
> > > than move it and remove it?
> >
> > Because every time I put multiple independent fixes into a single
> > patch I get told to separate them into individual patches.
> >
>
> Ok, fair enough:
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Revised commit log:
"xfs_reflink_clear_inode_flag double-joins an inode to a transaction,
which is not allowed. Fix that and document that the caller must have
already joined it."
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@fromorbit.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse
2018-05-08 3:42 ` [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse Dave Chinner
2018-05-08 14:18 ` Brian Foster
@ 2018-05-09 15:19 ` Darrick J. Wong
1 sibling, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 15:19 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:42:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Just to make sure the item isn't associated with another
> transaction when we try to reuse it.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_buf_item.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index df62082f2204..8d6ed045b643 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -745,8 +745,10 @@ xfs_buf_item_init(
> * nothing to do here so return.
> */
> ASSERT(bp->b_target->bt_mount == mp);
> - if (bip != NULL) {
> + if (bip) {
> ASSERT(bip->bli_item.li_type == XFS_LI_BUF);
> + ASSERT(!bp->b_transp);
> + ASSERT(bip->bli_buf == bp);
> return 0;
> }
>
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 9/9] xfs: get rid of the log item descriptor
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 6:27 ` Christoph Hellwig
@ 2018-05-09 15:19 ` Darrick J. Wong
2 siblings, 0 replies; 35+ messages in thread
From: Darrick J. Wong @ 2018-05-09 15:19 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Tue, May 08, 2018 at 01:42:02PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> It's just a connector between a transaction and a log item. There's
> a 1:1 relationship between a log item descriptor and a log item,
> and a 1:1 relationship between a log item descriptor and a
> transaction. Both relationships are created and terminated at the
> same time, so why do we even have the descriptor?
>
> Replace it with a specific list_head in the log item and a new
> log item dirtied flag to replace the XFS_LID_DIRTY flag.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Yum yum,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_bmap.c | 8 ++---
> fs/xfs/libxfs/xfs_shared.h | 15 ----------
> fs/xfs/xfs_buf_item.c | 2 +-
> fs/xfs/xfs_icreate_item.c | 2 +-
> fs/xfs/xfs_log.c | 10 +++----
> fs/xfs/xfs_log_cil.c | 21 ++++++--------
> fs/xfs/xfs_super.c | 10 +------
> fs/xfs/xfs_trace.h | 5 +---
> fs/xfs/xfs_trans.c | 58 ++++++++++---------------------------
> fs/xfs/xfs_trans.h | 8 ++---
> fs/xfs/xfs_trans_bmap.c | 4 +--
> fs/xfs/xfs_trans_buf.c | 22 ++++++--------
> fs/xfs/xfs_trans_dquot.c | 4 +--
> fs/xfs/xfs_trans_extfree.c | 4 +--
> fs/xfs/xfs_trans_inode.c | 3 +-
> fs/xfs/xfs_trans_priv.h | 1 -
> fs/xfs/xfs_trans_refcount.c | 4 +--
> fs/xfs/xfs_trans_rmap.c | 4 +--
> 18 files changed, 62 insertions(+), 123 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 040eeda8426f..ab90998c1867 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -246,7 +246,7 @@ xfs_bmap_get_bp(
> struct xfs_btree_cur *cur,
> xfs_fsblock_t bno)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> int i;
>
> if (!cur)
> @@ -260,9 +260,9 @@ xfs_bmap_get_bp(
> }
>
> /* Chase down all the log items to see if the bp is there */
> - list_for_each_entry(lidp, &cur->bc_tp->t_items, lid_trans) {
> - struct xfs_buf_log_item *bip;
> - bip = (struct xfs_buf_log_item *)lidp->lid_item;
> + list_for_each_entry(lip, &cur->bc_tp->t_items, li_trans) {
> + struct xfs_buf_log_item *bip = (struct xfs_buf_log_item *)lip;
> +
> if (bip->bli_item.li_type == XFS_LI_BUF &&
> XFS_BUF_ADDR(bip->bli_buf) == bno)
> return bip->bli_buf;
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index d0b84da0cb1e..8efc06e62b13 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -57,21 +57,6 @@ extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
> extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> extern const struct xfs_buf_ops xfs_rtbuf_ops;
>
> -/*
> - * This structure is used to track log items associated with
> - * a transaction. It points to the log item and keeps some
> - * flags to track the state of the log item. It also tracks
> - * the amount of space needed to log the item it describes
> - * once we get to commit processing (see xfs_trans_commit()).
> - */
> -struct xfs_log_item_desc {
> - struct xfs_log_item *lid_item;
> - struct list_head lid_trans;
> - unsigned char lid_flags;
> -};
> -
> -#define XFS_LID_DIRTY 0x1
> -
> /* log size calculation functions */
> int xfs_log_calc_unit_res(struct xfs_mount *mp, int unit_bytes);
> int xfs_log_calc_minimum_size(struct xfs_mount *);
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 8d6ed045b643..c2311379d1c3 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -438,7 +438,7 @@ xfs_buf_item_unpin(
> * xfs_trans_uncommit() will try to reference the
> * buffer which we no longer have a hold on.
> */
> - if (lip->li_desc)
> + if (!list_empty(&lip->li_trans))
> xfs_trans_del_item(lip);
>
> /*
> diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
> index a99a0f8aa528..5da9599156ed 100644
> --- a/fs/xfs/xfs_icreate_item.c
> +++ b/fs/xfs/xfs_icreate_item.c
> @@ -184,5 +184,5 @@ xfs_icreate_log(
>
> xfs_trans_add_item(tp, &icp->ic_item);
> tp->t_flags |= XFS_TRANS_DIRTY;
> - icp->ic_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
> }
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index e427864434c1..c21039f27e39 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1047,6 +1047,7 @@ xfs_log_item_init(
> INIT_LIST_HEAD(&item->li_ail);
> INIT_LIST_HEAD(&item->li_cil);
> INIT_LIST_HEAD(&item->li_bio_list);
> + INIT_LIST_HEAD(&item->li_trans);
> }
>
> /*
> @@ -2110,10 +2111,10 @@ xlog_print_tic_res(
> */
> void
> xlog_print_trans(
> - struct xfs_trans *tp)
> + struct xfs_trans *tp)
> {
> - struct xfs_mount *mp = tp->t_mountp;
> - struct xfs_log_item_desc *lidp;
> + struct xfs_mount *mp = tp->t_mountp;
> + struct xfs_log_item *lip;
>
> /* dump core transaction and ticket info */
> xfs_warn(mp, "transaction summary:");
> @@ -2124,8 +2125,7 @@ xlog_print_trans(
> xlog_print_tic_res(mp, tp->t_ticket);
>
> /* dump each log item */
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv = lip->li_lv;
> struct xfs_log_iovec *vec;
> int i;
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index a8675c631438..c15687724728 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -141,10 +141,9 @@ xlog_cil_alloc_shadow_bufs(
> struct xlog *log,
> struct xfs_trans *tp)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv;
> int niovecs = 0;
> int nbytes = 0;
> @@ -152,7 +151,7 @@ xlog_cil_alloc_shadow_bufs(
> bool ordered = false;
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /* get number of vecs and size of data to be stored */
> @@ -317,7 +316,7 @@ xlog_cil_insert_format_items(
> int *diff_len,
> int *diff_iovecs)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
>
> /* Bail out if we didn't find a log item. */
> @@ -326,15 +325,14 @@ xlog_cil_insert_format_items(
> return;
> }
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> struct xfs_log_vec *lv;
> struct xfs_log_vec *old_lv = NULL;
> struct xfs_log_vec *shadow;
> bool ordered = false;
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /*
> @@ -406,7 +404,7 @@ xlog_cil_insert_items(
> {
> struct xfs_cil *cil = log->l_cilp;
> struct xfs_cil_ctx *ctx = cil->xc_ctx;
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> int len = 0;
> int diff_iovecs = 0;
> int iclog_space;
> @@ -479,11 +477,10 @@ xlog_cil_insert_items(
> * We do this here so we only need to take the CIL lock once during
> * the transaction commit.
> */
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
>
> /* Skip items which aren't dirty in this transaction. */
> - if (!(lidp->lid_flags & XFS_LID_DIRTY))
> + if (!test_bit(XFS_LI_DIRTY, &lip->li_flags))
> continue;
>
> /*
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d71424052917..5726ef496980 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1880,11 +1880,6 @@ xfs_init_zones(void)
> if (!xfs_trans_zone)
> goto out_destroy_ifork_zone;
>
> - xfs_log_item_desc_zone =
> - kmem_zone_init(sizeof(struct xfs_log_item_desc),
> - "xfs_log_item_desc");
> - if (!xfs_log_item_desc_zone)
> - goto out_destroy_trans_zone;
>
> /*
> * The size of the zone allocated buf log item is the maximum
> @@ -1894,7 +1889,7 @@ xfs_init_zones(void)
> xfs_buf_item_zone = kmem_zone_init(sizeof(struct xfs_buf_log_item),
> "xfs_buf_item");
> if (!xfs_buf_item_zone)
> - goto out_destroy_log_item_desc_zone;
> + goto out_destroy_trans_zone;
>
> xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
> ((XFS_EFD_MAX_FAST_EXTENTS - 1) *
> @@ -1982,8 +1977,6 @@ xfs_init_zones(void)
> kmem_zone_destroy(xfs_efd_zone);
> out_destroy_buf_item_zone:
> kmem_zone_destroy(xfs_buf_item_zone);
> - out_destroy_log_item_desc_zone:
> - kmem_zone_destroy(xfs_log_item_desc_zone);
> out_destroy_trans_zone:
> kmem_zone_destroy(xfs_trans_zone);
> out_destroy_ifork_zone:
> @@ -2022,7 +2015,6 @@ xfs_destroy_zones(void)
> kmem_zone_destroy(xfs_efi_zone);
> kmem_zone_destroy(xfs_efd_zone);
> kmem_zone_destroy(xfs_buf_item_zone);
> - kmem_zone_destroy(xfs_log_item_desc_zone);
> kmem_zone_destroy(xfs_trans_zone);
> kmem_zone_destroy(xfs_ifork_zone);
> kmem_zone_destroy(xfs_da_state_zone);
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 947ab6e32c18..28a5b3f876ad 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -441,7 +441,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __field(unsigned, bli_recur)
> __field(int, bli_refcount)
> __field(unsigned, bli_flags)
> - __field(void *, li_desc)
> __field(unsigned long, li_flags)
> ),
> TP_fast_assign(
> @@ -455,12 +454,11 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __entry->buf_hold = atomic_read(&bip->bli_buf->b_hold);
> __entry->buf_pincount = atomic_read(&bip->bli_buf->b_pin_count);
> __entry->buf_lockval = bip->bli_buf->b_sema.count;
> - __entry->li_desc = bip->bli_item.li_desc;
> __entry->li_flags = bip->bli_item.li_flags;
> ),
> TP_printk("dev %d:%d bno 0x%llx len 0x%zx hold %d pincount %d "
> "lock %d flags %s recur %d refcount %d bliflags %s "
> - "lidesc %p liflags %s",
> + "liflags %s",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> (unsigned long long)__entry->buf_bno,
> __entry->buf_len,
> @@ -471,7 +469,6 @@ DECLARE_EVENT_CLASS(xfs_buf_item_class,
> __entry->bli_recur,
> __entry->bli_refcount,
> __print_flags(__entry->bli_flags, "|", XFS_BLI_FLAGS),
> - __entry->li_desc,
> __print_flags(__entry->li_flags, "|", XFS_LI_FLAGS))
> )
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 24744915357b..477c12412d31 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -33,7 +33,6 @@
> #include "xfs_error.h"
>
> kmem_zone_t *xfs_trans_zone;
> -kmem_zone_t *xfs_log_item_desc_zone;
>
> #if defined(CONFIG_TRACEPOINTS)
> static void
> @@ -732,77 +731,52 @@ xfs_trans_unreserve_and_mod_sb(
> return;
> }
>
> -/*
> - * Add the given log item to the transaction's list of log items.
> - *
> - * The log item will now point to its new descriptor with its li_desc field.
> - */
> +/* Add the given log item to the transaction's list of log items. */
> void
> xfs_trans_add_item(
> struct xfs_trans *tp,
> struct xfs_log_item *lip)
> {
> - struct xfs_log_item_desc *lidp;
> -
> ASSERT(lip->li_mountp == tp->t_mountp);
> ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> + ASSERT(list_empty(&lip->li_trans));
> + ASSERT(!test_bit(XFS_LI_DIRTY, &lip->li_flags));
>
> - lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
> -
> - lidp->lid_item = lip;
> - lidp->lid_flags = 0;
> - list_add_tail(&lidp->lid_trans, &tp->t_items);
> -
> - lip->li_desc = lidp;
> -
> + list_add_tail(&lip->li_trans, &tp->t_items);
> trace_xfs_trans_add_item(tp, _RET_IP_);
> }
>
> -STATIC void
> -xfs_trans_free_item_desc(
> - struct xfs_log_item_desc *lidp)
> -{
> - list_del_init(&lidp->lid_trans);
> - kmem_zone_free(xfs_log_item_desc_zone, lidp);
> -}
> -
> /*
> - * Unlink and free the given descriptor.
> + * Unlink the log item from the transaction. the log item is no longer
> + * considered dirty in this transaction, as the linked transaction has
> + * finished, either by abort or commit completion.
> */
> void
> xfs_trans_del_item(
> struct xfs_log_item *lip)
> {
> - xfs_trans_free_item_desc(lip->li_desc);
> - lip->li_desc = NULL;
> + clear_bit(XFS_LI_DIRTY, &lip->li_flags);
> + list_del_init(&lip->li_trans);
> }
>
> -/*
> - * Unlock all of the items of a transaction and free all the descriptors
> - * of that transaction.
> - */
> +/* Detach and unlock all of the items in a transaction */
> void
> xfs_trans_free_items(
> struct xfs_trans *tp,
> xfs_lsn_t commit_lsn,
> bool abort)
> {
> - struct xfs_log_item_desc *lidp, *next;
> + struct xfs_log_item *lip, *next;
>
> trace_xfs_trans_free_items(tp, _RET_IP_);
>
> - list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
> - struct xfs_log_item *lip = lidp->lid_item;
> -
> - lip->li_desc = NULL;
> -
> + list_for_each_entry_safe(lip, next, &tp->t_items, li_trans) {
> + xfs_trans_del_item(lip);
> if (commit_lsn != NULLCOMMITLSN)
> lip->li_ops->iop_committing(lip, commit_lsn);
> if (abort)
> set_bit(XFS_LI_ABORTED, &lip->li_flags);
> lip->li_ops->iop_unlock(lip);
> -
> - xfs_trans_free_item_desc(lidp);
> }
> }
>
> @@ -1047,10 +1021,10 @@ xfs_trans_cancel(
> }
> #ifdef DEBUG
> if (!dirty && !XFS_FORCED_SHUTDOWN(mp)) {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans)
> - ASSERT(!(lidp->lid_item->li_type == XFS_LI_EFD));
> + list_for_each_entry(lip, &tp->t_items, li_trans)
> + ASSERT(!(lip->li_type == XFS_LI_EFD));
> }
> #endif
> xfs_trans_unreserve_and_mod_sb(tp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 51145ddf7e5b..a32da7d7ea97 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -27,7 +27,6 @@ struct xfs_efi_log_item;
> struct xfs_inode;
> struct xfs_item_ops;
> struct xfs_log_iovec;
> -struct xfs_log_item_desc;
> struct xfs_mount;
> struct xfs_trans;
> struct xfs_trans_res;
> @@ -43,8 +42,8 @@ struct xfs_bud_log_item;
>
> typedef struct xfs_log_item {
> struct list_head li_ail; /* AIL pointers */
> + struct list_head li_trans; /* transaction list */
> xfs_lsn_t li_lsn; /* last on-disk lsn */
> - struct xfs_log_item_desc *li_desc; /* ptr to current desc*/
> struct xfs_mount *li_mountp; /* ptr to fs mount */
> struct xfs_ail *li_ailp; /* ptr to AIL */
> uint li_type; /* item type */
> @@ -72,11 +71,13 @@ typedef struct xfs_log_item {
> #define XFS_LI_IN_AIL 0
> #define XFS_LI_ABORTED 1
> #define XFS_LI_FAILED 2
> +#define XFS_LI_DIRTY 3 /* log item dirty in transaction */
>
> #define XFS_LI_FLAGS \
> { (1 << XFS_LI_IN_AIL), "IN_AIL" }, \
> { (1 << XFS_LI_ABORTED), "ABORTED" }, \
> - { (1 << XFS_LI_FAILED), "FAILED" }
> + { (1 << XFS_LI_FAILED), "FAILED" }, \
> + { (1 << XFS_LI_DIRTY), "DIRTY" }
>
> struct xfs_item_ops {
> void (*iop_size)(xfs_log_item_t *, int *, int *);
> @@ -247,7 +248,6 @@ void xfs_trans_buf_copy_type(struct xfs_buf *dst_bp,
> struct xfs_buf *src_bp);
>
> extern kmem_zone_t *xfs_trans_zone;
> -extern kmem_zone_t *xfs_log_item_desc_zone;
>
> /* rmap updates */
> enum xfs_rmap_intent_type;
> diff --git a/fs/xfs/xfs_trans_bmap.c b/fs/xfs/xfs_trans_bmap.c
> index 14543d93cd4b..230a21df4b12 100644
> --- a/fs/xfs/xfs_trans_bmap.c
> +++ b/fs/xfs/xfs_trans_bmap.c
> @@ -79,7 +79,7 @@ xfs_trans_log_finish_bmap_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - budp->bud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &budp->bud_item.li_flags);
>
> return error;
> }
> @@ -158,7 +158,7 @@ xfs_bmap_update_log_item(
> bmap = container_of(item, struct xfs_bmap_intent, bi_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - buip->bui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &buip->bui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 0081e9b3decf..a8ddb4eed279 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -40,7 +40,7 @@ xfs_trans_buf_item_match(
> struct xfs_buf_map *map,
> int nmaps)
> {
> - struct xfs_log_item_desc *lidp;
> + struct xfs_log_item *lip;
> struct xfs_buf_log_item *blip;
> int len = 0;
> int i;
> @@ -48,8 +48,8 @@ xfs_trans_buf_item_match(
> for (i = 0; i < nmaps; i++)
> len += map[i].bm_len;
>
> - list_for_each_entry(lidp, &tp->t_items, lid_trans) {
> - blip = (struct xfs_buf_log_item *)lidp->lid_item;
> + list_for_each_entry(lip, &tp->t_items, li_trans) {
> + blip = (struct xfs_buf_log_item *)lip;
> if (blip->bli_item.li_type == XFS_LI_BUF &&
> blip->bli_buf->b_target == target &&
> XFS_BUF_ADDR(blip->bli_buf) == map[0].bm_bn &&
> @@ -100,14 +100,10 @@ _xfs_trans_bjoin(
> atomic_inc(&bip->bli_refcount);
>
> /*
> - * Get a log_item_desc to point at the new item.
> + * Attach the item to the transaction so we can find it in
> + * xfs_trans_get_buf() and friends.
> */
> xfs_trans_add_item(tp, &bip->bli_item);
> -
> - /*
> - * Initialize b_fsprivate2 so we can find it with incore_match()
> - * in xfs_trans_get_buf() and friends above.
> - */
> bp->b_transp = tp;
>
> }
> @@ -391,7 +387,7 @@ xfs_trans_brelse(
> * If the buffer is dirty within this transaction, we can't
> * release it until we commit.
> */
> - if (bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY)
> + if (test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags))
> return;
>
> /*
> @@ -542,7 +538,7 @@ xfs_trans_dirty_buf(
> bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> }
>
> /*
> @@ -626,7 +622,7 @@ xfs_trans_binval(
> ASSERT(!(bip->__bli_format.blf_flags & XFS_BLF_INODE_BUF));
> ASSERT(!(bip->__bli_format.blf_flags & XFS_BLFT_MASK));
> ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
> - ASSERT(bip->bli_item.li_desc->lid_flags & XFS_LID_DIRTY);
> + ASSERT(test_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags));
> ASSERT(tp->t_flags & XFS_TRANS_DIRTY);
> return;
> }
> @@ -642,7 +638,7 @@ xfs_trans_binval(
> memset(bip->bli_formats[i].blf_data_map, 0,
> (bip->bli_formats[i].blf_map_size * sizeof(uint)));
> }
> - bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
> tp->t_flags |= XFS_TRANS_DIRTY;
> }
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index c3d547211d16..c381c02cca45 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -77,7 +77,7 @@ xfs_trans_log_dquot(
> ASSERT(XFS_DQ_IS_LOCKED(dqp));
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - dqp->q_logitem.qli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &dqp->q_logitem.qli_item.li_flags);
> }
>
> /*
> @@ -879,7 +879,7 @@ xfs_trans_log_quotaoff_item(
> xfs_qoff_logitem_t *qlp)
> {
> tp->t_flags |= XFS_TRANS_DIRTY;
> - qlp->qql_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &qlp->qql_item.li_flags);
> }
>
> STATIC void
> diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> index ab438647592a..fb631081d829 100644
> --- a/fs/xfs/xfs_trans_extfree.c
> +++ b/fs/xfs/xfs_trans_extfree.c
> @@ -90,7 +90,7 @@ xfs_trans_free_extent(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
>
> next_extent = efdp->efd_next_extent;
> ASSERT(next_extent < efdp->efd_format.efd_nextents);
> @@ -155,7 +155,7 @@ xfs_extent_free_log_item(
> free = container_of(item, struct xfs_extent_free_item, xefi_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &efip->efi_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> index 07cea592dc01..f7bd7960a90f 100644
> --- a/fs/xfs/xfs_trans_inode.c
> +++ b/fs/xfs/xfs_trans_inode.c
> @@ -133,14 +133,13 @@ xfs_trans_log_inode(
> * set however, then go ahead and bump the i_version counter
> * unconditionally.
> */
> - if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) &&
> + if (!test_and_set_bit(XFS_LI_DIRTY, &ip->i_itemp->ili_item.li_flags) &&
> IS_I_VERSION(VFS_I(ip))) {
> if (inode_maybe_inc_iversion(VFS_I(ip), flags & XFS_ILOG_CORE))
> flags |= XFS_ILOG_CORE;
> }
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - ip->i_itemp->ili_item.li_desc->lid_flags |= XFS_LID_DIRTY;
>
> /*
> * Always OR in the bits from the ili_last_fields field.
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 43f773297b9d..9717ae74b36d 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -19,7 +19,6 @@
> #define __XFS_TRANS_PRIV_H__
>
> struct xfs_log_item;
> -struct xfs_log_item_desc;
> struct xfs_mount;
> struct xfs_trans;
> struct xfs_ail;
> diff --git a/fs/xfs/xfs_trans_refcount.c b/fs/xfs/xfs_trans_refcount.c
> index 94c1877af834..c7f8e82f5bda 100644
> --- a/fs/xfs/xfs_trans_refcount.c
> +++ b/fs/xfs/xfs_trans_refcount.c
> @@ -77,7 +77,7 @@ xfs_trans_log_finish_refcount_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - cudp->cud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &cudp->cud_item.li_flags);
>
> return error;
> }
> @@ -154,7 +154,7 @@ xfs_refcount_update_log_item(
> refc = container_of(item, struct xfs_refcount_intent, ri_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - cuip->cui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &cuip->cui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> diff --git a/fs/xfs/xfs_trans_rmap.c b/fs/xfs/xfs_trans_rmap.c
> index 9b577beb43d7..5831ca0c270b 100644
> --- a/fs/xfs/xfs_trans_rmap.c
> +++ b/fs/xfs/xfs_trans_rmap.c
> @@ -117,7 +117,7 @@ xfs_trans_log_finish_rmap_update(
> * 2.) shuts down the filesystem
> */
> tp->t_flags |= XFS_TRANS_DIRTY;
> - rudp->rud_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &rudp->rud_item.li_flags);
>
> return error;
> }
> @@ -175,7 +175,7 @@ xfs_rmap_update_log_item(
> rmap = container_of(item, struct xfs_rmap_intent, ri_list);
>
> tp->t_flags |= XFS_TRANS_DIRTY;
> - ruip->rui_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> + set_bit(XFS_LI_DIRTY, &ruip->rui_item.li_flags);
>
> /*
> * atomic_inc_return gives us the value after the increment;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-09 15:02 ` Darrick J. Wong
@ 2018-05-11 2:04 ` Dave Chinner
2018-05-11 13:24 ` Brian Foster
0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-11 2:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, linux-xfs
On Wed, May 09, 2018 at 08:02:38AM -0700, Darrick J. Wong wrote:
> On Wed, May 09, 2018 at 06:10:42AM -0400, Brian Foster wrote:
> > On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> > > On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > > > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > >
> > > > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > > > into a transaction it is already joined to. This means the inode can
> > > > > have multiple log item descriptors attached to the transaction for
> > > > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > > > between the log item and log item descriptor.
> > > > >
> > > > > This results in the log item being processed twice during
> > > > > transaction commit and CIL formatting, and there are lots of other
> > > > > potential issues tha arise from double processing of log items in
> > > > > the transaction commit state machine.
> > > > >
> > > > > In this case, the inode is already held by the rolling transaction
> > > > > returned from xfs_defer_finish(), so there's no need to join it
> > > > > again.
> > > > >
> > > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > ---
> > > > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > > > index 5b66ac12913c..27870e5cd259 100644
> > > > > --- a/fs/xfs/xfs_symlink.c
> > > > > +++ b/fs/xfs/xfs_symlink.c
> > > > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > > > error = xfs_defer_finish(&tp, &dfops);
> > > > > if (error)
> > > > > goto error_bmap_cancel;
> > > > > - /*
> > > > > - * The first xact was committed, so add the inode to the new one.
> > > > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > > > - * part of every commit.
> > > > > - */
> > > > > - xfs_trans_ijoin(tp, ip, 0);
> > > > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > +
> > > > > /*
> > > > > * Commit the transaction containing extent freeing and EFDs.
> > > > > */
> > > > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > >
> > > > Seems fine.. but do we even need this call? We're about to commit the
> > > > transaction and unlock the inode...
> > >
> > > Yes, I think we do. We need it to be committed in each of the
> > > rolling transactions so that the inode doesn't get written/replayed
> > > before any of the other dependent metadata changes in this final
> > > transaction.
> > >
> >
> > Hmm, I don't follow what that means. IIUC the act of logging it again
> > simply moves it forward in the log. That makes sense down in the dfops
> > code but seems unecessary here given that we are about to complete the
> > chain of transactions.
> >
> > xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
> > remote bufs, joins the inode to the dfops and finishes the dfops. We
> > return from xfs_defer_finish() having committed the (still locked) inode
> > modifications and have a new/rolled transaction that covers the free of
> > the associated blocks (EFDs). I could certainly be missing something,
> > but from that point what difference does it make whether the final
> > transaction relogs the inode before it commits?
It ensures the inode changes are sanely ordered. i.e. we don't
write the inode to disk before all the other changes made in the
rolling transaction are written to disk. And the same goes for
recovery.
> > The in-core inode is
> > still locked and the previous inode modifications may very well already
> > be in the on-disk log.
> >
> > That said, it seems harmless despite not understanding what it's for. So
> > with or without the xfs_trans_log_inode() call:
>
> <shrug> I interpreted this as Dave being fastidious about relogging
> inodes after every transaction roll even if it's not strictly necessary
> (but not otherwise harmful)
That's wrong. It is a requirement of rolling transactions that we
log every object that is held locked across xfs_trans_commit()
regardless of whether it was dirtied in that specific transaction or
not.
That's because we call xfs_trans_reserve() when rolling the
transaction, and that means the locked objects cannot be written to
disk during that rolling transaction sequence. That means we can
deadlock if the object we hold locked pins the tail of the log and
there isn't space for the re-reservation of log space for the next
transaction in the sequence.
IOWs, the simple rule of thumb is that objects held locked across
xfs_trans_commit() should be logged in that transaction. Follow that
simple rule everywhere, and we don't leave nasty landmines around
to step on when we change code around....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-11 2:04 ` Dave Chinner
@ 2018-05-11 13:24 ` Brian Foster
2018-05-12 2:00 ` Dave Chinner
0 siblings, 1 reply; 35+ messages in thread
From: Brian Foster @ 2018-05-11 13:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, linux-xfs
On Fri, May 11, 2018 at 12:04:25PM +1000, Dave Chinner wrote:
> On Wed, May 09, 2018 at 08:02:38AM -0700, Darrick J. Wong wrote:
> > On Wed, May 09, 2018 at 06:10:42AM -0400, Brian Foster wrote:
> > > On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> > > > On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > > > > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > >
> > > > > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > > > > into a transaction it is already joined to. This means the inode can
> > > > > > have multiple log item descriptors attached to the transaction for
> > > > > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > > > > between the log item and log item descriptor.
> > > > > >
> > > > > > This results in the log item being processed twice during
> > > > > > transaction commit and CIL formatting, and there are lots of other
> > > > > > potential issues tha arise from double processing of log items in
> > > > > > the transaction commit state machine.
> > > > > >
> > > > > > In this case, the inode is already held by the rolling transaction
> > > > > > returned from xfs_defer_finish(), so there's no need to join it
> > > > > > again.
> > > > > >
> > > > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > > ---
> > > > > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > > > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > > > > index 5b66ac12913c..27870e5cd259 100644
> > > > > > --- a/fs/xfs/xfs_symlink.c
> > > > > > +++ b/fs/xfs/xfs_symlink.c
> > > > > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > > > > error = xfs_defer_finish(&tp, &dfops);
> > > > > > if (error)
> > > > > > goto error_bmap_cancel;
> > > > > > - /*
> > > > > > - * The first xact was committed, so add the inode to the new one.
> > > > > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > > > > - * part of every commit.
> > > > > > - */
> > > > > > - xfs_trans_ijoin(tp, ip, 0);
> > > > > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > > +
> > > > > > /*
> > > > > > * Commit the transaction containing extent freeing and EFDs.
> > > > > > */
> > > > > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > >
> > > > > Seems fine.. but do we even need this call? We're about to commit the
> > > > > transaction and unlock the inode...
> > > >
> > > > Yes, I think we do. We need it to be committed in each of the
> > > > rolling transactions so that the inode doesn't get written/replayed
> > > > before any of the other dependent metadata changes in this final
> > > > transaction.
> > > >
> > >
> > > Hmm, I don't follow what that means. IIUC the act of logging it again
> > > simply moves it forward in the log. That makes sense down in the dfops
> > > code but seems unecessary here given that we are about to complete the
> > > chain of transactions.
> > >
> > > xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
> > > remote bufs, joins the inode to the dfops and finishes the dfops. We
> > > return from xfs_defer_finish() having committed the (still locked) inode
> > > modifications and have a new/rolled transaction that covers the free of
> > > the associated blocks (EFDs). I could certainly be missing something,
> > > but from that point what difference does it make whether the final
> > > transaction relogs the inode before it commits?
>
> It ensures the inode changes are sanely ordered. i.e. we don't
> write the inode to disk before all the other changes made in the
> rolling transaction are written to disk. And the same goes for
> recovery.
>
But what exactly does that accomplish? The inode changes, block unmap
and and EFIs are all logged in the same transaction. Given the inode
remains locked, what difference does it make whether it is relogged in
the transaction that processes the EFIs? IOW, what can go wrong here
without the additional inode relog?
Also, what do you mean by "sanely ordered?" AFAICT, there is no such
writeback ordering guarantee even if the log items are part of the same
transaction. Suppose we return from xfs_inactive_symlink_rmt(),
everything is committed/unlocked and an AIL push occurs. Nothing
prevents a completely unrelated transaction from locking those same
allocation btree buffers that were in the same (rolled) transaction as
the EFD (plus relogged inode) before xfsaild gets to them, which means
the associated items could be written back in arbitrary order anyways.
I think that unrelated transaction could even relog those allocbt
buffers, which means technically the inode and final written back buffer
ended up in different checkpoints (or the inode was written back and
only the buffer changes are in the log after a crash, etc.). But that's
probably getting too far into the weeds..
> > > The in-core inode is
> > > still locked and the previous inode modifications may very well already
> > > be in the on-disk log.
> > >
> > > That said, it seems harmless despite not understanding what it's for. So
> > > with or without the xfs_trans_log_inode() call:
> >
> > <shrug> I interpreted this as Dave being fastidious about relogging
> > inodes after every transaction roll even if it's not strictly necessary
> > (but not otherwise harmful)
>
> That's wrong. It is a requirement of rolling transactions that we
> log every object that is held locked across xfs_trans_commit()
> regardless of whether it was dirtied in that specific transaction or
> not.
>
> That's because we call xfs_trans_reserve() when rolling the
> transaction, and that means the locked objects cannot be written to
> disk during that rolling transaction sequence. That means we can
> deadlock if the object we hold locked pins the tail of the log and
> there isn't space for the re-reservation of log space for the next
> transaction in the sequence.
>
Right.. this is my understanding for why we relog items in rolling
transactions. I think that in this case the log is pinned regardless of
the inode because the first transaction commits an EFI.
(Which has me wondering a bit over whether we even need to join the
inode to the dfops as opposed to transfer the ilock to the transaction,
but that's another train of thought..).
> IOWs, the simple rule of thumb is that objects held locked across
> xfs_trans_commit() should be logged in that transaction. Follow that
> simple rule everywhere, and we don't leave nasty landmines around
> to step on when we change code around....
>
I'm all for following simple guidelines as such to reduce the likelihood
or impact of landmines. That's a different argument from the above
though. I'd at least like to clear up whether I'm missing some reason
for the extra relog being required or if we're just trying to keep
things simple/safe. Hm?
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-11 13:24 ` Brian Foster
@ 2018-05-12 2:00 ` Dave Chinner
2018-05-12 14:17 ` Brian Foster
0 siblings, 1 reply; 35+ messages in thread
From: Dave Chinner @ 2018-05-12 2:00 UTC (permalink / raw)
To: Brian Foster; +Cc: Darrick J. Wong, linux-xfs
On Fri, May 11, 2018 at 09:24:49AM -0400, Brian Foster wrote:
> On Fri, May 11, 2018 at 12:04:25PM +1000, Dave Chinner wrote:
> > On Wed, May 09, 2018 at 08:02:38AM -0700, Darrick J. Wong wrote:
> > > On Wed, May 09, 2018 at 06:10:42AM -0400, Brian Foster wrote:
> > > > On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> > > > > On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > > > > > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > >
> > > > > > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > > > > > into a transaction it is already joined to. This means the inode can
> > > > > > > have multiple log item descriptors attached to the transaction for
> > > > > > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > > > > > between the log item and log item descriptor.
> > > > > > >
> > > > > > > This results in the log item being processed twice during
> > > > > > > transaction commit and CIL formatting, and there are lots of other
> > > > > > > potential issues tha arise from double processing of log items in
> > > > > > > the transaction commit state machine.
> > > > > > >
> > > > > > > In this case, the inode is already held by the rolling transaction
> > > > > > > returned from xfs_defer_finish(), so there's no need to join it
> > > > > > > again.
> > > > > > >
> > > > > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > > > ---
> > > > > > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > > > > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > > > > > index 5b66ac12913c..27870e5cd259 100644
> > > > > > > --- a/fs/xfs/xfs_symlink.c
> > > > > > > +++ b/fs/xfs/xfs_symlink.c
> > > > > > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > > > > > error = xfs_defer_finish(&tp, &dfops);
> > > > > > > if (error)
> > > > > > > goto error_bmap_cancel;
> > > > > > > - /*
> > > > > > > - * The first xact was committed, so add the inode to the new one.
> > > > > > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > > > > > - * part of every commit.
> > > > > > > - */
> > > > > > > - xfs_trans_ijoin(tp, ip, 0);
> > > > > > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > > > +
> > > > > > > /*
> > > > > > > * Commit the transaction containing extent freeing and EFDs.
> > > > > > > */
> > > > > > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > >
> > > > > > Seems fine.. but do we even need this call? We're about to commit the
> > > > > > transaction and unlock the inode...
> > > > >
> > > > > Yes, I think we do. We need it to be committed in each of the
> > > > > rolling transactions so that the inode doesn't get written/replayed
> > > > > before any of the other dependent metadata changes in this final
> > > > > transaction.
> > > > >
> > > >
> > > > Hmm, I don't follow what that means. IIUC the act of logging it again
> > > > simply moves it forward in the log. That makes sense down in the dfops
> > > > code but seems unecessary here given that we are about to complete the
> > > > chain of transactions.
> > > >
> > > > xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
> > > > remote bufs, joins the inode to the dfops and finishes the dfops. We
> > > > return from xfs_defer_finish() having committed the (still locked) inode
> > > > modifications and have a new/rolled transaction that covers the free of
> > > > the associated blocks (EFDs). I could certainly be missing something,
> > > > but from that point what difference does it make whether the final
> > > > transaction relogs the inode before it commits?
> >
> > It ensures the inode changes are sanely ordered. i.e. we don't
> > write the inode to disk before all the other changes made in the
> > rolling transaction are written to disk. And the same goes for
> > recovery.
> >
>
> But what exactly does that accomplish? The inode changes, block unmap
> and and EFIs are all logged in the same transaction. Given the inode
> remains locked, what difference does it make whether it is relogged in
> the transaction that processes the EFIs? IOW, what can go wrong here
> without the additional inode relog?
>
> Also, what do you mean by "sanely ordered?" AFAICT, there is no such
> writeback ordering guarantee even if the log items are part of the same
> transaction.
The rolling tranactions can be committed can be in different
checkpoints. If the journal is commited after the last roll but
before the final commit, then the inode is unpinned before it is
unlocked, while everything in the current transaction is still
pinned in memory. When the last transaction commits, the inode gets
unlocked and can be written back before the remaining operations in
the rolling transaction are committed to stable storage.
i.e. we end up with an inode on disk with a younger LSN in it than a
bunch of it's dependent changes have. Strictly speaking, that's an
on-disk ordering violation and potentially an on-disk transactional
change atomicity violation. We don't get a violation in memory
because of the inode locking being held across the transaction
commit, but we do get one on disk because the inode is unpinned in
memory before all it's dependent changes are in stable journal
storage....
Hence by logging the inode in the final transaction, we ensure it
stays pinned in memory until the entire transaction is stable in the
journal and we've guaranteed the on disk ordering and atomicity
matches what is provided by the in-memory locking....
> Suppose we return from xfs_inactive_symlink_rmt(),
> everything is committed/unlocked and an AIL push occurs.
All the objects are still pinned in memory, so they can't be written
back by the AIL until the journal commits. :P
> Nothing
> prevents a completely unrelated transaction from locking those same
> allocation btree buffers that were in the same (rolled) transaction as
> the EFD (plus relogged inode) before xfsaild gets to them, which means
> the associated items could be written back in arbitrary order anyways.
Sure, but the combination of locking and journal ordering via
relogging in the CIL handles reuse via independent atomic
transaction cases just fine. That's different to ordering within a
a single atomic transaction chain....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt()
2018-05-12 2:00 ` Dave Chinner
@ 2018-05-12 14:17 ` Brian Foster
0 siblings, 0 replies; 35+ messages in thread
From: Brian Foster @ 2018-05-12 14:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, linux-xfs
On Sat, May 12, 2018 at 12:00:07PM +1000, Dave Chinner wrote:
> On Fri, May 11, 2018 at 09:24:49AM -0400, Brian Foster wrote:
> > On Fri, May 11, 2018 at 12:04:25PM +1000, Dave Chinner wrote:
> > > On Wed, May 09, 2018 at 08:02:38AM -0700, Darrick J. Wong wrote:
> > > > On Wed, May 09, 2018 at 06:10:42AM -0400, Brian Foster wrote:
> > > > > On Wed, May 09, 2018 at 10:24:28AM +1000, Dave Chinner wrote:
> > > > > > On Tue, May 08, 2018 at 10:18:11AM -0400, Brian Foster wrote:
> > > > > > > On Tue, May 08, 2018 at 01:41:58PM +1000, Dave Chinner wrote:
> > > > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > > >
> > > > > > > > xfs_inactive_symlink_rmt() does something nasty - it joins an inode
> > > > > > > > into a transaction it is already joined to. This means the inode can
> > > > > > > > have multiple log item descriptors attached to the transaction for
> > > > > > > > it. This breaks teh 1:1 mapping that is supposed to exist
> > > > > > > > between the log item and log item descriptor.
> > > > > > > >
> > > > > > > > This results in the log item being processed twice during
> > > > > > > > transaction commit and CIL formatting, and there are lots of other
> > > > > > > > potential issues tha arise from double processing of log items in
> > > > > > > > the transaction commit state machine.
> > > > > > > >
> > > > > > > > In this case, the inode is already held by the rolling transaction
> > > > > > > > returned from xfs_defer_finish(), so there's no need to join it
> > > > > > > > again.
> > > > > > > >
> > > > > > > > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > > > > ---
> > > > > > > > fs/xfs/xfs_symlink.c | 9 ++-------
> > > > > > > > 1 file changed, 2 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > > > > > > > index 5b66ac12913c..27870e5cd259 100644
> > > > > > > > --- a/fs/xfs/xfs_symlink.c
> > > > > > > > +++ b/fs/xfs/xfs_symlink.c
> > > > > > > > @@ -488,16 +488,11 @@ xfs_inactive_symlink_rmt(
> > > > > > > > error = xfs_defer_finish(&tp, &dfops);
> > > > > > > > if (error)
> > > > > > > > goto error_bmap_cancel;
> > > > > > > > - /*
> > > > > > > > - * The first xact was committed, so add the inode to the new one.
> > > > > > > > - * Mark it dirty so it will be logged and moved forward in the log as
> > > > > > > > - * part of every commit.
> > > > > > > > - */
> > > > > > > > - xfs_trans_ijoin(tp, ip, 0);
> > > > > > > > - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > > > > +
> > > > > > > > /*
> > > > > > > > * Commit the transaction containing extent freeing and EFDs.
> > > > > > > > */
> > > > > > > > + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> > > > > > >
> > > > > > > Seems fine.. but do we even need this call? We're about to commit the
> > > > > > > transaction and unlock the inode...
> > > > > >
> > > > > > Yes, I think we do. We need it to be committed in each of the
> > > > > > rolling transactions so that the inode doesn't get written/replayed
> > > > > > before any of the other dependent metadata changes in this final
> > > > > > transaction.
> > > > > >
> > > > >
> > > > > Hmm, I don't follow what that means. IIUC the act of logging it again
> > > > > simply moves it forward in the log. That makes sense down in the dfops
> > > > > code but seems unecessary here given that we are about to complete the
> > > > > chain of transactions.
> > > > >
> > > > > xfs_inactive_symlink_rmt() makes changes to the inode, invals/unmaps the
> > > > > remote bufs, joins the inode to the dfops and finishes the dfops. We
> > > > > return from xfs_defer_finish() having committed the (still locked) inode
> > > > > modifications and have a new/rolled transaction that covers the free of
> > > > > the associated blocks (EFDs). I could certainly be missing something,
> > > > > but from that point what difference does it make whether the final
> > > > > transaction relogs the inode before it commits?
> > >
> > > It ensures the inode changes are sanely ordered. i.e. we don't
> > > write the inode to disk before all the other changes made in the
> > > rolling transaction are written to disk. And the same goes for
> > > recovery.
> > >
> >
> > But what exactly does that accomplish? The inode changes, block unmap
> > and and EFIs are all logged in the same transaction. Given the inode
> > remains locked, what difference does it make whether it is relogged in
> > the transaction that processes the EFIs? IOW, what can go wrong here
> > without the additional inode relog?
> >
> > Also, what do you mean by "sanely ordered?" AFAICT, there is no such
> > writeback ordering guarantee even if the log items are part of the same
> > transaction.
>
> The rolling tranactions can be committed can be in different
> checkpoints. If the journal is commited after the last roll but
> before the final commit, then the inode is unpinned before it is
> unlocked, while everything in the current transaction is still
> pinned in memory. When the last transaction commits, the inode gets
> unlocked and can be written back before the remaining operations in
> the rolling transaction are committed to stable storage.
>
> i.e. we end up with an inode on disk with a younger LSN in it than a
> bunch of it's dependent changes have. Strictly speaking, that's an
> on-disk ordering violation and potentially an on-disk transactional
> change atomicity violation. We don't get a violation in memory
> because of the inode locking being held across the transaction
> commit, but we do get one on disk because the inode is unpinned in
> memory before all it's dependent changes are in stable journal
> storage....
>
It's unpinned before it's unlocked in that case because there are no
dependent changes in the subsequent transaction. The subsequent
transaction doesn't modify the inode, it is just arbitrarily
dirtied/relogged in the final transaction without having been modified
since the previous roll.
Therefore, ISTM that it's perfectly fine if the inode were written back
immediately after being unlocked after the final tx commit. The first
transaction that did ultimately commit, checkpoint to the log and unpin
the inode included an EFI that ensures the filesystem remains consistent
in the event of a crash.
> Hence by logging the inode in the final transaction, we ensure it
> stays pinned in memory until the entire transaction is stable in the
> journal and we've guaranteed the on disk ordering and atomicity
> matches what is provided by the in-memory locking....
>
> > Suppose we return from xfs_inactive_symlink_rmt(),
> > everything is committed/unlocked and an AIL push occurs.
>
> All the objects are still pinned in memory, so they can't be written
> back by the AIL until the journal commits. :P
>
I don't think I stated the context clearly enough. The context for the
example I'm giving is that xfs_inactive_symlink_rmt() completes and the
log has checkpointed since the final transaction has committed. The
associated log items are unlocked, AIL resident and unpinned. The inode
was relogged in the final symlink_rmt() transaction, so we can assume
the lsn of the inode log item matches everything else committed in the
final tx. The only thing left to do now is writeback the associated
metadata objects from the AIL.
The example is otherwise the same... Given that context, ISTM that AIL
writeback contending with a newly created transaction can cause
writeback of the inode to complete while other objects in that final
symlink_rmt() transaction go from the starting point defined by the
above context (AIL+unpinned) to AIL+locked -> AIL+unlocked+pinned to
then being physically relogged in a new checkpoint (i.e., back to
AIL+unpinned) by a log force and thus ultimately written back with a
different LSN from that of the checkpoint that included the
xfs_inactive_symlink_rmt() changes.
The inode is relogged in this example, but the writeback looks roughly
equivalent to me as if the inode were not relogged in your example above
(and thus written back immediately after unlock). If I'm still missing
something here, a more specific example would be helpful..
> > Nothing
> > prevents a completely unrelated transaction from locking those same
> > allocation btree buffers that were in the same (rolled) transaction as
> > the EFD (plus relogged inode) before xfsaild gets to them, which means
> > the associated items could be written back in arbitrary order anyways.
>
> Sure, but the combination of locking and journal ordering via
> relogging in the CIL handles reuse via independent atomic
> transaction cases just fine. That's different to ordering within a
> a single atomic transaction chain....
>
See the context above. The example refers to writeback of an already
checkpointed transaction contending with one that is just starting. IOW,
the AIL push races with a new transaction locking previously unlocked
objects in the AIL. Nothing is in the CIL until the contending
transaction commits and so the CIL has nothing to reorder. The
associated items are physically relogged when the CIL checkpoints the
contending transaction.
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2018-05-12 14:17 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-08 3:41 [PATCH 0/9 v2] xfs: log item and transaction cleanups Dave Chinner
2018-05-08 3:41 ` [PATCH 1/9] xfs: log item flags are racy Dave Chinner
2018-05-09 14:51 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 2/9] xfs: add tracing to high level transaction operations Dave Chinner
2018-05-09 14:51 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 3/9] xfs: adder caller IP to xfs_defer* tracepoints Dave Chinner
2018-05-09 14:52 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 4/9] xfs: don't assert fail with AIL lock held Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 6:13 ` Christoph Hellwig
2018-05-09 14:52 ` Darrick J. Wong
2018-05-08 3:41 ` [PATCH 5/9] xfs: fix double ijoin in xfs_inactive_symlink_rmt() Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 0:24 ` Dave Chinner
2018-05-09 10:10 ` Brian Foster
2018-05-09 15:02 ` Darrick J. Wong
2018-05-11 2:04 ` Dave Chinner
2018-05-11 13:24 ` Brian Foster
2018-05-12 2:00 ` Dave Chinner
2018-05-12 14:17 ` Brian Foster
2018-05-08 3:41 ` [PATCH 6/9] xfs: fix double ijoin in xfs_reflink_cancel_cow_range Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 15:17 ` Darrick J. Wong
2018-05-08 3:42 ` [PATCH 7/9] xfs: fix double ijoin in xfs_reflink_clear_inode_flag() Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 0:40 ` Dave Chinner
2018-05-09 10:12 ` Brian Foster
2018-05-09 15:19 ` Darrick J. Wong
2018-05-08 3:42 ` [PATCH 8/9] xfs: add some more debug checks to buffer log item reuse Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 15:19 ` Darrick J. Wong
2018-05-08 3:42 ` [PATCH 9/9] xfs: get rid of the log item descriptor Dave Chinner
2018-05-08 14:18 ` Brian Foster
2018-05-09 6:27 ` Christoph Hellwig
2018-05-09 15:19 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.