diff mbox

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

Message ID 147588168294.12127.1609759616316145097.stgit@birch.djwong.org
State Accepted
Headers show

Commit Message

Darrick J. Wong Oct. 7, 2016, 11:08 p.m. UTC
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(-)



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

Comments

Brian Foster Oct. 10, 2016, 12:44 p.m. UTC | #1
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
--
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
Christoph Hellwig Oct. 10, 2016, 12:45 p.m. UTC | #2
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?
--
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
Dave Chinner Oct. 10, 2016, 12:59 p.m. UTC | #3
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.
Darrick J. Wong Oct. 10, 2016, 5:18 p.m. UTC | #4
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
--
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
Darrick J. Wong Oct. 10, 2016, 6:10 p.m. UTC | #5
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
--
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
diff mbox

Patch

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