On Mon, May 09, 2022 at 10:41:21AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> generic/642 triggered a reproducable assert failure in
> xlog_cil_commit() that resulted from a xfs_attr_set() committing
> an empty but dirty transaction. When the CIL is empty and this
> occurs, xlog_cil_commit() tries a background push and this triggers
> a "pushing an empty CIL" assert.
>
> XFS: Assertion failed: !list_empty(&cil->xc_cil), file: fs/xfs/xfs_log_cil.c, line: 1274
> Call Trace:
> <TASK>
> xlog_cil_commit+0xa5a/0xad0
> __xfs_trans_commit+0xb8/0x330
> xfs_trans_commit+0x10/0x20
> xfs_attr_set+0x3e2/0x4c0
> xfs_xattr_set+0x8d/0xe0
> __vfs_setxattr+0x6b/0x90
> __vfs_setxattr_noperm+0x76/0x220
> __vfs_setxattr_locked+0xdf/0x100
> vfs_setxattr+0x94/0x170
> setxattr+0x110/0x200
> path_setxattr+0xbf/0xe0
> __x64_sys_setxattr+0x2b/0x30
> do_syscall_64+0x35/0x80
>
> The problem is related to the breakdown of attribute addition in
> xfs_attr_set_iter() and how it is called from deferred operations.
> When we have a pure leaf xattr insert, we add the xattr to the leaf
> and set the next state to XFS_DAS_FOUND_LBLK and return -EAGAIN.
> This requeues the xattr defered work, rolls the transaction and
> runs xfs_attr_set_iter() again. This then checks the xattr for
> being remote (it's not) and whether a replace op is being done (this
> is a create op) and if neither are true it returns without having
> done anything.
>
> xfs_xattri_finish_update() then unconditionally sets the transaction
> dirty, and the deferops finishes and returns to __xfs_trans_commit()
> which sees the transaction dirty and tries to commit it by calling
> xlog_cil_commit(). The transaction is empty, and then the assert
> fires if this happens when the CIL is empty.
>
> This patch addresses the structure of xfs_attr_set_iter() that
> requires re-entry on leaf add even when nothing will be done. This
> gets rid of the trailing empty transaction and so doesn't trigger
> the XFS_TRANS_DIRTY assignment in xfs_xattri_finish_update()
> incorrectly. Addressing that is for a different patch.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Reviewed-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
Looks ok to me,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 48b7e7efbb30..b3d918195160 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -315,6 +315,7 @@ xfs_attr_leaf_addname(
> {
> struct xfs_da_args *args = attr->xattri_da_args;
> struct xfs_inode *dp = args->dp;
> + enum xfs_delattr_state next_state = XFS_DAS_UNINIT;
> int error;
>
> if (xfs_attr_is_leaf(dp)) {
> @@ -335,37 +336,35 @@ xfs_attr_leaf_addname(
> * when we come back, we'll be a node, so we'll fall
> * down into the node handling code below
> */
> - trace_xfs_attr_set_iter_return(
> - attr->xattri_dela_state, args->dp);
> - return -EAGAIN;
> + error = -EAGAIN;
> + goto out;
> }
> -
> - if (error)
> - return error;
> -
> - attr->xattri_dela_state = XFS_DAS_FOUND_LBLK;
> + next_state = XFS_DAS_FOUND_LBLK;
> } else {
> error = xfs_attr_node_addname_find_attr(attr);
> if (error)
> return error;
>
> + next_state = XFS_DAS_FOUND_NBLK;
> error = xfs_attr_node_addname(attr);
> - if (error)
> - return error;
> -
> - /*
> - * If addname was successful, and we dont need to alloc or
> - * remove anymore blks, we're done.
> - */
> - if (!args->rmtblkno &&
> - !(args->op_flags & XFS_DA_OP_RENAME))
> - return 0;
> + }
> + if (error)
> + return error;
>
> - attr->xattri_dela_state = XFS_DAS_FOUND_NBLK;
> + /*
> + * We need to commit and roll if we need to allocate remote xattr blocks
> + * or perform more xattr manipulations. Otherwise there is nothing more
> + * to do and we can return success.
> + */
> + if (args->rmtblkno ||
> + (args->op_flags & XFS_DA_OP_RENAME)) {
> + attr->xattri_dela_state = next_state;
> + error = -EAGAIN;
> }
>
> +out:
> trace_xfs_attr_leaf_addname_return(attr->xattri_dela_state, args->dp);
> - return -EAGAIN;
> + return error;
> }
>
> /*
> --
> 2.35.1
>
