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

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

[Eric Sandeen]

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;
+		}
 		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;

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

[Christoph Hellwig]

On Thu, May 14, 2020 at 04:43:36PM -0500, Eric Sandeen wrote:
> +		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;

Fun fact of the day: xfs_inode_free_quota_eofblocks and
xfs_inode_free_quota_cowblocks don't actually do anything for project
quotas.  I've started to prepare a little cleanup series to help
implementing what you want.  Give me some time to do a quick test
run and send it out.

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

[Brian Foster]

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

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

[Christoph Hellwig]

On Mon, May 18, 2020 at 08:34:54AM -0400, Brian Foster wrote:
> 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.

That's what I've implemented.  But it turns out -ENOSPC can of course
still mean a real -ENOSPC even with project quotas attached.  So back
to the drawing board - I think I basically need to replace the enospc
with a tristate saying what kind of scan we've tried.  Or we just ignore
the issue and keep the current global scan after a potential project
quota -ENOSPC, because all that cruft isn't worth it after all.

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

[Brian Foster]

On Mon, May 18, 2020 at 10:01:12AM -0700, Christoph Hellwig wrote:
> On Mon, May 18, 2020 at 08:34:54AM -0400, Brian Foster wrote:
> > 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.
> 
> That's what I've implemented.  But it turns out -ENOSPC can of course
> still mean a real -ENOSPC even with project quotas attached.  So back
> to the drawing board - I think I basically need to replace the enospc
> with a tristate saying what kind of scan we've tried.  Or we just ignore
> the issue and keep the current global scan after a potential project
> quota -ENOSPC, because all that cruft isn't worth it after all.
> 

Sure, that's why I was suggesting to check the quota for low free space
conditions. One one hand, a quota scan might be worth it under low quota
space conditions to avoid the heavy handed impact (i.e. flush
everything) on an fs that otherwise might have plenty of free space.
OTOH, it might be pointless if permanent -ENOSPC (due to project quota)
is imminent and we always fall back to the global scan.

Brian