On Wed, May 02, 2018 at 06:01:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> I've comes across a series of subtle bugs in development code that
> are a result of inodes being joined to the same transaction more
> than once. This means the transaction has multiple log item
> descriptors pointing to the same log item, and so processes them
> multiple times during transaction commit processing. Further, the
> log item can only store a single log item descriptor back pointer,
> so this means all but one of the log item descriptors pointing
> to the log item can't be found from the log item.
>
> 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>
> ---
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> fs/xfs/xfs_trans.c | 6 ++++--
> fs/xfs/xfs_trans.h | 4 +++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index c6a2a3cb9df5..33c40788cffa 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -741,6 +741,7 @@ xfs_trans_add_item(
>
> ASSERT(lip->li_mountp == tp->t_mountp);
> ASSERT(lip->li_ailp == tp->t_mountp->m_ail);
> + ASSERT(!test_bit(XFS_LI_TRANS, &lip->li_flags));
>
> lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);
>
> @@ -749,6 +750,7 @@ xfs_trans_add_item(
> list_add_tail(&lidp->lid_trans, &tp->t_items);
>
> lip->li_desc = lidp;
> + set_bit(XFS_LI_TRANS, &lip->li_flags);
> }
>
> STATIC void
> @@ -766,6 +768,7 @@ void
> xfs_trans_del_item(
> struct xfs_log_item *lip)
> {
> + clear_bit(XFS_LI_TRANS, &lip->li_flags);
> xfs_trans_free_item_desc(lip->li_desc);
> lip->li_desc = NULL;
> }
> @@ -785,7 +788,7 @@ 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);
> @@ -793,7 +796,6 @@ xfs_trans_free_items(
> set_bit(XFS_LI_ABORTED, &lip->li_flags);
> 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 51145ddf7e5b..af52107adffc 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -72,11 +72,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_TRANS 3 /* attached to an active 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_TRANS), "TRANS" }
>
> struct xfs_item_ops {
> void (*iop_size)(xfs_log_item_t *, int *, int *);
> --
> 2.17.0
>
> --
> 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
