Re: [PATCH 2/6] xfs: CoW fork operations should only update quota reservations

[Brian Foster <bfoster@redhat.com>]

On Thu, Jan 25, 2018 at 06:05:00PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since the CoW fork only exists in memory, it is incorrect to update the
> on-disk quota block counts when we modify the CoW fork.  Unlike the data
> fork, even real extents in the CoW fork are only reservations (on-disk
> they're owned by the refcountbt) so they must not be tracked in the on
> disk quota info.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Looks sane.. just a couple comments from my braindump on the previous
patch and a few comment suggestions that help me understand this better
(feel free to tweak, rewrite, etc.).

>  fs/xfs/libxfs/xfs_bmap.c |   33 ++++++++++++++++++++++++++++++---
>  fs/xfs/libxfs/xfs_bmap.h |    2 +-
>  fs/xfs/xfs_reflink.c     |   12 ++++++------
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6ad79ea..a59d5be 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3343,8 +3343,26 @@ xfs_bmap_btalloc_accounting(
>  	struct xfs_bmalloca	*ap,
>  	struct xfs_alloc_arg	*args)
>  {
> -	if (!(ap->flags & XFS_BMAPI_COWFORK))
> -		ap->ip->i_d.di_nblocks += args->len;
> +	if (ap->flags & XFS_BMAPI_COWFORK) {

/*
 * COW fork blocks are in-core only and thus are treated as in-core
 * quota reservation (like delalloc blocks) even when converted to real
 * blocks. The quota reservation is not accounted to disk until blocks
 * are remapped to the data fork. So if these blocks were previously
 * delalloc, we already have quota reservation and there's nothing to do
 * yet.
 */

> +		/* Filling a previously reserved extent; nothing to do here. */
> +		if (ap->wasdel)
> +			return;
> +
> +		/*
> +		 * If we get here, we're filling a CoW hole with a real
> +		 * (non-delalloc) CoW extent having reserved enough blocks
> +		 * from both q_res_bcount and qt_blk_res to guarantee that we
> +		 * won't run out of space.  The unused qt_blk_res is given
> +		 * back to q_res_bcount when the transaction commits, so we
> +		 * must decrease qt_blk_res without decreasing q_res_bcount.
> +		 */

/*
 * Otherwise, we've allocated blocks in a hole. The transaction has
 * acquired in-core quota reservation for this extent. Rather than
 * account these as real blocks, however, we reduce the transaction
 * quota reservation based on the allocation. This essentially transfers
 * the transaction quota reservation to that of a delalloc extent.
 */

> +		xfs_trans_mod_dquot_byino(ap->tp, ap->ip, XFS_TRANS_DQ_RES_BLKS,
> +				-(long)args->len);
> +		return;
> +	}
> +
> +	/* data/attr fork only */
> +	ap->ip->i_d.di_nblocks += args->len;
>  	xfs_trans_log_inode(ap->tp, ap->ip, XFS_ILOG_CORE);
>  	if (ap->wasdel)
>  		ap->ip->i_delayed_blks -= args->len;
> @@ -4761,13 +4779,15 @@ xfs_bmap_del_extent_cow(
>  	struct xfs_inode	*ip,
>  	struct xfs_iext_cursor	*icur,
>  	struct xfs_bmbt_irec	*got,
> -	struct xfs_bmbt_irec	*del)
> +	struct xfs_bmbt_irec	*del,
> +	bool			free_quotares)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
>  	struct xfs_bmbt_irec	new;
>  	xfs_fileoff_t		del_endoff, got_endoff;
>  	int			state = BMAP_COWFORK;
> +	int			error;
>  
>  	XFS_STATS_INC(mp, xs_del_exlist);
>  
> @@ -4824,6 +4844,13 @@ xfs_bmap_del_extent_cow(
>  		xfs_iext_insert(ip, icur, &new, state);
>  		break;
>  	}
> +
> +	/* Remove the quota reservation */
> +	if (!free_quotares)
> +		return;
> +	error = xfs_trans_reserve_quota_nblks(NULL, ip,
> +			-(long)del->br_blockcount, 0, XFS_QMOPT_RES_REGBLKS);
> +	ASSERT(error == 0);

Might as well pull this into the only free_quotares = true caller.

Brian

>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index e36d757..e99f28f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -224,7 +224,7 @@ int	xfs_bmap_del_extent_delay(struct xfs_inode *ip, int whichfork,
>  		struct xfs_bmbt_irec *del);
>  void	xfs_bmap_del_extent_cow(struct xfs_inode *ip,
>  		struct xfs_iext_cursor *cur, struct xfs_bmbt_irec *got,
> -		struct xfs_bmbt_irec *del);
> +		struct xfs_bmbt_irec *del, bool free_quotares);
>  uint	xfs_default_attroffset(struct xfs_inode *ip);
>  int	xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
>  		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 82abff6..3644a08 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -599,10 +599,6 @@ xfs_reflink_cancel_cow_blocks(
>  					del.br_startblock, del.br_blockcount,
>  					NULL);
>  
> -			/* Update quota accounting */
> -			xfs_trans_mod_dquot_byino(*tpp, ip, XFS_TRANS_DQ_BCOUNT,
> -					-(long)del.br_blockcount);
> -
>  			/* Roll the transaction */
>  			xfs_defer_ijoin(&dfops, ip);
>  			error = xfs_defer_finish(tpp, &dfops);
> @@ -612,7 +608,7 @@ xfs_reflink_cancel_cow_blocks(
>  			}
>  
>  			/* Remove the mapping from the CoW fork. */
> -			xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
> +			xfs_bmap_del_extent_cow(ip, &icur, &got, &del, true);
>  		} else {
>  			/* Didn't do anything, push cursor back. */
>  			xfs_iext_prev(ifp, &icur);
> @@ -795,8 +791,12 @@ xfs_reflink_end_cow(
>  		if (error)
>  			goto out_defer;
>  
> +		/* Charge this new data fork mapping to the on-disk quota. */
> +		xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
> +				(long)del.br_blockcount);
> +
>  		/* Remove the mapping from the CoW fork. */
> -		xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
> +		xfs_bmap_del_extent_cow(ip, &icur, &got, &del, false);
>  
>  		xfs_defer_ijoin(&dfops, ip);
>  		error = xfs_defer_finish(&tp, &dfops);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html