Re: [PATCH, RFCRAP] xfs: handle ENOSPC quota return in xfs_file_buffered_aio_write

[Brian Foster <bfoster@redhat.com>]

On Thu, May 14, 2020 at 04:43:36PM -0500, Eric Sandeen wrote:
> Since project quota returns ENOSPC rather than EDQUOT, the distinction
> between those two error conditions in xfs_file_buffered_aio_write() is
> incomplete.  If project quota is on, and we get ENOSPC, it seems that
> we have no good way to know if it was due to quota, or due to actual
> out of space conditions, and we may need to run both remediation options...
> 
> This is completely untested and not even built, because I'm really not sure
> what the best way to go here is.  True ENOSPC is hopefully rare, pquota
> ENOSPC is probably less so, so I'm not sure how far we should go digging
> to figure out what the root cause of the ENOSPC condition is, when pquota
> is on (on this inode).
> 
> If project quota is on on this inode and pquota enforced, should we look
> to the super to make a determination about low free blocks in the fs?
> 
> Should we crack open the dquot and see if it's over limit?
> 
> Should we just run both conditions and hope for the best?
> 
> Is this all best effort anyway, so we just simply care if we take the
> improper action for pquota+ENOSPC?
> 
> Probably-shouldn't-merge-this-sez: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 4b8bdecc3863..8cec826046ce 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -647,27 +647,31 @@ xfs_file_buffered_aio_write(
>  	 * waits on dirty mappings. Since xfs_flush_inodes() is serialized, this
>  	 * also behaves as a filter to prevent too many eofblocks scans from
>  	 * running at the same time.
> +	 *
> +	 * Fun fact: Project quota returns -ENOSPC, so... complexity here.
>  	 */
> -	if (ret == -EDQUOT && !enospc) {
> -		xfs_iunlock(ip, iolock);
> -		enospc = xfs_inode_free_quota_eofblocks(ip);
> -		if (enospc)
> -			goto write_retry;
> -		enospc = xfs_inode_free_quota_cowblocks(ip);
> +	if (!enospc) {
> +		if (ret == -ENOSPC) {
> +			struct xfs_eofblocks eofb = {0};
> +	
> +			enospc = 1;
> +			xfs_flush_inodes(ip->i_mount);
> +	
> +			xfs_iunlock(ip, iolock);
> +			eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
> +			xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> +			xfs_icache_free_cowblocks(ip->i_mount, &eofb);
> +		}
> +		if (ret == -EDQUOT ||
> +		    (ret == -ENOSPC && ip->i_pdquot &&
> +		     XFS_IS_PQUOTA_ENFORCED(mp) && ip->i_pdquot)) {
> +			xfs_iunlock(ip, iolock);
> +			enospc |= xfs_inode_free_quota_eofblocks(ip);
> +			enospc |= xfs_inode_free_quota_cowblocks(ip);
> +			iolock = 0;
> +		}

Christoph's comment aside, note that the quota helpers here are filtered
scans based on the dquots attached to the inode. It's basically an
optimized scan when we know the failure was due to quota, so I don't
think there should ever be a need to run a quota scan after running the
-ENOSPC handling above. For project quota, it might make more sense to
check if a pdquot is attached, check xfs_dquot_lowsp() and conditionally
update the eofb to do a filtered pquota scan if appropriate (since
calling the quota helper above would also affect other dquots attached
to the inode, which I don't think we want to do). Then we can fall back
to the global scan if the pquota optimization is not relevant or still
returns -ENOSPC on the subsequent retry.

Brian

>  		if (enospc)
>  			goto write_retry;
> -		iolock = 0;
> -	} else if (ret == -ENOSPC && !enospc) {
> -		struct xfs_eofblocks eofb = {0};
> -
> -		enospc = 1;
> -		xfs_flush_inodes(ip->i_mount);
> -
> -		xfs_iunlock(ip, iolock);
> -		eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
> -		xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> -		xfs_icache_free_cowblocks(ip->i_mount, &eofb);
> -		goto write_retry;
>  	}
>  
>  	current->backing_dev_info = NULL;
>