[PATCH] xfs: fix hung when transaction commit fail in xfs_inactive_ifree

[PATCH] xfs: fix hung when transaction commit fail in xfs_inactive_ifree

[Long Li]

After running unplug disk test and unmount filesystem, the umount thread
hung all the time.

 crash> dmesg
 sd 0:0:0:0: rejecting I/O to offline device
 XFS (sda): log I/O error -5
 XFS (sda): Corruption of in-memory data (0x8) detected at xfs_defer_finish_noroll+0x12e0/0x1cf0
	(fs/xfs/libxfs/xfs_defer.c:504).  Shutting down filesystem.
 XFS (sda): Please unmount the filesystem and rectify the problem(s)
 XFS (sda): xfs_inactive_ifree: xfs_trans_commit returned error -5
 XFS (sda): Unmounting Filesystem

 crash> bt 3368
 PID: 3368   TASK: ffff88801bcd8040  CPU: 3   COMMAND: "umount"
  #0 [ffffc900086a7ae0] __schedule at ffffffff83d3fd25
  #1 [ffffc900086a7be8] schedule at ffffffff83d414dd
  #2 [ffffc900086a7c10] xfs_ail_push_all_sync at ffffffff8256db24
  #3 [ffffc900086a7d18] xfs_unmount_flush_inodes at ffffffff824ee7e2
  #4 [ffffc900086a7d28] xfs_unmountfs at ffffffff824f2eff
  #5 [ffffc900086a7da8] xfs_fs_put_super at ffffffff82503e69
  #6 [ffffc900086a7de8] generic_shutdown_super at ffffffff81aeb8cd
  #7 [ffffc900086a7e10] kill_block_super at ffffffff81aefcfa
  #8 [ffffc900086a7e30] deactivate_locked_super at ffffffff81aeb2da
  #9 [ffffc900086a7e48] deactivate_super at ffffffff81aeb639
 #10 [ffffc900086a7e68] cleanup_mnt at ffffffff81b6ddd5
 #11 [ffffc900086a7ea0] __cleanup_mnt at ffffffff81b6dfdf
 #12 [ffffc900086a7eb0] task_work_run at ffffffff8126e5cf
 #13 [ffffc900086a7ef8] exit_to_user_mode_prepare at ffffffff813fa136
 #14 [ffffc900086a7f28] syscall_exit_to_user_mode at ffffffff83d25dbb
 #15 [ffffc900086a7f40] do_syscall_64 at ffffffff83d1f8d9
 #16 [ffffc900086a7f50] entry_SYSCALL_64_after_hwframe at ffffffff83e00085

When we free a cluster buffer from xfs_ifree_cluster, all the inodes in
cache are marked XFS_ISTALE. On journal commit dirty stale inodes as are
handled by both buffer and inode log items, inodes marked as XFS_ISTALE
in AIL will be removed from the AIL because the buffer log item will clean
it. If the transaction commit fails in the xfs_inactive_ifree(), inodes
marked as XFS_ISTALE will be left in AIL due to buf log item is not
committed, this will cause the unmount thread above to be blocked all the
time. Error handling in xfs_inactive_ifree() is not enough, the above
exception needs to be considered.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_inode.c | 114 +++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_inode.h |   1 -
 2 files changed, 105 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index d354ea2b74f9..b6808c0a2868 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -49,6 +49,9 @@ struct kmem_cache *xfs_inode_cache;
 STATIC int xfs_iunlink(struct xfs_trans *, struct xfs_inode *);
 STATIC int xfs_iunlink_remove(struct xfs_trans *tp, struct xfs_perag *pag,
 	struct xfs_inode *);
+STATIC int xfs_ifree(struct xfs_trans *tp, struct xfs_inode *ip,
+		struct xfs_icluster *xic);
+STATIC void xfs_ifree_abort(struct xfs_inode *ip, struct xfs_icluster *xic);
 
 /*
  * helper function to extract extent size hint from inode
@@ -1544,6 +1547,7 @@ xfs_inactive_ifree(
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
+	struct xfs_icluster     xic = { 0 };
 	int			error;
 
 	/*
@@ -1598,7 +1602,7 @@ xfs_inactive_ifree(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 
-	error = xfs_ifree(tp, ip);
+	error = xfs_ifree(tp, ip, &xic);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	if (error) {
 		/*
@@ -1612,7 +1616,7 @@ xfs_inactive_ifree(
 			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
 		}
 		xfs_trans_cancel(tp);
-		return error;
+		goto out_error;
 	}
 
 	/*
@@ -1625,11 +1629,19 @@ xfs_inactive_ifree(
 	 * to try to keep going. Make sure it's not a silent error.
 	 */
 	error = xfs_trans_commit(tp);
