On Wed, Jan 27, 2021 at 10:02:01PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Now that we've converted xfs_reflink_remap_extent to use the new
> xfs_trans_alloc_inode API, we can focus on its slightly unusual behavior
> with regard to quota reservations.
>
> Since it's valid to remap written blocks into a hole, we must be able to
> increase the quota count by the number of blocks in the mapping.
> However, the incore space reservation process requires us to supply an
> asymptotic guess before we can gain exclusive access to resources. We'd
> like to reserve all the quota we need up front, but we also don't want
> to fail a written -> allocated remap operation unnecessarily.
>
> The solution is to make the remap_extents function call the transaction
> allocation function twice. The first time we ask to reserve enough
> space and quota to handle the absolute worst case situation, but if that
> fails, we can fall back to the old strategy: ask for the bare minimum
> space reservation upfront and increase the quota reservation later if we
> need to.
>
> This isn't a problem now, but in the next patchset we change the
> transaction and quota code to try to reclaim space if we cannot reserve
> free space or quota. Restructuring the remap_extent function in this
> manner means that if the fallback increase fails, we can pass that back
> to the caller knowing that the transaction allocation already tried
> freeing space.
>
This looks pretty good to me, but I'd probably move this patch closer to
the patch that depends on it..
Brian
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_reflink.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index ded86cc4764c..c6296fd1512f 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -992,6 +992,7 @@ xfs_reflink_remap_extent(
> xfs_off_t newlen;
> int64_t qdelta = 0;
> unsigned int resblks;
> + bool quota_reserved = true;
> bool smap_real;
> bool dmap_written = xfs_bmap_is_written_extent(dmap);
> int iext_delta = 0;
> @@ -1007,10 +1008,26 @@ xfs_reflink_remap_extent(
> * the same index in the bmap btree, so we only need a reservation for
> * one bmbt split if either thing is happening. However, we haven't
> * locked the inode yet, so we reserve assuming this is the case.
> + *
> + * The first allocation call tries to reserve enough space to handle
> + * mapping dmap into a sparse part of the file plus the bmbt split. We
> + * haven't locked the inode or read the existing mapping yet, so we do
> + * not know for sure that we need the space. This should succeed most
> + * of the time.
> + *
> + * If the first attempt fails, try again but reserving only enough
> + * space to handle a bmbt split. This is the hard minimum requirement,
> + * and we revisit quota reservations later when we know more about what
> + * we're remapping.
> */
> resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
> - error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
> - false, &tp);
> + error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
> + resblks + dmap->br_blockcount, 0, false, &tp);
> + if (error == -EDQUOT || error == -ENOSPC) {
> + quota_reserved = false;
> + error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write,
> + resblks, 0, false, &tp);
> + }
> if (error)
> goto out;
>
> @@ -1077,7 +1094,7 @@ xfs_reflink_remap_extent(
> * before we started. That should have removed all the delalloc
> * reservations, but we code defensively.
> */
> - if (!smap_real && dmap_written) {
> + if (!quota_reserved && !smap_real && dmap_written) {
> error = xfs_trans_reserve_quota_nblks(tp, ip,
> dmap->br_blockcount, 0, false);
> if (error)
>
