diff mbox series

[09/13] xfs: refactor reflink functions to use xfs_trans_alloc_inode

Message ID 161181371557.1523592.14364313318301403930.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: minor cleanups of the quota functions | expand

Commit Message

Darrick J. Wong Jan. 28, 2021, 6:01 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

The two remaining callers of xfs_trans_reserve_quota_nblks are in the
reflink code.  These conversions aren't as uniform as the previous
conversions, so call that out in a separate patch.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_reflink.c |   58 +++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

Comments

Christoph Hellwig Jan. 28, 2021, 9:53 a.m. UTC | #1 
On Wed, Jan 27, 2021 at 10:01:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The two remaining callers of xfs_trans_reserve_quota_nblks are in the
> reflink code.  These conversions aren't as uniform as the previous
> conversions, so call that out in a separate patch.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_reflink.c |   58 +++++++++++++++++++++-----------------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 0778b5810c26..ded86cc4764c 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -376,16 +376,15 @@ xfs_reflink_allocate_cow(
>  	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
>  
>  	xfs_iunlock(ip, *lockmode);
>  
> +	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
> +			false, &tp);
> +	if (error) {
> +		/* This function must return with ILOCK_EXCL held. */
> +		*lockmode = XFS_ILOCK_EXCL;
> +		xfs_ilock(ip, *lockmode);
>  		return error;
> +	}

The only thing that the only caller of xfs_reflink_allocate_cow does
on error is to immediately release the lock.  So I think we are better
off changing the calling convention instead of relocking here.
Darrick J. Wong Jan. 28, 2021, 6:06 p.m. UTC | #2 
On Thu, Jan 28, 2021 at 09:53:45AM +0000, Christoph Hellwig wrote:
> On Wed, Jan 27, 2021 at 10:01:55PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The two remaining callers of xfs_trans_reserve_quota_nblks are in the
> > reflink code.  These conversions aren't as uniform as the previous
> > conversions, so call that out in a separate patch.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_reflink.c |   58 +++++++++++++++++++++-----------------------------
> >  1 file changed, 24 insertions(+), 34 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 0778b5810c26..ded86cc4764c 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -376,16 +376,15 @@ xfs_reflink_allocate_cow(
> >  	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
> >  
> >  	xfs_iunlock(ip, *lockmode);
> >  
> > +	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
> > +			false, &tp);
> > +	if (error) {
> > +		/* This function must return with ILOCK_EXCL held. */
> > +		*lockmode = XFS_ILOCK_EXCL;
> > +		xfs_ilock(ip, *lockmode);
> >  		return error;
> > +	}
> 
> The only thing that the only caller of xfs_reflink_allocate_cow does
> on error is to immediately release the lock.  So I think we are better
> off changing the calling convention instead of relocking here.

Ok, will do.

--D
Brian Foster Jan. 28, 2021, 6:23 p.m. UTC | #3 
On Wed, Jan 27, 2021 at 10:01:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The two remaining callers of xfs_trans_reserve_quota_nblks are in the
> reflink code.  These conversions aren't as uniform as the previous
> conversions, so call that out in a separate patch.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Modulo Christoph's feedback on the locking:

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

