[PATCH 0/7] xfs: reflink fixes

[PATCH 0/7] xfs: reflink fixes

[Darrick J. Wong]

Hi all,

Here's all the fixes[1] for reflink I've collected against the latest
for-next kernel branches.  They're all minor bug fixes that came up
in review or on irc.  They've survived a quick go-round with the
clone tests.

--D

[1] https://github.com/djwong/linux/tree/for-dave-for-4.9-14

[PATCH 1/7] xfs: rework refcount cow recovery error handling

[Darrick J. Wong]

The error handling in xfs_refcount_recover_cow_leftovers is confused
and can potentially leak memory, so rework it to release resources
correctly on error.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_refcount.c |   18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 56bfef1..87f00df 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
 	error = xfs_btree_query_range(cur, &low, &high,
 			xfs_refcount_recover_extent, &debris);
 	if (error)
-		goto out_error;
+		goto out_cursor;
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
 	xfs_buf_relse(agbp);
 
@@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
 
 		error = xfs_trans_commit(tp);
 		if (error)
-			goto out_cancel;
+			goto out_free;
 	}
-	goto out_free;
+	return 0;
 
 out_defer:
 	xfs_defer_cancel(&dfops);
-out_cancel:
 	xfs_trans_cancel(tp);
-
+	goto out_free;
+out_cursor:
+	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+	xfs_buf_relse(agbp);
 out_free:
 	/* Free the leftover list */
 	list_for_each_entry_safe(rr, n, &debris, rr_list) {
 		list_del(&rr->rr_list);
 		kmem_free(rr);
 	}
-
-	return error;
-
-out_error:
-	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-	xfs_buf_relse(agbp);
 	return error;
 }

Re: [PATCH 1/7] xfs: rework refcount cow recovery error handling

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:07:25PM -0700, Darrick J. Wong wrote:
> The error handling in xfs_refcount_recover_cow_leftovers is confused
> and can potentially leak memory, so rework it to release resources
> correctly on error.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |   18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 56bfef1..87f00df 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
>  	error = xfs_btree_query_range(cur, &low, &high,
>  			xfs_refcount_recover_extent, &debris);
>  	if (error)
> -		goto out_error;
> +		goto out_cursor;
>  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
>  	xfs_buf_relse(agbp);
>  
> @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
>  
>  		error = xfs_trans_commit(tp);
>  		if (error)
> -			goto out_cancel;
> +			goto out_free;
>  	}
> -	goto out_free;
> +	return 0;

How does the debris list entries get freed in this case?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/7] xfs: rework refcount cow recovery error handling

[Darrick J. Wong]

On Mon, Oct 10, 2016 at 04:37:40PM +1100, Dave Chinner wrote:
> On Fri, Oct 07, 2016 at 04:07:25PM -0700, Darrick J. Wong wrote:
> > The error handling in xfs_refcount_recover_cow_leftovers is confused
> > and can potentially leak memory, so rework it to release resources
> > correctly on error.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reported-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |   18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 56bfef1..87f00df 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
> >  	error = xfs_btree_query_range(cur, &low, &high,
> >  			xfs_refcount_recover_extent, &debris);
> >  	if (error)
> > -		goto out_error;
> > +		goto out_cursor;
> >  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> >  	xfs_buf_relse(agbp);
> >  
> > @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
> >  
> >  		error = xfs_trans_commit(tp);
> >  		if (error)
> > -			goto out_cancel;
> > +			goto out_free;
> >  	}
> > -	goto out_free;
> > +	return 0;
> 
> How does the debris list entries get freed in this case?

Heh, serves me right for being overeager in reading sandeen's message.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH v2 1/7] xfs: rework refcount cow recovery error handling

[Darrick J. Wong]

The error handling in xfs_refcount_recover_cow_leftovers is confused
and can potentially leak memory, so rework it to release resources
correctly on error.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_refcount.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 56bfef1..955b1d9 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
 	error = xfs_btree_query_range(cur, &low, &high,
 			xfs_refcount_recover_extent, &debris);
 	if (error)
