| Message ID | 147588168294.12127.1609759616316145097.stgit@birch.djwong.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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.
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
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 --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))
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