>  fs/xfs/xfs_reflink.c |   58 +++++++++++++++++++++-----------------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 0778b5810c26..ded86cc4764c 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -376,16 +376,15 @@ xfs_reflink_allocate_cow(
>  	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
>  
>  	xfs_iunlock(ip, *lockmode);
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> -	*lockmode = XFS_ILOCK_EXCL;
> -	xfs_ilock(ip, *lockmode);
>  
> -	if (error)
> +	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
> +			false, &tp);
> +	if (error) {
> +		/* This function must return with ILOCK_EXCL held. */
> +		*lockmode = XFS_ILOCK_EXCL;
> +		xfs_ilock(ip, *lockmode);
>  		return error;
> -
> -	error = xfs_qm_dqattach_locked(ip, false);
> -	if (error)
> -		goto out_trans_cancel;
> +	}
>  
>  	/*
>  	 * Check for an overlapping extent again now that we dropped the ilock.
> @@ -398,12 +397,6 @@ xfs_reflink_allocate_cow(
>  		goto convert;
>  	}
>  
> -	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
> -	if (error)
> -		goto out_trans_cancel;
> -
> -	xfs_trans_ijoin(tp, ip, 0);
> -
>  	/* Allocate the entire reservation as unwritten blocks. */
>  	nimaps = 1;
>  	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
> @@ -997,7 +990,7 @@ xfs_reflink_remap_extent(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_trans	*tp;
>  	xfs_off_t		newlen;
> -	int64_t			qres, qdelta;
> +	int64_t			qdelta = 0;
>  	unsigned int		resblks;
>  	bool			smap_real;
>  	bool			dmap_written = xfs_bmap_is_written_extent(dmap);
> @@ -1005,15 +998,22 @@ xfs_reflink_remap_extent(
>  	int			nimaps;
>  	int			error;
>  
> -	/* Start a rolling transaction to switch the mappings */
> +	/*
> +	 * Start a rolling transaction to switch the mappings.
> +	 *
> +	 * Adding a written extent to the extent map can cause a bmbt split,
> +	 * and removing a mapped extent from the extent can cause a bmbt split.
> +	 * The two operations cannot both cause a split since they operate on
> +	 * 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.
> +	 */
>  	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
> -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> +	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
> +			false, &tp);
>  	if (error)
>  		goto out;
>  
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -	xfs_trans_ijoin(tp, ip, 0);
> -
>  	/*
>  	 * Read what's currently mapped in the destination file into smap.
>  	 * If smap isn't a hole, we will have to remove it before we can add
> @@ -1061,15 +1061,9 @@ xfs_reflink_remap_extent(
>  	}
>  
>  	/*
> -	 * Compute quota reservation if we think the quota block counter for
> +	 * Increase quota reservation if we think the quota block counter for
>  	 * this file could increase.
>  	 *
> -	 * Adding a written extent to the extent map can cause a bmbt split,
> -	 * and removing a mapped extent from the extent can cause a bmbt split.
> -	 * The two operations cannot both cause a split since they operate on
> -	 * the same index in the bmap btree, so we only need a reservation for
> -	 * one bmbt split if either thing is happening.
> -	 *
>  	 * If we are mapping a written extent into the file, we need to have
>  	 * enough quota block count reservation to handle the blocks in that
>  	 * extent.  We log only the delta to the quota block counts, so if the
> @@ -1083,13 +1077,9 @@ xfs_reflink_remap_extent(
>  	 * before we started.  That should have removed all the delalloc
>  	 * reservations, but we code defensively.
>  	 */
> -	qres = qdelta = 0;
> -	if (smap_real || dmap_written)
> -		qres = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
> -	if (!smap_real && dmap_written)
> -		qres += dmap->br_blockcount;
> -	if (qres > 0) {
> -		error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0, false);
> +	if (!smap_real && dmap_written) {
> +		error = xfs_trans_reserve_quota_nblks(tp, ip,
> +				dmap->br_blockcount, 0, false);
>  		if (error)
>  			goto out_cancel;
>  	}
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0778b5810c26..ded86cc4764c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -376,16 +376,15 @@  xfs_reflink_allocate_cow(
 	resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 
 	xfs_iunlock(ip, *lockmode);
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
-	*lockmode = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, *lockmode);
 
-	if (error)
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
+			false, &tp);
+	if (error) {
+		/* This function must return with ILOCK_EXCL held. */
+		*lockmode = XFS_ILOCK_EXCL;
+		xfs_ilock(ip, *lockmode);
 		return error;
-
-	error = xfs_qm_dqattach_locked(ip, false);
-	if (error)
-		goto out_trans_cancel;
+	}
 
 	/*
 	 * Check for an overlapping extent again now that we dropped the ilock.
@@ -398,12 +397,6 @@  xfs_reflink_allocate_cow(
 		goto convert;
 	}
 
-	error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0, false);
-	if (error)
-		goto out_trans_cancel;
-
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/* Allocate the entire reservation as unwritten blocks. */
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
@@ -997,7 +990,7 @@  xfs_reflink_remap_extent(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	xfs_off_t		newlen;
-	int64_t			qres, qdelta;
+	int64_t			qdelta = 0;
 	unsigned int		resblks;
 	bool			smap_real;
 	bool			dmap_written = xfs_bmap_is_written_extent(dmap);
@@ -1005,15 +998,22 @@  xfs_reflink_remap_extent(
 	int			nimaps;
 	int			error;
 
-	/* Start a rolling transaction to switch the mappings */
+	/*
+	 * Start a rolling transaction to switch the mappings.
+	 *
+	 * Adding a written extent to the extent map can cause a bmbt split,
+	 * and removing a mapped extent from the extent can cause a bmbt split.
+	 * The two operations cannot both cause a split since they operate on
+	 * 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.
+	 */
 	resblks = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
-	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, resblks, 0,
+			false, &tp);
 	if (error)
 		goto out;
 
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/*
 	 * Read what's currently mapped in the destination file into smap.
 	 * If smap isn't a hole, we will have to remove it before we can add
@@ -1061,15 +1061,9 @@  xfs_reflink_remap_extent(
 	}
 
 	/*
-	 * Compute quota reservation if we think the quota block counter for
+	 * Increase quota reservation if we think the quota block counter for
 	 * this file could increase.
 	 *
-	 * Adding a written extent to the extent map can cause a bmbt split,
-	 * and removing a mapped extent from the extent can cause a bmbt split.
-	 * The two operations cannot both cause a split since they operate on
-	 * the same index in the bmap btree, so we only need a reservation for
-	 * one bmbt split if either thing is happening.
-	 *
 	 * If we are mapping a written extent into the file, we need to have
 	 * enough quota block count reservation to handle the blocks in that
 	 * extent.  We log only the delta to the quota block counts, so if the
@@ -1083,13 +1077,9 @@  xfs_reflink_remap_extent(
 	 * before we started.  That should have removed all the delalloc
 	 * reservations, but we code defensively.
 	 */
-	qres = qdelta = 0;
-	if (smap_real || dmap_written)
-		qres = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
-	if (!smap_real && dmap_written)
-		qres += dmap->br_blockcount;
-	if (qres > 0) {
-		error = xfs_trans_reserve_quota_nblks(tp, ip, qres, 0, false);
+	if (!smap_real && dmap_written) {
+		error = xfs_trans_reserve_quota_nblks(tp, ip,
+				dmap->br_blockcount, 0, false);
 		if (error)
 			goto out_cancel;
 	}