xfs: avoid empty xattr transaction when attrs are inline

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@redhat.com>
Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Dave Chinner <david@fromorbit.com>

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 3a5f22eae6077b6ca72e4239206a1000c5e3b30d..98a2a2f89664dcaaee2b38ac50475e737f1ea18b 100644 (file)
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -314,6 +314,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)) {
@@ -334,37 +335,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;
 }
 
 /*