-		goto out_error;
+		goto out_cursor;
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
 	xfs_buf_relse(agbp);
 
@@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
 
 		error = xfs_trans_commit(tp);
 		if (error)
-			goto out_cancel;
+			goto out_free;
 	}
 	goto out_free;
 
 out_defer:
 	xfs_defer_cancel(&dfops);
-out_cancel:
 	xfs_trans_cancel(tp);
-
+	goto out_free;
+out_cursor:
+	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+	xfs_buf_relse(agbp);
 out_free:
 	/* Free the leftover list */
 	list_for_each_entry_safe(rr, n, &debris, rr_list) {
 		list_del(&rr->rr_list);
 		kmem_free(rr);
 	}
-
-	return error;
-
-out_error:
-	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-	xfs_buf_relse(agbp);
 	return error;
 }

Re: [PATCH v2 1/7] xfs: rework refcount cow recovery error handling

[Christoph Hellwig]

On Sun, Oct 09, 2016 at 11:11:22PM -0700, Darrick J. Wong wrote:
> The error handling in xfs_refcount_recover_cow_leftovers is confused
> and can potentially leak memory, so rework it to release resources
> correctly on error.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |   16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 56bfef1..955b1d9 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
>  	error = xfs_btree_query_range(cur, &low, &high,
>  			xfs_refcount_recover_extent, &debris);
>  	if (error)
> -		goto out_error;
> +		goto out_cursor;
>  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
>  	xfs_buf_relse(agbp);
>  
> @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
>  
>  		error = xfs_trans_commit(tp);
>  		if (error)
> +			goto out_free;
>  	}
>  	goto out_free;

don't we need to also delete the cursor after a successful exit?

Re: [PATCH v2 1/7] xfs: rework refcount cow recovery error handling

[Darrick J. Wong]

On Mon, Oct 10, 2016 at 03:21:13AM -0700, Christoph Hellwig wrote:
> On Sun, Oct 09, 2016 at 11:11:22PM -0700, Darrick J. Wong wrote:
> > The error handling in xfs_refcount_recover_cow_leftovers is confused
> > and can potentially leak memory, so rework it to release resources
> > correctly on error.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reported-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |   16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 56bfef1..955b1d9 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
> >  	error = xfs_btree_query_range(cur, &low, &high,
> >  			xfs_refcount_recover_extent, &debris);
> >  	if (error)
> > -		goto out_error;
> > +		goto out_cursor;
> >  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> >  	xfs_buf_relse(agbp);
> >  
> > @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
> >  
> >  		error = xfs_trans_commit(tp);
> >  		if (error)
> > +			goto out_free;
> >  	}
> >  	goto out_free;
> 
> don't we need to also delete the cursor after a successful exit?

Gah.  Yeah.  Serves me right for trying to code patches on Sunday night.

I think Dave already made a separate fix, though, so I'll wait for the
next for-next update and see what needs to be fixed.

--D

Re: [PATCH v2 1/7] xfs: rework refcount cow recovery error handling

[Dave Chinner]

On Mon, Oct 10, 2016 at 11:21:52AM -0700, Darrick J. Wong wrote:
> On Mon, Oct 10, 2016 at 03:21:13AM -0700, Christoph Hellwig wrote:
> > On Sun, Oct 09, 2016 at 11:11:22PM -0700, Darrick J. Wong wrote:
> > > The error handling in xfs_refcount_recover_cow_leftovers is confused
> > > and can potentially leak memory, so rework it to release resources
> > > correctly on error.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Reported-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_refcount.c |   16 ++++++----------
> > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > > index 56bfef1..955b1d9 100644
> > > --- a/fs/xfs/libxfs/xfs_refcount.c
> > > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > > @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
> > >  	error = xfs_btree_query_range(cur, &low, &high,
> > >  			xfs_refcount_recover_extent, &debris);
> > >  	if (error)
> > > -		goto out_error;
> > > +		goto out_cursor;
> > >  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> > >  	xfs_buf_relse(agbp);

