Re: [PATCH 2/2 V2] xfs: always return -ENOSPC on project quota reservation failure

[Brian Foster <bfoster@redhat.com>]

On Wed, May 13, 2020 at 12:39:00PM -0500, Eric Sandeen wrote:
> XFS project quota treats project hierarchies as "mini filesysems" and
> so rather than -EDQUOT, the intent is to return -ENOSPC when a quota
> reservation fails, but this behavior is not consistent.
> 
> The only place we make a decision between -EDQUOT and -ENOSPC
> returns based on quota type is in xfs_trans_dqresv().
> 
> This behavior is currently controlled by whether or not the
> XFS_QMOPT_ENOSPC flag gets passed into the quota reservation.  However,
> its use is not consistent; paths such as xfs_create() and xfs_symlink()
> don't set the flag, so a reservation failure will return -EDQUOT for
> project quota reservation failures rather than -ENOSPC for these sorts
> of operations, even for project quota:
> 
> # mkdir mnt/project
> # xfs_quota -x -c "project -s -p mnt/project 42" mnt
> # xfs_quota -x -c 'limit -p isoft=2 ihard=3 42' mnt
> # touch mnt/project/file{1,2,3}
> touch: cannot touch ‘mnt/project/file3’: Disk quota exceeded
> 
> We can make this consistent by not requiring the flag to be set at the
> top of the callchain; instead we can simply test whether we are
> reserving a project quota with XFS_QM_ISPDQ in xfs_trans_dqresv and if
> so, return -ENOSPC for that failure.  This removes the need for the
> XFS_QMOPT_ENOSPC altogether and simplifies the code a fair bit.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---

Thanks for the update.

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> V2: Change title and changelog to clearly reflect the bug and behavior
> change, which wasn't really identified as a bug and behavior change until
> Brian carefully reviewed V1.  :)
> 
> No patch changes.
> 
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index b2113b17e53c..56d9dd787e7b 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -100,7 +100,6 @@ typedef uint16_t	xfs_qwarncnt_t;
>  #define XFS_QMOPT_FORCE_RES	0x0000010 /* ignore quota limits */
>  #define XFS_QMOPT_SBVERSION	0x0000040 /* change superblock version num */
>  #define XFS_QMOPT_GQUOTA	0x0002000 /* group dquot requested */
> -#define XFS_QMOPT_ENOSPC	0x0004000 /* enospc instead of edquot (prj) */
>  
>  /*
>   * flags to xfs_trans_mod_dquot to indicate which field needs to be
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index c225691fad15..591779aa2fd0 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1808,7 +1808,7 @@ xfs_qm_vop_chown_reserve(
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	uint64_t		delblks;
> -	unsigned int		blkflags, prjflags = 0;
> +	unsigned int		blkflags;
>  	struct xfs_dquot	*udq_unres = NULL;
>  	struct xfs_dquot	*gdq_unres = NULL;
>  	struct xfs_dquot	*pdq_unres = NULL;
> @@ -1849,7 +1849,6 @@ xfs_qm_vop_chown_reserve(
>  
>  	if (XFS_IS_PQUOTA_ON(ip->i_mount) && pdqp &&
>  	    ip->i_d.di_projid != be32_to_cpu(pdqp->q_core.d_id)) {
> -		prjflags = XFS_QMOPT_ENOSPC;
>  		pdq_delblks = pdqp;
>  		if (delblks) {
>  			ASSERT(ip->i_pdquot);
> @@ -1859,8 +1858,7 @@ xfs_qm_vop_chown_reserve(
>  
>  	error = xfs_trans_reserve_quota_bydquots(tp, ip->i_mount,
>  				udq_delblks, gdq_delblks, pdq_delblks,
> -				ip->i_d.di_nblocks, 1,
> -				flags | blkflags | prjflags);
> +				ip->i_d.di_nblocks, 1, flags | blkflags);
>  	if (error)
>  		return error;
>  
> @@ -1878,8 +1876,7 @@ xfs_qm_vop_chown_reserve(
>  		ASSERT(udq_unres || gdq_unres || pdq_unres);
>  		error = xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
>  			    udq_delblks, gdq_delblks, pdq_delblks,
> -			    (xfs_qcnt_t)delblks, 0,
> -			    flags | blkflags | prjflags);
> +			    (xfs_qcnt_t)delblks, 0, flags | blkflags);
>  		if (error)
>  			return error;
>  		xfs_trans_reserve_quota_bydquots(NULL, ip->i_mount,
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 2c3557a80e69..2c07897a3c37 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -711,7 +711,7 @@ xfs_trans_dqresv(
>  
>  error_return:
>  	xfs_dqunlock(dqp);
> -	if (flags & XFS_QMOPT_ENOSPC)
> +	if (XFS_QM_ISPDQ(dqp))
>  		return -ENOSPC;
>  	return -EDQUOT;
>  }
> @@ -751,15 +751,13 @@ xfs_trans_reserve_quota_bydquots(
>  	ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>  
>  	if (udqp) {
> -		error = xfs_trans_dqresv(tp, mp, udqp, nblks, ninos,
> -					(flags & ~XFS_QMOPT_ENOSPC));
> +		error = xfs_trans_dqresv(tp, mp, udqp, nblks, ninos, flags);
>  		if (error)
>  			return error;
>  	}
>  
>  	if (gdqp) {
> -		error = xfs_trans_dqresv(tp, mp, gdqp, nblks, ninos,
> -					(flags & ~XFS_QMOPT_ENOSPC));
> +		error = xfs_trans_dqresv(tp, mp, gdqp, nblks, ninos, flags);
>  		if (error)
>  			goto unwind_usr;
>  	}
> @@ -804,16 +802,12 @@ xfs_trans_reserve_quota_nblks(
>  
>  	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>  		return 0;
> -	if (XFS_IS_PQUOTA_ON(mp))
> -		flags |= XFS_QMOPT_ENOSPC;
>  
>  	ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
>  
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -	ASSERT((flags & ~(XFS_QMOPT_FORCE_RES | XFS_QMOPT_ENOSPC)) ==
> -				XFS_TRANS_DQ_RES_RTBLKS ||
> -	       (flags & ~(XFS_QMOPT_FORCE_RES | XFS_QMOPT_ENOSPC)) ==
> -				XFS_TRANS_DQ_RES_BLKS);
> +	ASSERT((flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_RTBLKS ||
> +	       (flags & ~(XFS_QMOPT_FORCE_RES)) == XFS_TRANS_DQ_RES_BLKS);
>  
>  	/*
>  	 * Reserve nblks against these dquots, with trans as the mediator.
> 
> 
>