Re: [PATCH v2] xfs: improve handling of prjquot ENOSPC

[Dave Chinner <david@fromorbit.com>]

On Sat, Dec 16, 2023 at 11:35:22PM +0800, Jian Wen wrote:
> Don't clear space of the whole fs when the project quota limit is
> reached, since it affects the writing performance of files of
> the directories that are under quota.
> 
> Changes since v1:
> - use the want_blockgc_free_quota helper that written by Darrick

I'm not convinced this is correct behaviour.

That is, we can get a real full filesystem ENOSPC even when project
quotas are on and the the project quota space is low. With this
change we will only flush project quotas rather than the whole
filesystem.

That seems like scope for real world ENOSPC regressions when project
quotas are enabled.

Hence my suggestion that we should be returning -EDQUOT from project
quotas and only converting it to -ENOSPC once the project quota has
been flushed and failed with EDQUOT a second time.

Keep in mind that I'm not interested in changing this code to
simplify it - this EDQUOT/ENOSPC flushing is replicated across
multiple fuinctions and so -all- of them need to change, not just
the buffered write path.

IOWs, I'm interested in having the code behave correctly in these
situations. If correctness means the code has to become more
complex, then so be it. However, with some simple refactoring, we
can isolate the complexity and make the code simpler.

> Signed-off-by: Jian Wen <wenjian1@xiaomi.com>
> ---
>  fs/xfs/xfs_file.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index e33e5e13b95f..7764697e7822 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -24,6 +24,9 @@
>  #include "xfs_pnfs.h"
>  #include "xfs_iomap.h"
>  #include "xfs_reflink.h"
> +#include "xfs_quota.h"
> +#include "xfs_dquot_item.h"
> +#include "xfs_dquot.h"
>  
>  #include <linux/dax.h>
>  #include <linux/falloc.h>
> @@ -761,6 +764,20 @@ xfs_file_dax_write(
>  	return ret;
>  }
>  
> +static inline bool want_blockgc_free_quota(struct xfs_inode *ip, int ret)
> +{
> +	if (ret == -EDQUOT)
> +		return true;
> +	if (ret != -ENOSPC)
> +		return false;
> +
> +	if (XFS_IS_PQUOTA_ENFORCED(ip->i_mount) &&
> +	    ip->i_pdquot && xfs_dquot_lowsp(ip->i_pdquot))
> +		return true;
> +
> +	return false;
> +}

>  STATIC ssize_t
>  xfs_file_buffered_write(
>  	struct kiocb		*iocb,
> @@ -796,7 +813,7 @@ xfs_file_buffered_write(
>  	 * running at the same time.  Use a synchronous scan to increase the
>  	 * effectiveness of the scan.
>  	 */
> -	if (ret == -EDQUOT && !cleared_space) {
> +	if (want_blockgc_free_quota(ip, ret) && !cleared_space) {
>  		xfs_iunlock(ip, iolock);
>  		xfs_blockgc_free_quota(ip, XFS_ICWALK_FLAG_SYNC);
>  		cleared_space = true;

IMO, this makes messy code even more messy, and makes it even more
inconsistent with all the EDQUOT/ENOSPC flushing that is done in the
xfs_trans_alloc_*() inode transaction setup helpers.

So, with the assumption that project quotas return EDQUOT and not
ENOSPC, we add this helper to fs/xfs/xfs_dquot.h:

static inline bool
xfs_dquot_is_enospc(
	struct xfs_dquot	*dqp)
{
	if (!dqp)
		return false;
	if (!xfs_dquot_is_enforced(dqp)
		return false;
	if (dqp->q_blk.hardlimit - dqp->q_blk.reserved > 0)
		return false;
	return true;
}

And this helper to fs/xfs/xfs_icache.c:

static void
xfs_blockgc_nospace_flush(
	struct xfs_inode	*ip,
	int			what)
{
	if (what == -EDQUOT) {
		xfs_blockgc_free_quota(ip, XFS_ICWALK_FLAG_SYNC);
		return;
	}
	ASSERT(what == -ENOSPC);

	xfs_flush_inodes(ip->i_mount);
	icw.icw_flags = XFS_ICWALK_FLAG_SYNC;
	xfs_blockgc_free_space(ip->i_mount, &icw);
}

The buffered write code ends up as:

	.....
	do {
		iolock = XFS_IOLOCK_EXCL;
		ret = xfs_ilock_iocb(iocb, iolock);
		if (ret)
			return ret;

		ret = xfs_file_write_checks(iocb, from, &iolock);
		if (ret)
			goto out;

		trace_xfs_file_buffered_write(iocb, from);
		ret = iomap_file_buffered_write(iocb, from,
				&xfs_buffered_write_iomap_ops);
		if (!(ret == -EDQUOT || ret = -ENOSPC))
			break;

		xfs_iunlock(ip, iolock);
		xfs_blockgc_nospace_flush(ip, ret);
	} while (retries++ == 0);

	if (ret == -EDQUOT && xfs_dquot_is_enospc(ip->i_pdquot))
		ret = -ENOSPC;
	.....

This factors out the actual flushing behaviour when an error occurs
from the write code, and removes the clunky goto from the code. It
now clearly loops on a single retry after a ENOSPC/EDQUOT error,
and the high level code transforms the EDQUOT project quota error
once the loop errors out completely.

We can then do the same transformation to xfs_trans_alloc_icreate(),
xfs_trans_alloc_inode(), xfs_trans_alloc_ichange() and
xfs_trans_alloc_dir() using xfs_blockgc_nospace_flush() and
xfs_dquot_is_enospc().

This will then give us consistent project quota only flushing on
project quota failure, as well as consistent full filesystem ENOSPC
flushing behaviour across all types of inode operations.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com