Cursor is freed here when everything works...

I almost modified this to be:

	error = xfs_btree_query_range(cur, &low, &high,
			xfs_refcount_recover_extent, &debris);
	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
	xfs_buf_relse(agbp);
	if (error)
		goto out_free;

so the "out_cursor" branch could go away....

> > > @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
> > >  
> > >  		error = xfs_trans_commit(tp);
> > >  		if (error)
> > > +			goto out_free;
> > >  	}
> > >  	goto out_free;
> > 
> > don't we need to also delete the cursor after a successful exit?
> 
> Gah.  Yeah.  Serves me right for trying to code patches on Sunday night.

It all looked ok to me - what did I miss?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/7] xfs: rework refcount cow recovery error handling

[Darrick J. Wong]

On Tue, Oct 11, 2016 at 07:25:28AM +1100, Dave Chinner wrote:
> On Mon, Oct 10, 2016 at 11:21:52AM -0700, Darrick J. Wong wrote:
> > On Mon, Oct 10, 2016 at 03:21:13AM -0700, Christoph Hellwig wrote:
> > > On Sun, Oct 09, 2016 at 11:11:22PM -0700, Darrick J. Wong wrote:
> > > > The error handling in xfs_refcount_recover_cow_leftovers is confused
> > > > and can potentially leak memory, so rework it to release resources
> > > > correctly on error.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > Reported-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_refcount.c |   16 ++++++----------
> > > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > > > index 56bfef1..955b1d9 100644
> > > > --- a/fs/xfs/libxfs/xfs_refcount.c
> > > > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > > > @@ -1643,7 +1643,7 @@ xfs_refcount_recover_cow_leftovers(
> > > >  	error = xfs_btree_query_range(cur, &low, &high,
> > > >  			xfs_refcount_recover_extent, &debris);
> > > >  	if (error)
> > > > -		goto out_error;
> > > > +		goto out_cursor;
> > > >  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> > > >  	xfs_buf_relse(agbp);
> 
> Cursor is freed here when everything works...
> 
> I almost modified this to be:
> 
> 	error = xfs_btree_query_range(cur, &low, &high,
> 			xfs_refcount_recover_extent, &debris);
> 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> 	xfs_buf_relse(agbp);
> 	if (error)
> 		goto out_free;
> 
> so the "out_cursor" branch could go away....
> 
> > > > @@ -1675,26 +1675,22 @@ xfs_refcount_recover_cow_leftovers(
> > > >  
> > > >  		error = xfs_trans_commit(tp);
> > > >  		if (error)
> > > > +			goto out_free;
> > > >  	}
> > > >  	goto out_free;
> > > 
> > > don't we need to also delete the cursor after a successful exit?
> > 
> > Gah.  Yeah.  Serves me right for trying to code patches on Sunday night.
> 
> It all looked ok to me - what did I miss?

The current code in your for-next tree looks fine.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH 2/7] xfs: check inode reflink flag before calling reflink functions

[Darrick J. Wong]