-	if (error)
+	if (error) {
 		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
 			__func__, error);
+		goto out_error;
+	}
 
 	return 0;
+
+out_error:
+	if (xic.deleted)
+		xfs_ifree_abort(ip, &xic);
+
+	return error;
 }
 
 /*
@@ -2259,14 +2271,14 @@ xfs_ifree_cluster(
  * inodes in the AGI. We need to remove the inode from that list atomically with
  * respect to freeing it here.
  */
-int
+STATIC int
 xfs_ifree(
 	struct xfs_trans	*tp,
-	struct xfs_inode	*ip)
+	struct xfs_inode	*ip,
+	struct xfs_icluster     *xic)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_perag	*pag;
-	struct xfs_icluster	xic = { 0 };
 	struct xfs_inode_log_item *iip = ip->i_itemp;
 	int			error;
 
@@ -2284,7 +2296,7 @@ xfs_ifree(
 	 * makes the AGI lock -> unlinked list modification order the same as
 	 * used in O_TMPFILE creation.
 	 */
-	error = xfs_difree(tp, pag, ip->i_ino, &xic);
+	error = xfs_difree(tp, pag, ip->i_ino, xic);
 	if (error)
 		goto out;
 
@@ -2323,13 +2335,97 @@ xfs_ifree(
 	VFS_I(ip)->i_generation++;
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-	if (xic.deleted)
-		error = xfs_ifree_cluster(tp, pag, ip, &xic);
+	if (xic->deleted)
+		error = xfs_ifree_cluster(tp, pag, ip, xic);
 out:
 	xfs_perag_put(pag);
 	return error;
 }
 
+static void
+xfs_ifree_abort_inode_stale(
+	struct xfs_perag	*pag,
+	xfs_ino_t		inum)
+{
+	struct xfs_mount        *mp = pag->pag_mount;
+	struct xfs_inode_log_item *iip;
+	struct xfs_inode	*ip;
+
+retry:
+	rcu_read_lock();
+	ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
+
+	/* Inode not in memory, nothing to do */
+	if (!ip) {
+		rcu_read_unlock();
+		return;
+	}
+
+	/* Skip invalid or not stale inode */
+	if (ip->i_ino != inum || !xfs_iflags_test(ip, XFS_ISTALE)) {
+		rcu_read_unlock();
+		return;
+	}
+
+	if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
+		rcu_read_unlock();
+		delay(1);
+		goto retry;
+	}
+
+	iip = ip->i_itemp;
+	if (!iip || list_empty(&iip->ili_item.li_bio_list))
+		goto out_iunlock;
+
+	if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags))
+		xfs_iflush_abort(ip);
+	else
+		xfs_iflags_clear(ip, XFS_IFLUSHING);
+
+out_iunlock:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	rcu_read_unlock();
+}
+
+/*
+ * This is called to clean up inodes marked as stale in xfs_ifree
+ */
+STATIC void
+xfs_ifree_abort(
+	struct xfs_inode	*ip,
+	struct xfs_icluster	*xic)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_perag        *pag;
+	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
+	xfs_ino_t		inum = xic->first_ino;
+	int			nbufs;
+	int			i, j;
+	int			ioffset;
+
+	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
+
+	nbufs = igeo->ialloc_blks / igeo->blocks_per_cluster;
+
+	for (j = 0; j < nbufs; j++, inum += igeo->inodes_per_cluster) {
+		/*
+		 * The allocation bitmap tells us which inodes of the chunk were
+		 * physically allocated. Skip the cluster if an inode falls into
+		 * a sparse region.
+		 */
+		ioffset = inum - xic->first_ino;
+		if ((xic->alloc & XFS_INOBT_MASK(ioffset)) == 0) {
+			ASSERT(ioffset % igeo->inodes_per_cluster == 0);
+			continue;
+		}
+
+		for (i = 0; i < igeo->inodes_per_cluster; i++)
+			xfs_ifree_abort_inode_stale(pag, inum + i);
+
+	}
+	xfs_perag_put(pag);
+}
+
 /*
  * This is called to unpin an inode.  The caller must have the inode locked
  * in at least shared mode so that the buffer cannot be subsequently pinned
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index fa780f08dc89..423542bf6af1 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -499,7 +499,6 @@ uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 
 uint		xfs_ip2xflags(struct xfs_inode *);
-int		xfs_ifree(struct xfs_trans *, struct xfs_inode *);
 int		xfs_itruncate_extents_flags(struct xfs_trans **,
 				struct xfs_inode *, int, xfs_fsize_t, int);
 void		xfs_iext_realloc(xfs_inode_t *, int, int);
-- 
2.31.1

Re: [PATCH] xfs: fix hung when transaction commit fail in xfs_inactive_ifree

[Darrick J. Wong]

On Fri, Dec 09, 2022 at 07:05:19PM +0800, Long Li wrote:
> After running unplug disk test and unmount filesystem, the umount thread
> hung all the time.
> 
>  crash> dmesg
>  sd 0:0:0:0: rejecting I/O to offline device
>  XFS (sda): log I/O error -5
>  XFS (sda): Corruption of in-memory data (0x8) detected at xfs_defer_finish_noroll+0x12e0/0x1cf0
> 	(fs/xfs/libxfs/xfs_defer.c:504).  Shutting down filesystem.
>  XFS (sda): Please unmount the filesystem and rectify the problem(s)
>  XFS (sda): xfs_inactive_ifree: xfs_trans_commit returned error -5
>  XFS (sda): Unmounting Filesystem
> 
>  crash> bt 3368
>  PID: 3368   TASK: ffff88801bcd8040  CPU: 3   COMMAND: "umount"
>   #0 [ffffc900086a7ae0] __schedule at ffffffff83d3fd25
>   #1 [ffffc900086a7be8] schedule at ffffffff83d414dd
>   #2 [ffffc900086a7c10] xfs_ail_push_all_sync at ffffffff8256db24
>   #3 [ffffc900086a7d18] xfs_unmount_flush_inodes at ffffffff824ee7e2
>   #4 [ffffc900086a7d28] xfs_unmountfs at ffffffff824f2eff
>   #5 [ffffc900086a7da8] xfs_fs_put_super at ffffffff82503e69
>   #6 [ffffc900086a7de8] generic_shutdown_super at ffffffff81aeb8cd
>   #7 [ffffc900086a7e10] kill_block_super at ffffffff81aefcfa
>   #8 [ffffc900086a7e30] deactivate_locked_super at ffffffff81aeb2da
>   #9 [ffffc900086a7e48] deactivate_super at ffffffff81aeb639
>  #10 [ffffc900086a7e68] cleanup_mnt at ffffffff81b6ddd5
>  #11 [ffffc900086a7ea0] __cleanup_mnt at ffffffff81b6dfdf
>  #12 [ffffc900086a7eb0] task_work_run at ffffffff8126e5cf
>  #13 [ffffc900086a7ef8] exit_to_user_mode_prepare at ffffffff813fa136
>  #14 [ffffc900086a7f28] syscall_exit_to_user_mode at ffffffff83d25dbb
>  #15 [ffffc900086a7f40] do_syscall_64 at ffffffff83d1f8d9
>  #16 [ffffc900086a7f50] entry_SYSCALL_64_after_hwframe at ffffffff83e00085
> 
> When we free a cluster buffer from xfs_ifree_cluster, all the inodes in
> cache are marked XFS_ISTALE. On journal commit dirty stale inodes as are
> handled by both buffer and inode log items, inodes marked as XFS_ISTALE
> in AIL will be removed from the AIL because the buffer log item will clean
> it. If the transaction commit fails in the xfs_inactive_ifree(), inodes
> marked as XFS_ISTALE will be left in AIL due to buf log item is not
> committed, this will cause the unmount thread above to be blocked all the
> time. Error handling in xfs_inactive_ifree() is not enough, the above
> exception needs to be considered.
> 
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_inode.c | 114 +++++++++++++++++++++++++++++++++++++++++----
>  fs/xfs/xfs_inode.h |   1 -
>  2 files changed, 105 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d354ea2b74f9..b6808c0a2868 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -49,6 +49,9 @@ struct kmem_cache *xfs_inode_cache;
>  STATIC int xfs_iunlink(struct xfs_trans *, struct xfs_inode *);
>  STATIC int xfs_iunlink_remove(struct xfs_trans *tp, struct xfs_perag *pag,
>  	struct xfs_inode *);
> +STATIC int xfs_ifree(struct xfs_trans *tp, struct xfs_inode *ip,
> +		struct xfs_icluster *xic);
> +STATIC void xfs_ifree_abort(struct xfs_inode *ip, struct xfs_icluster *xic);
>  
>  /*
>   * helper function to extract extent size hint from inode
> @@ -1544,6 +1547,7 @@ xfs_inactive_ifree(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_trans	*tp;
> +	struct xfs_icluster     xic = { 0 };
>  	int			error;
>  
>  	/*
> @@ -1598,7 +1602,7 @@ xfs_inactive_ifree(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  
> -	error = xfs_ifree(tp, ip);
> +	error = xfs_ifree(tp, ip, &xic);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	if (error) {
>  		/*
> @@ -1612,7 +1616,7 @@ xfs_inactive_ifree(
>  			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
>  		}
>  		xfs_trans_cancel(tp);
> -		return error;
> +		goto out_error;
>  	}
>  
>  	/*
> @@ -1625,11 +1629,19 @@ xfs_inactive_ifree(
>  	 * to try to keep going. Make sure it's not a silent error.
>  	 */
>  	error = xfs_trans_commit(tp);
> -	if (error)
> +	if (error) {
>  		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
>  			__func__, error);
> +		goto out_error;
> +	}
>  
>  	return 0;
> +
> +out_error:
> +	if (xic.deleted)
> +		xfs_ifree_abort(ip, &xic);
> +
> +	return error;
>  }
>  
>  /*
> @@ -2259,14 +2271,14 @@ xfs_ifree_cluster(
>   * inodes in the AGI. We need to remove the inode from that list atomically with
>   * respect to freeing it here.
>   */
> -int
> +STATIC int
>  xfs_ifree(
>  	struct xfs_trans	*tp,
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	struct xfs_icluster     *xic)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_perag	*pag;
> -	struct xfs_icluster	xic = { 0 };
>  	struct xfs_inode_log_item *iip = ip->i_itemp;
>  	int			error;
>  
> @@ -2284,7 +2296,7 @@ xfs_ifree(
>  	 * makes the AGI lock -> unlinked list modification order the same as
>  	 * used in O_TMPFILE creation.
>  	 */
> -	error = xfs_difree(tp, pag, ip->i_ino, &xic);
> +	error = xfs_difree(tp, pag, ip->i_ino, xic);
>  	if (error)
>  		goto out;
>  
> @@ -2323,13 +2335,97 @@ xfs_ifree(
>  	VFS_I(ip)->i_generation++;
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -	if (xic.deleted)
> -		error = xfs_ifree_cluster(tp, pag, ip, &xic);
> +	if (xic->deleted)
> +		error = xfs_ifree_cluster(tp, pag, ip, xic);
>  out:
>  	xfs_perag_put(pag);
>  	return error;
>  }
>  
> +static void
> +xfs_ifree_abort_inode_stale(
> +	struct xfs_perag	*pag,
> +	xfs_ino_t		inum)
> +{
> +	struct xfs_mount        *mp = pag->pag_mount;
> +	struct xfs_inode_log_item *iip;
> +	struct xfs_inode	*ip;
> +
> +retry:
> +	rcu_read_lock();
> +	ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
> +
> +	/* Inode not in memory, nothing to do */
> +	if (!ip) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	/* Skip invalid or not stale inode */
> +	if (ip->i_ino != inum || !xfs_iflags_test(ip, XFS_ISTALE)) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> +		rcu_read_unlock();
> +		delay(1);
> +		goto retry;
> +	}
> +
> +	iip = ip->i_itemp;
> +	if (!iip || list_empty(&iip->ili_item.li_bio_list))
> +		goto out_iunlock;
> +
> +	if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags))
> +		xfs_iflush_abort(ip);
> +	else
> +		xfs_iflags_clear(ip, XFS_IFLUSHING);

