On Wed, Mar 07, 2018 at 08:10:19PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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.
>
> Also add a log item flag to say the item has been joined to a
> transaction and assert that it's not set when adding a log item to a
> transaction. Clear it when the log item is disconnected from the log
> item descriptor. This will tell us if there are other log items that
> transactions are referencing multiple times.
>
> Signed-Off-By: Dave Chinner <dchinner@xxxxxxxxxx>
Looks ok, but please split this patch into one piece that adds more
explicit tracking (and ASSERTing) of the log items' attachment to
transactions and a second patch for the xfs_inactive_symlink_rmt
correction. Christoph stumbled over the new assert being here, and I
nearly did too.
--D
> ---
> fs/xfs/xfs_symlink.c | 9 ++-------
> fs/xfs/xfs_trans.c | 7 +++++--
> fs/xfs/xfs_trans.h | 4 +++-
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 2e9e793a8f9d..b0502027070e 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));
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 86f92df32c42..2aeeb2ad276a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -737,12 +737,14 @@ xfs_trans_add_item(
>
> ASSERT(lip->li_mountp == tp->t_mountp);
> ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> + ASSERT(!(lip->li_flags & XFS_LI_TRANS));
>
> 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_flags |= XFS_LI_TRANS;
>
> lip->li_desc = lidp;
> }
> @@ -762,6 +764,7 @@ void
> xfs_trans_del_item(
> struct xfs_log_item *lip)
> {
> + lip->li_flags &= ~XFS_LI_TRANS;
> xfs_trans_free_item_desc(lip->li_desc);
> lip->li_desc = NULL;
> }
> @@ -781,15 +784,15 @@ xfs_trans_free_items(
> list_for_each_entry_safe(lidp, next, &tp->t_items, lid_trans) {
> struct xfs_log_item *lip = lidp->lid_item;
>
> - lip->li_desc = NULL;
> + xfs_trans_del_item(lip);
>
> if (commit_lsn != NULLCOMMITLSN)
> lip->li_ops->iop_committing(lip, commit_lsn);
> if (abort)
> lip->li_flags |= XFS_LI_ABORTED;
> +
> lip->li_ops->iop_unlock(lip);
>
> - xfs_trans_free_item_desc(lidp);
> }
> }
>
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 9d542dfe0052..c514486ba2a0 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -67,11 +67,13 @@ typedef struct xfs_log_item {
> #define XFS_LI_IN_AIL 0x1
> #define XFS_LI_ABORTED 0x2
> #define XFS_LI_FAILED 0x4
> +#define XFS_LI_TRANS 0x8 /* attached to an active transaction */
>
> #define XFS_LI_FLAGS \
> { XFS_LI_IN_AIL, "IN_AIL" }, \
> { XFS_LI_ABORTED, "ABORTED" }, \
> - { XFS_LI_FAILED, "FAILED" }
> + { XFS_LI_FAILED, "FAILED" }, \
> + { XFS_LI_TRANS, "TRANS" }
>
> struct xfs_item_ops {
> void (*iop_size)(xfs_log_item_t *, int *, int *);
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