There are a couple of places where we don't check the inode's
reflink flag before calling into the reflink code.  Fix those,
and add some asserts so we don't make this mistake again.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c |    4 ++--
 fs/xfs/xfs_super.c   |   12 +++++++-----
 2 files changed, 9 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 685c419..9c58b4a 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -624,6 +624,7 @@ xfs_reflink_cancel_cow_range(
 	int			error;
 
 	trace_xfs_reflink_cancel_cow_range(ip, offset, count);
+	ASSERT(xfs_is_reflink_inode(ip));
 
 	offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
 	if (count == NULLFILEOFF)
@@ -1510,8 +1511,7 @@ xfs_reflink_clear_inode_flag(
 	int			nmaps;
 	int			error = 0;
 
-	if (!(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK))
-		return 0;
+	ASSERT(xfs_is_reflink_inode(ip));
 
 	fbno = 0;
 	end = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip)));
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 72bde28..ade4691 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -947,11 +947,13 @@ xfs_fs_destroy_inode(
 	XFS_STATS_INC(ip->i_mount, vn_rele);
 	XFS_STATS_INC(ip->i_mount, vn_remove);
 
-	error = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF);
-	if (error && !XFS_FORCED_SHUTDOWN(ip->i_mount))
-		xfs_warn(ip->i_mount, "Error %d while evicting CoW blocks "
-				"for inode %llu.",
-				error, ip->i_ino);
+	if (xfs_is_reflink_inode(ip)) {
+		error = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF);
+		if (error && !XFS_FORCED_SHUTDOWN(ip->i_mount))
+			xfs_warn(ip->i_mount,
+"Error %d while evicting CoW blocks for inode %llu.",
+					error, ip->i_ino);
+	}
 
 	xfs_inactive(ip);
 

Re: [PATCH 2/7] xfs: check inode reflink flag before calling reflink functions

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:07:32PM -0700, Darrick J. Wong wrote:
> There are a couple of places where we don't check the inode's
> reflink flag before calling into the reflink code.  Fix those,
> and add some asserts so we don't make this mistake again.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/7] xfs: check inode reflink flag before calling reflink functions

[Christoph Hellwig]

I would prefer to add the xfs_is_reflink_inode check to
xfs_reflink_cancel_cow_range instead dealing with it in the caller.

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/7] xfs: reduce stack usage of _reflink_clear_inode_flag

[Darrick J. Wong]