Er... why is the ifree code tearing into the inode log item state ?

Shouldn't this be getting done from the buffer log item when we release
it and find that it's aborted?

--D

> +
> +out_iunlock:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	rcu_read_unlock();
> +}
> +
> +/*
> + * This is called to clean up inodes marked as stale in xfs_ifree
> + */
> +STATIC void
> +xfs_ifree_abort(
> +	struct xfs_inode	*ip,
> +	struct xfs_icluster	*xic)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_perag        *pag;
> +	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> +	xfs_ino_t		inum = xic->first_ino;
> +	int			nbufs;
> +	int			i, j;
> +	int			ioffset;
> +
> +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
> +
> +	nbufs = igeo->ialloc_blks / igeo->blocks_per_cluster;
> +
> +	for (j = 0; j < nbufs; j++, inum += igeo->inodes_per_cluster) {
> +		/*
> +		 * The allocation bitmap tells us which inodes of the chunk were
> +		 * physically allocated. Skip the cluster if an inode falls into
> +		 * a sparse region.
> +		 */
> +		ioffset = inum - xic->first_ino;
> +		if ((xic->alloc & XFS_INOBT_MASK(ioffset)) == 0) {
> +			ASSERT(ioffset % igeo->inodes_per_cluster == 0);
> +			continue;
> +		}
> +
> +		for (i = 0; i < igeo->inodes_per_cluster; i++)
> +			xfs_ifree_abort_inode_stale(pag, inum + i);
> +
> +	}
> +	xfs_perag_put(pag);
> +}
> +
>  /*
>   * This is called to unpin an inode.  The caller must have the inode locked
>   * in at least shared mode so that the buffer cannot be subsequently pinned
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index fa780f08dc89..423542bf6af1 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -499,7 +499,6 @@ uint		xfs_ilock_data_map_shared(struct xfs_inode *);
>  uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
>  
>  uint		xfs_ip2xflags(struct xfs_inode *);
> -int		xfs_ifree(struct xfs_trans *, struct xfs_inode *);
>  int		xfs_itruncate_extents_flags(struct xfs_trans **,
>  				struct xfs_inode *, int, xfs_fsize_t, int);
>  void		xfs_iext_realloc(xfs_inode_t *, int, int);
> -- 
> 2.31.1
> 

Re: [PATCH] xfs: fix hung when transaction commit fail in xfs_inactive_ifree

[Long Li]

On Tue, Jan 31, 2023 at 07:21:39PM -0800, Darrick J. Wong wrote:
> On Fri, Dec 09, 2022 at 07:05:19PM +0800, Long Li wrote:
> > After running unplug disk test and unmount filesystem, the umount thread
> > hung all the time.
> > 
> >  crash> dmesg
> >  sd 0:0:0:0: rejecting I/O to offline device
> >  XFS (sda): log I/O error -5
> >  XFS (sda): Corruption of in-memory data (0x8) detected at xfs_defer_finish_noroll+0x12e0/0x1cf0
> > 	(fs/xfs/libxfs/xfs_defer.c:504).  Shutting down filesystem.
> >  XFS (sda): Please unmount the filesystem and rectify the problem(s)
> >  XFS (sda): xfs_inactive_ifree: xfs_trans_commit returned error -5
> >  XFS (sda): Unmounting Filesystem
> > 
> >  crash> bt 3368
> >  PID: 3368   TASK: ffff88801bcd8040  CPU: 3   COMMAND: "umount"
> >   #0 [ffffc900086a7ae0] __schedule at ffffffff83d3fd25
> >   #1 [ffffc900086a7be8] schedule at ffffffff83d414dd
> >   #2 [ffffc900086a7c10] xfs_ail_push_all_sync at ffffffff8256db24
> >   #3 [ffffc900086a7d18] xfs_unmount_flush_inodes at ffffffff824ee7e2
> >   #4 [ffffc900086a7d28] xfs_unmountfs at ffffffff824f2eff
> >   #5 [ffffc900086a7da8] xfs_fs_put_super at ffffffff82503e69
> >   #6 [ffffc900086a7de8] generic_shutdown_super at ffffffff81aeb8cd
> >   #7 [ffffc900086a7e10] kill_block_super at ffffffff81aefcfa
> >   #8 [ffffc900086a7e30] deactivate_locked_super at ffffffff81aeb2da
> >   #9 [ffffc900086a7e48] deactivate_super at ffffffff81aeb639
> >  #10 [ffffc900086a7e68] cleanup_mnt at ffffffff81b6ddd5
> >  #11 [ffffc900086a7ea0] __cleanup_mnt at ffffffff81b6dfdf
> >  #12 [ffffc900086a7eb0] task_work_run at ffffffff8126e5cf
> >  #13 [ffffc900086a7ef8] exit_to_user_mode_prepare at ffffffff813fa136
> >  #14 [ffffc900086a7f28] syscall_exit_to_user_mode at ffffffff83d25dbb
> >  #15 [ffffc900086a7f40] do_syscall_64 at ffffffff83d1f8d9
> >  #16 [ffffc900086a7f50] entry_SYSCALL_64_after_hwframe at ffffffff83e00085
> > 
> > When we free a cluster buffer from xfs_ifree_cluster, all the inodes in
> > cache are marked XFS_ISTALE. On journal commit dirty stale inodes as are
> > handled by both buffer and inode log items, inodes marked as XFS_ISTALE
> > in AIL will be removed from the AIL because the buffer log item will clean
> > it. If the transaction commit fails in the xfs_inactive_ifree(), inodes
> > marked as XFS_ISTALE will be left in AIL due to buf log item is not
> > committed, this will cause the unmount thread above to be blocked all the
> > time. Error handling in xfs_inactive_ifree() is not enough, the above
> > exception needs to be considered.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/xfs_inode.c | 114 +++++++++++++++++++++++++++++++++++++++++----
> >  fs/xfs/xfs_inode.h |   1 -
> >  2 files changed, 105 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index d354ea2b74f9..b6808c0a2868 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -49,6 +49,9 @@ struct kmem_cache *xfs_inode_cache;
> >  STATIC int xfs_iunlink(struct xfs_trans *, struct xfs_inode *);
> >  STATIC int xfs_iunlink_remove(struct xfs_trans *tp, struct xfs_perag *pag,
> >  	struct xfs_inode *);
> > +STATIC int xfs_ifree(struct xfs_trans *tp, struct xfs_inode *ip,
> > +		struct xfs_icluster *xic);
> > +STATIC void xfs_ifree_abort(struct xfs_inode *ip, struct xfs_icluster *xic);
> >  
> >  /*
> >   * helper function to extract extent size hint from inode
> > @@ -1544,6 +1547,7 @@ xfs_inactive_ifree(
> >  {
> >  	struct xfs_mount	*mp = ip->i_mount;
> >  	struct xfs_trans	*tp;
> > +	struct xfs_icluster     xic = { 0 };
> >  	int			error;
> >  
> >  	/*
> > @@ -1598,7 +1602,7 @@ xfs_inactive_ifree(
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> >  
> > -	error = xfs_ifree(tp, ip);
> > +	error = xfs_ifree(tp, ip, &xic);
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >  	if (error) {
> >  		/*
> > @@ -1612,7 +1616,7 @@ xfs_inactive_ifree(
> >  			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> >  		}
> >  		xfs_trans_cancel(tp);
> > -		return error;
> > +		goto out_error;
> >  	}
> >  
> >  	/*
> > @@ -1625,11 +1629,19 @@ xfs_inactive_ifree(
> >  	 * to try to keep going. Make sure it's not a silent error.
> >  	 */
> >  	error = xfs_trans_commit(tp);
> > -	if (error)
> > +	if (error) {
> >  		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
> >  			__func__, error);
> > +		goto out_error;
> > +	}
> >  
> >  	return 0;
> > +
> > +out_error:
> > +	if (xic.deleted)
> > +		xfs_ifree_abort(ip, &xic);
> > +
> > +	return error;
> >  }
> >  
> >  /*
> > @@ -2259,14 +2271,14 @@ xfs_ifree_cluster(
> >   * inodes in the AGI. We need to remove the inode from that list atomically with
> >   * respect to freeing it here.
> >   */
> > -int
> > +STATIC int
> >  xfs_ifree(
> >  	struct xfs_trans	*tp,
> > -	struct xfs_inode	*ip)
> > +	struct xfs_inode	*ip,
> > +	struct xfs_icluster     *xic)
> >  {
> >  	struct xfs_mount	*mp = ip->i_mount;
> >  	struct xfs_perag	*pag;
> > -	struct xfs_icluster	xic = { 0 };
> >  	struct xfs_inode_log_item *iip = ip->i_itemp;
> >  	int			error;
> >  
> > @@ -2284,7 +2296,7 @@ xfs_ifree(
> >  	 * makes the AGI lock -> unlinked list modification order the same as
> >  	 * used in O_TMPFILE creation.
> >  	 */
> > -	error = xfs_difree(tp, pag, ip->i_ino, &xic);
> > +	error = xfs_difree(tp, pag, ip->i_ino, xic);
> >  	if (error)
> >  		goto out;
> >  
> > @@ -2323,13 +2335,97 @@ xfs_ifree(
> >  	VFS_I(ip)->i_generation++;
> >  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >  
> > -	if (xic.deleted)
> > -		error = xfs_ifree_cluster(tp, pag, ip, &xic);
> > +	if (xic->deleted)
> > +		error = xfs_ifree_cluster(tp, pag, ip, xic);
> >  out:
> >  	xfs_perag_put(pag);
> >  	return error;
> >  }
> >  
> > +static void
> > +xfs_ifree_abort_inode_stale(
> > +	struct xfs_perag	*pag,
> > +	xfs_ino_t		inum)
> > +{
> > +	struct xfs_mount        *mp = pag->pag_mount;
> > +	struct xfs_inode_log_item *iip;
> > +	struct xfs_inode	*ip;
> > +
> > +retry:
> > +	rcu_read_lock();
> > +	ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
> > +
> > +	/* Inode not in memory, nothing to do */
> > +	if (!ip) {
> > +		rcu_read_unlock();
> > +		return;
> > +	}
> > +
> > +	/* Skip invalid or not stale inode */
> > +	if (ip->i_ino != inum || !xfs_iflags_test(ip, XFS_ISTALE)) {
> > +		rcu_read_unlock();
> > +		return;
> > +	}
> > +
> > +	if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> > +		rcu_read_unlock();
> > +		delay(1);
> > +		goto retry;
> > +	}
> > +
> > +	iip = ip->i_itemp;
> > +	if (!iip || list_empty(&iip->ili_item.li_bio_list))
> > +		goto out_iunlock;
> > +
> > +	if (test_bit(XFS_LI_IN_AIL, &iip->ili_item.li_flags))
> > +		xfs_iflush_abort(ip);
> > +	else
> > +		xfs_iflags_clear(ip, XFS_IFLUSHING);
> 
> Er... why is the ifree code tearing into the inode log item state ?
> 
> Shouldn't this be getting done from the buffer log item when we release
> it and find that it's aborted?
> 
> --D

Yes, it doesn't looks good here, traverse buffer's b_li_list and abort xfs_inode
marked as XFS_ISTALE could be better.
> 
> > +
> > +out_iunlock:
> > +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +	rcu_read_unlock();
> > +}
> > +
> > +/*
> > + * This is called to clean up inodes marked as stale in xfs_ifree
> > + */
> > +STATIC void
> > +xfs_ifree_abort(
> > +	struct xfs_inode	*ip,
> > +	struct xfs_icluster	*xic)
> > +{
> > +	struct xfs_mount	*mp = ip->i_mount;
> > +	struct xfs_perag        *pag;
> > +	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> > +	xfs_ino_t		inum = xic->first_ino;
> > +	int			nbufs;
> > +	int			i, j;
> > +	int			ioffset;
> > +
> > +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino));
> > +
> > +	nbufs = igeo->ialloc_blks / igeo->blocks_per_cluster;
> > +
> > +	for (j = 0; j < nbufs; j++, inum += igeo->inodes_per_cluster) {
> > +		/*
> > +		 * The allocation bitmap tells us which inodes of the chunk were
> > +		 * physically allocated. Skip the cluster if an inode falls into
> > +		 * a sparse region.
> > +		 */
> > +		ioffset = inum - xic->first_ino;
> > +		if ((xic->alloc & XFS_INOBT_MASK(ioffset)) == 0) {
> > +			ASSERT(ioffset % igeo->inodes_per_cluster == 0);
> > +			continue;
> > +		}
> > +
> > +		for (i = 0; i < igeo->inodes_per_cluster; i++)
> > +			xfs_ifree_abort_inode_stale(pag, inum + i);
> > +
> > +	}
> > +	xfs_perag_put(pag);
> > +}
> > +
> >  /*
> >   * This is called to unpin an inode.  The caller must have the inode locked
> >   * in at least shared mode so that the buffer cannot be subsequently pinned
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index fa780f08dc89..423542bf6af1 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -499,7 +499,6 @@ uint		xfs_ilock_data_map_shared(struct xfs_inode *);
> >  uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
> >  
> >  uint		xfs_ip2xflags(struct xfs_inode *);
> > -int		xfs_ifree(struct xfs_trans *, struct xfs_inode *);
> >  int		xfs_itruncate_extents_flags(struct xfs_trans **,
> >  				struct xfs_inode *, int, xfs_fsize_t, int);
> >  void		xfs_iext_realloc(xfs_inode_t *, int, int);
> > -- 
> > 2.31.1
> >