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

[Long Li <leo.lilong@huawei.com>]

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
> >