The loop in _reflink_clear_inode_flag isn't necessary since we
jump out if any part of any extent is shared.  Remove the loop
and we no longer need two maps, so we can save some stack use.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c |   40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 9c58b4a..c4e35dc 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1507,7 +1507,7 @@ xfs_reflink_clear_inode_flag(
 	xfs_extlen_t		aglen;
 	xfs_agblock_t		rbno;
 	xfs_extlen_t		rlen;
-	struct xfs_bmbt_irec	map[2];
+	struct xfs_bmbt_irec	map;
 	int			nmaps;
 	int			error = 0;
 
@@ -1521,37 +1521,29 @@ xfs_reflink_clear_inode_flag(
 		 * Look for extents in the file.  Skip holes, delalloc, or
 		 * unwritten extents; they can't be reflinked.
 		 */
-		error = xfs_bmapi_read(ip, fbno, end - fbno, map, &nmaps, 0);
+		error = xfs_bmapi_read(ip, fbno, end - fbno, &map, &nmaps, 0);
 		if (error)
 			return error;
 		if (nmaps == 0)
 			break;
-		if (map[0].br_startblock == HOLESTARTBLOCK ||
-		    map[0].br_startblock == DELAYSTARTBLOCK ||
-		    ISUNWRITTEN(&map[0]))
+		if (map.br_startblock == HOLESTARTBLOCK ||
+		    map.br_startblock == DELAYSTARTBLOCK ||
+		    ISUNWRITTEN(&map))
 			goto next;
 
-		map[1] = map[0];
-		while (map[1].br_blockcount) {
-			agno = XFS_FSB_TO_AGNO(mp, map[1].br_startblock);
-			agbno = XFS_FSB_TO_AGBNO(mp, map[1].br_startblock);
-			aglen = map[1].br_blockcount;
-
-			error = xfs_reflink_find_shared(mp, agno, agbno, aglen,
-					&rbno, &rlen, false);
-			if (error)
-				return error;
-			/* Is there still a shared block here? */
-			if (rbno != NULLAGBLOCK)
-				return 0;
-
-			map[1].br_blockcount -= aglen;
-			map[1].br_startoff += aglen;
-			map[1].br_startblock += aglen;
-		}
+		agno = XFS_FSB_TO_AGNO(mp, map.br_startblock);
+		agbno = XFS_FSB_TO_AGBNO(mp, map.br_startblock);
+		aglen = map.br_blockcount;
 
+		error = xfs_reflink_find_shared(mp, agno, agbno, aglen,
+				&rbno, &rlen, false);
+		if (error)
+			return error;
+		/* Is there still a shared block here? */
+		if (rbno != NULLAGBLOCK)
+			return 0;
 next:
-		fbno = map[0].br_startoff + map[0].br_blockcount;
+		fbno = map.br_startoff + map.br_blockcount;
 	}
 
 	/*

Re: [PATCH 3/7] xfs: reduce stack usage of _reflink_clear_inode_flag

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:07:38PM -0700, Darrick J. Wong wrote:
> The loop in _reflink_clear_inode_flag isn't necessary since we
> jump out if any part of any extent is shared.  Remove the loop
> and we no longer need two maps, so we can save some stack use.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/7] xfs: reduce stack usage of _reflink_clear_inode_flag

[Christoph Hellwig]

On Fri, Oct 07, 2016 at 04:07:38PM -0700, Darrick J. Wong wrote:
> The loop in _reflink_clear_inode_flag isn't necessary since we
> jump out if any part of any extent is shared.  Remove the loop
> and we no longer need two maps, so we can save some stack use.

I have a patch that gets rid of the xfs_bmapi_read calls entirely
and instead does an initial xfs_bmap_search_extents and then iterates
over the extent index.

It also uses xfs_reflink_trim_around_shared instead of opencoding
it.

It didn't plan to submit it now as it seemed to superficial for 4.9,
I can't dust it off if you want.

[PATCH 4/7] xfs: remove isize check from unshare operation

[Darrick J. Wong]

Now that fallocate has an explicit unshare flag again, let's try
to remove the inode reflink flag whenever the user unshares any
part of a file since checking is cheap compared to the CoW.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_reflink.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index c4e35dc..220d263 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1570,8 +1570,7 @@ xfs_reflink_clear_inode_flag(
  */
 STATIC int
 xfs_reflink_try_clear_inode_flag(
-	struct xfs_inode	*ip,
-	xfs_off_t		old_isize)
+	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
@@ -1585,9 +1584,6 @@ xfs_reflink_try_clear_inode_flag(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	if (old_isize != i_size_read(VFS_I(ip)))
-		goto cancel;
-
 	error = xfs_reflink_clear_inode_flag(ip, &tp);
 	if (error)
 		goto cancel;
@@ -1630,7 +1626,7 @@ xfs_reflink_unshare(
 
 	/* Try to CoW the selected ranges */
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	fbno = XFS_B_TO_FSB(mp, offset);
+	fbno = XFS_B_TO_FSBT(mp, offset);
 	isize = i_size_read(VFS_I(ip));
 	end = XFS_B_TO_FSB(mp, offset + len);
 	error = xfs_reflink_dirty_extents(ip, fbno, end, isize);
@@ -1643,12 +1639,10 @@ xfs_reflink_unshare(
 	if (error)
 		goto out;
 
-	/* Turn off the reflink flag if we unshared the whole file */
-	if (offset == 0 && len == isize) {
-		error = xfs_reflink_try_clear_inode_flag(ip, isize);
-		if (error)
-			goto out;
-	}
+	/* Turn off the reflink flag if possible. */
+	error = xfs_reflink_try_clear_inode_flag(ip);
+	if (error)
+		goto out;
 
 	return 0;
 

Re: [PATCH 4/7] xfs: remove isize check from unshare operation

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:07:44PM -0700, Darrick J. Wong wrote:
> Now that fallocate has an explicit unshare flag again, let's try
> to remove the inode reflink flag whenever the user unshares any
> part of a file since checking is cheap compared to the CoW.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/7] xfs: remove isize check from unshare operation

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/7] xfs: fix label inaccuracies

[Darrick J. Wong]

Since we don't unlock anything on the way out, change the label.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_file.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 54c27ed..710ee11 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1098,19 +1098,19 @@ xfs_file_share_range(
 	/* Wait for the completion of any pending IOs on srcfile */
 	ret = xfs_file_wait_for_io(inode_in, pos_in, len);
 	if (ret)
-		goto out_unlock;
+		goto out;
 	ret = xfs_file_wait_for_io(inode_out, pos_out, len);
 	if (ret)
-		goto out_unlock;
+		goto out;
 
 	if (is_dedupe)
 		flags |= XFS_REFLINK_DEDUPE;
 	ret = xfs_reflink_remap_range(XFS_I(inode_in), pos_in, XFS_I(inode_out),
 			pos_out, len, flags);
 	if (ret < 0)
-		goto out_unlock;
+		goto out;
 
-out_unlock:
+out:
 	return ret;
 }
 

Re: [PATCH 5/7] xfs: fix label inaccuracies

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:07:50PM -0700, Darrick J. Wong wrote:
> Since we don't unlock anything on the way out, change the label.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>


Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/7] xfs: fix label inaccuracies

