[PATCH 2/2] xfs: don't unconditionally clear the reflink flag on zero-block files

[PATCH 2/2] xfs: don't unconditionally clear the reflink flag on zero-block files

[Darrick J. Wong]

If we have speculative cow preallocations hanging around in the cow
fork, don't let a truncate operation clear the reflink flag because if
we do then there's a chance we'll forget to free those extents when we
destroy the incore inode.

Reported-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_inode.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5599dda..4ec5b7f 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1624,10 +1624,12 @@ xfs_itruncate_extents(
 		goto out;
 
 	/*
-	 * Clear the reflink flag if we truncated everything.
+	 * Clear the reflink flag if there are no data fork blocks and
+	 * there are no extents staged in the cow fork.
 	 */
-	if (ip->i_d.di_nblocks == 0 && xfs_is_reflink_inode(ip)) {
-		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
+	if (xfs_is_reflink_inode(ip) && ip->i_cnextents == 0) {
+		if (ip->i_d.di_nblocks == 0)
+			ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
 		xfs_inode_clear_cowblocks_tag(ip);
 	}
 

Re: [PATCH 2/2] xfs: don't unconditionally clear the reflink flag on zero-block files

[Carlos Maiolino]

On Mon, Sep 04, 2017 at 08:49:44AM -0700, Darrick J. Wong wrote:
> If we have speculative cow preallocations hanging around in the cow
> fork, don't let a truncate operation clear the reflink flag because if
> we do then there's a chance we'll forget to free those extents when we
> destroy the incore inode.
> 
> Reported-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> ---
>  fs/xfs/xfs_inode.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5599dda..4ec5b7f 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1624,10 +1624,12 @@ xfs_itruncate_extents(
>  		goto out;
>  
>  	/*
> -	 * Clear the reflink flag if we truncated everything.
> +	 * Clear the reflink flag if there are no data fork blocks and
> +	 * there are no extents staged in the cow fork.
>  	 */
> -	if (ip->i_d.di_nblocks == 0 && xfs_is_reflink_inode(ip)) {
> -		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> +	if (xfs_is_reflink_inode(ip) && ip->i_cnextents == 0) {
> +		if (ip->i_d.di_nblocks == 0)
> +			ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
>  		xfs_inode_clear_cowblocks_tag(ip);
>  	}
>  
> --
> 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

-- 
Carlos

Re: [PATCH 2/2] xfs: don't unconditionally clear the reflink flag on zero-block files

[Christoph Hellwig]

On Mon, Sep 04, 2017 at 08:49:44AM -0700, Darrick J. Wong wrote:
> If we have speculative cow preallocations hanging around in the cow
> fork, don't let a truncate operation clear the reflink flag because if
> we do then there's a chance we'll forget to free those extents when we
> destroy the incore inode.
> 
> Reported-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5599dda..4ec5b7f 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1624,10 +1624,12 @@ xfs_itruncate_extents(
>  		goto out;
>  
>  	/*
> -	 * Clear the reflink flag if we truncated everything.
> +	 * Clear the reflink flag if there are no data fork blocks and
> +	 * there are no extents staged in the cow fork.
>  	 */
> -	if (ip->i_d.di_nblocks == 0 && xfs_is_reflink_inode(ip)) {
> -		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> +	if (xfs_is_reflink_inode(ip) && ip->i_cnextents == 0) {
> +		if (ip->i_d.di_nblocks == 0)
> +			ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
>  		xfs_inode_clear_cowblocks_tag(ip);

This part looks ok:

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

But while looking at that I decided I'd want to take a look at
other places where we potentially clear XFS_DIFLAG2_REFLINK.

In xfs_ioctl_setattr_xflags it seems like it's not currently safe
for setting the RT flag vs having COW extents?  I also think we'd
need the iolock for every change to the rt flag, so this probably
needs a bigger rework..

swapext also doesn't seem to flush out existing cow extents.

Re: [PATCH 2/2] xfs: don't unconditionally clear the reflink flag on zero-block files

[Darrick J. Wong]

On Mon, Sep 04, 2017 at 12:33:52PM -0700, Christoph Hellwig wrote:
> On Mon, Sep 04, 2017 at 08:49:44AM -0700, Darrick J. Wong wrote:
> > If we have speculative cow preallocations hanging around in the cow
> > fork, don't let a truncate operation clear the reflink flag because if
> > we do then there's a chance we'll forget to free those extents when we
> > destroy the incore inode.
> > 
> > Reported-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 5599dda..4ec5b7f 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1624,10 +1624,12 @@ xfs_itruncate_extents(
> >  		goto out;
> >  
> >  	/*
> > -	 * Clear the reflink flag if we truncated everything.
> > +	 * Clear the reflink flag if there are no data fork blocks and
> > +	 * there are no extents staged in the cow fork.
> >  	 */
> > -	if (ip->i_d.di_nblocks == 0 && xfs_is_reflink_inode(ip)) {
> > -		ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> > +	if (xfs_is_reflink_inode(ip) && ip->i_cnextents == 0) {
> > +		if (ip->i_d.di_nblocks == 0)
> > +			ip->i_d.di_flags2 &= ~XFS_DIFLAG2_REFLINK;
> >  		xfs_inode_clear_cowblocks_tag(ip);
> 
> This part looks ok:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But while looking at that I decided I'd want to take a look at
> other places where we potentially clear XFS_DIFLAG2_REFLINK.
> 
> In xfs_ioctl_setattr_xflags it seems like it's not currently safe
> for setting the RT flag vs having COW extents?

Yeah... if we set the rt flag (which for now implies clearing reflink)
we also have to kill everything in the cow fork.

> I also think we'd need the iolock for every change to the rt flag, so
> this probably needs a bigger rework..

<eep>

> swapext also doesn't seem to flush out existing cow extents.

Yes, it swaps the cow forks too... except that there's a bug where we
only swap them if the reflink flags mismatch on ip/tip, and don't swap
i_cnextents at all, ever.  Ugggh.

--D

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