Re: [PATCH v2] xfs: prevent close() from hanging on frozen filesystems

[Christoph Hellwig]

Hi Aditya,

this looks mosltly good.  A few procedural comments first, I'll then
move on to the code nitpicks.

 - please don't respond to the previous patch, always post standalone.
   git₋send-email gets this right.
 - a lot of the history in this commit log belongs into a cover letter,
   not the commit log itself.  Same for the reproducer.
 - please split this up into multiple patches dong one thing a a time,
   e.g.

xfs: add a XFS_TRANS_TRYLOCK flag
xfs: prevent close() from hanging on frozen filesystems

> A simple GPLv2-licensed C reproducer demonstrating the hang (compile
> with -pthread):

Can you also wire this up to xfstests?

> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 0ab00615f1ad..56c7919bf1e5 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -574,7 +574,8 @@ xfs_can_free_eofblocks(
>   */
>  int
>  xfs_free_eofblocks(
> -	struct xfs_inode	*ip)
> +	struct xfs_inode	*ip,
> +	uint			flags)

Maybe name this trans_flags so that the usage sticks out?

> @@ -1807,8 +1807,17 @@ xfs_file_release(
>  	if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
>  	    xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
>  		if (xfs_can_free_eofblocks(ip) &&
> -		    !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED))
> -			xfs_free_eofblocks(ip);
> +		    !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED)) {
> +			int error = xfs_free_eofblocks(ip, XFS_TRANS_TRYLOCK);
> +
> +			/*
> +			 * If transaction allocation fails due to a frozen/freezing
> +			 * filesystem, clear the released flag so that subsequent
> +			 * releases or background blockgc can retry post-thaw.
> +			 */
> +			if (error == -EAGAIN)
> +				xfs_iflags_clear(ip, XFS_EOFBLOCKS_RELEASED);

The comment has two overly long lines.

Also the resetting the flag on error is a bit odd.  As far as I can tell
there is no need to clear it before the action, so I think we can just
move to clearing it only on successful return, e.g.:

	if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
	    xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
                if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
		    xfs_can_free_eofblocks(ip) &&
		    !xfs_free_eofblocks(ip, XFS_TRANS_TRYLOCK))
			xfs_iflags_set(ip, XFS_EOFBLOCKS_RELEASED);


> +	if (!(flags & XFS_TRANS_NO_WRITECOUNT)) {
> +		/*
> +		 * If TRYLOCK is specified, attempt a non-blocking lock to
> +		 * avoid blocking indefinitely on frozen/freezing filesystems.
> +		 */

This would be a bit cleaner as:

	if (flags & XFS_TRANS_TRYLOCK) {
		if (!sb_start_intwrite_trylock(mp->m_super)) {
			kmem_cache_free(xfs_trans_cache, tp);
			return ERR_PTR(-EAGAIN);
		}
	} else if (!(flags & XFS_TRANS_NO_WRITECOUNT)) {
		sb_start_intwrite(mp->m_super);
	}

And maybe the flag name would better match XFS_TRANS_NO_WRITECOUNT
a bit, e.g. XFS_TRANS_WRITECOUNT_TRYLOCK?

Maybe also add an assert that XFS_TRANS_NO_WRITECOUNT and
XFS_TRANS_NO_WRITECOUNT are not specified at the same time.