[Christoph Hellwig]

On Fri, Oct 07, 2016 at 04:07:50PM -0700, Darrick J. Wong wrote:
> Since we don't unlock anything on the way out, change the label.

But we should.. The xfs_file_wait_for_io calls should be under the
I/O lock to be race free.  My idea was to simply merge
xfs_file_share_range into xfs_reflink_remap_range while also cleaning
the bool dedup vs flags mess.  I just haven't gotten to it yet..

[PATCH 6/7] xfs: fix error initialization

[Darrick J. Wong]

Eric Sandeen reported a gcc complaint about uninitialized error
variables, so fix that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_reflink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 220d263..5965e94 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1437,7 +1437,7 @@ xfs_reflink_dirty_extents(
 	xfs_off_t		flen;
 	struct xfs_bmbt_irec	map[2];
 	int			nmaps;
-	int			error;
+	int			error = 0;
 
 	while (end - fbno > 0) {
 		nmaps = 1;

Re: [PATCH 6/7] xfs: fix error initialization

[Eric Sandeen]

On 10/7/16 6:07 PM, Darrick J. Wong wrote:
> Eric Sandeen reported a gcc complaint about uninitialized error
> variables, so fix that.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_reflink.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 220d263..5965e94 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1437,7 +1437,7 @@ xfs_reflink_dirty_extents(
>  	xfs_off_t		flen;
>  	struct xfs_bmbt_irec	map[2];
>  	int			nmaps;
> -	int			error;
> +	int			error = 0;
>  
>  	while (end - fbno > 0) {

If we should never get here w/ end <= fbno, then maybe an ASSERT is
in order.

And if we ever do get here w/ that case, perhaps default error to a relevant
error so we don't return success and carry on in that case?

-Eric

>  		nmaps = 1;
> 
> --
> 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
> 

Re: [PATCH 6/7] xfs: fix error initialization

[Dave Chinner]

On Sat, Oct 08, 2016 at 10:55:58AM -0500, Eric Sandeen wrote:
> On 10/7/16 6:07 PM, Darrick J. Wong wrote:
> > Eric Sandeen reported a gcc complaint about uninitialized error
> > variables, so fix that.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reported-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> >  fs/xfs/xfs_reflink.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 220d263..5965e94 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1437,7 +1437,7 @@ xfs_reflink_dirty_extents(
> >  	xfs_off_t		flen;
> >  	struct xfs_bmbt_irec	map[2];
> >  	int			nmaps;
> > -	int			error;
> > +	int			error = 0;
> >  
> >  	while (end - fbno > 0) {
> 
> If we should never get here w/ end <= fbno, then maybe an ASSERT is
> in order.

That should be the case - the fallocate infrastructure should be
ensuring len > 0 and we don't overflow. Followup patch, if you think
it necessary, Darrick?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 6/7] xfs: fix error initialization

[Christoph Hellwig]

On Sat, Oct 08, 2016 at 10:55:58AM -0500, Eric Sandeen wrote:
> On 10/7/16 6:07 PM, Darrick J. Wong wrote:
> > Eric Sandeen reported a gcc complaint about uninitialized error
> > variables, so fix that.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reported-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> >  fs/xfs/xfs_reflink.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 220d263..5965e94 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1437,7 +1437,7 @@ xfs_reflink_dirty_extents(
> >  	xfs_off_t		flen;
> >  	struct xfs_bmbt_irec	map[2];
> >  	int			nmaps;
> > -	int			error;
> > +	int			error = 0;
> >  
> >  	while (end - fbno > 0) {
> 
> If we should never get here w/ end <= fbno, then maybe an ASSERT is
> in order.
> 
> And if we ever do get here w/ that case, perhaps default error to a relevant
> error so we don't return success and carry on in that case?

Initializing it to -EINVAL plus an assert for debug builds sounds fine.

[PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Darrick J. Wong]

Since we can only turn on the rt flag if there are no data extents,
we can safely turn off the reflink flag if the rt flag is being
turned on.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reported-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_ioctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8b9f31c5..598b97b 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1034,9 +1034,9 @@ xfs_ioctl_setattr_xflags(
 			return -EINVAL;
 	}
 
-	/* Don't allow us to set realtime mode for a reflinked file. */
+	/* Clear reflink if we are actually able to set the rt flag. */
 	if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
-		return -EINVAL;
+		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
 
 	/* Don't allow us to set DAX mode for a reflinked file for now. */
 	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Dave Chinner]

On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> Since we can only turn on the rt flag if there are no data extents,
> we can safely turn off the reflink flag if the rt flag is being
> turned on.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Christoph Hellwig]

On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> Since we can only turn on the rt flag if there are no data extents,
> we can safely turn off the reflink flag if the rt flag is being
> turned on.

Why would the reflink flag ever be turned on if we have no data extents?

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Dave Chinner]

On Mon, Oct 10, 2016 at 03:24:18AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> > Since we can only turn on the rt flag if there are no data extents,
> > we can safely turn off the reflink flag if the rt flag is being
> > turned on.
> 
> Why would the reflink flag ever be turned on if we have no data extents?

After a truncate?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Christoph Hellwig]

On Mon, Oct 10, 2016 at 11:25:08PM +1100, Dave Chinner wrote:
> On Mon, Oct 10, 2016 at 03:24:18AM -0700, Christoph Hellwig wrote:
> > On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> > > Since we can only turn on the rt flag if there are no data extents,
> > > we can safely turn off the reflink flag if the rt flag is being
> > > turned on.
> > 
> > Why would the reflink flag ever be turned on if we have no data extents?
> 
> After a truncate?

Any why don't we simply clear the flag when we know there can't be
any shared extents left?

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Dave Chinner]

On Mon, Oct 10, 2016 at 05:45:00AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 10, 2016 at 11:25:08PM +1100, Dave Chinner wrote:
> > On Mon, Oct 10, 2016 at 03:24:18AM -0700, Christoph Hellwig wrote:
> > > On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> > > > Since we can only turn on the rt flag if there are no data extents,
> > > > we can safely turn off the reflink flag if the rt flag is being
> > > > turned on.
> > > 
> > > Why would the reflink flag ever be turned on if we have no data extents?
> > 
> > After a truncate?
> 
> Any why don't we simply clear the flag when we know there can't be
> any shared extents left?

Sure, go ahead. I'll queue it up for the next cycle....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Darrick J. Wong]

On Mon, Oct 10, 2016 at 05:45:00AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 10, 2016 at 11:25:08PM +1100, Dave Chinner wrote:
> > On Mon, Oct 10, 2016 at 03:24:18AM -0700, Christoph Hellwig wrote:
> > > On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> > > > Since we can only turn on the rt flag if there are no data extents,
> > > > we can safely turn off the reflink flag if the rt flag is being
> > > > turned on.
> > > 
> > > Why would the reflink flag ever be turned on if we have no data extents?
> > 
> > After a truncate?
> 
> Any why don't we simply clear the flag when we know there can't be
> any shared extents left?

We /do/ clear the flag if we truncate everything out of the file.  See
the end of xfs_itruncate_extents().

--D

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Brian Foster]

On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> Since we can only turn on the rt flag if there are no data extents,
> we can safely turn off the reflink flag if the rt flag is being
> turned on.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reported-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 8b9f31c5..598b97b 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1034,9 +1034,9 @@ xfs_ioctl_setattr_xflags(
>  			return -EINVAL;
>  	}
>  
> -	/* Don't allow us to set realtime mode for a reflinked file. */
> +	/* Clear reflink if we are actually able to set the rt flag. */
>  	if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
> -		return -EINVAL;
> +		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
>  
>  	/* Don't allow us to set DAX mode for a reflinked file for now. */
>  	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))

This seems fine to me, but don't we still have the original problem in
the code that shortly follows with regard to the DAX flag? In other
words, the fundamental issue was that we clear the reflink flag lazily
(which by itself is perfectly fine), but we have a few spots where we
trust that the inode absolutely has shared extents when the flag is set.

That also might be fine logic in certain contexts, but in these couple
cases we fail requested operations from the user (setting RT, DAX,
etc.). It's not a big deal in that I suppose one can always run an
unshare fallocate, but just could be confusing if somebody is aware of
the already unshared state of a file.

Could we invoke xfs_reflink_clear_inode_flag() somewhere earlier in this
path if the reflink flag is set and any of the conflicting flags have
been requested by the user?

Brian

> 
> --
> 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

Re: [PATCH 7/7] xfs: clear reflink flag if setting realtime flag

[Darrick J. Wong]

On Mon, Oct 10, 2016 at 08:44:39AM -0400, Brian Foster wrote:
> On Fri, Oct 07, 2016 at 04:08:03PM -0700, Darrick J. Wong wrote:
> > Since we can only turn on the rt flag if there are no data extents,
> > we can safely turn off the reflink flag if the rt flag is being
> > turned on.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reported-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_ioctl.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 8b9f31c5..598b97b 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1034,9 +1034,9 @@ xfs_ioctl_setattr_xflags(
> >  			return -EINVAL;
> >  	}
> >  
> > -	/* Don't allow us to set realtime mode for a reflinked file. */
> > +	/* Clear reflink if we are actually able to set the rt flag. */
> >  	if ((fa->fsx_xflags & FS_XFLAG_REALTIME) && xfs_is_reflink_inode(ip))
> > -		return -EINVAL;
> > +		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> >  
> >  	/* Don't allow us to set DAX mode for a reflinked file for now. */
> >  	if ((fa->fsx_xflags & FS_XFLAG_DAX) && xfs_is_reflink_inode(ip))
> 
> This seems fine to me, but don't we still have the original problem in
> the code that shortly follows with regard to the DAX flag? In other
> words, the fundamental issue was that we clear the reflink flag lazily
> (which by itself is perfectly fine), but we have a few spots where we
> trust that the inode absolutely has shared extents when the flag is set.
> 
> That also might be fine logic in certain contexts, but in these couple
> cases we fail requested operations from the user (setting RT, DAX,
> etc.). It's not a big deal in that I suppose one can always run an
> unshare fallocate, but just could be confusing if somebody is aware of
> the already unshared state of a file.
> 
> Could we invoke xfs_reflink_clear_inode_flag() somewhere earlier in this
> path if the reflink flag is set and any of the conflicting flags have
> been requested by the user?

Seems reasonable.  I'll work on a patch.

--D

> 
> Brian
> 
> > 
> > --
> > 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