[PATCH 0/2] xfs: fix up some reflink+dax interactions

[PATCH 0/2] xfs: fix up some reflink+dax interactions

[Eric Sandeen]

dax behavior has changed semi-recently, most notably that per-inode dax
flags are back, which opens the possibility of dax-capable files existing on
reflink-capable filesystems.

While we still have a reflink-vs-dax-on-the-same-file incompatibilty, and for
the most part this is handled correctly, there are a couple of known issues:

1) xfs_dinode_verify will trap an inode with reflink+dax flags as corrupted;
   this needs to be removed, because we actually can get into this state today,
   and eventually that state will be supported in future kernels.

2) (more RFC) until we actually support reflink+dax files, perhaps we should
   prevent the flags from co-existing in a kernel that cannot support both
   states.  patch 2 stops us from reflinking files with the dax flag set,
   whether or not the file is actually "in the CPU direct access state"

-Eric

[PATCH 1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

[Eric Sandeen]

We don't yet support dax on reflinked files, but that is in the works.

Further, having the flag set does not automatically mean that the inode
is actually "in the CPU direct access state," which depends on several
other conditions in addition to the flag being set.

As such, we should not catch this as corruption in the verifier - simply
not actually enabling S_DAX on reflinked files is enough for now.

Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index c667c63f2cb0..4d7410e49db4 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -547,10 +547,6 @@ xfs_dinode_verify(
 	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
 		return __this_address;
 
-	/* don't let reflink and dax mix */
-	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
-		return __this_address;
-
 	/* COW extent size hint validation */
 	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
 			mode, flags, flags2);
-- 
2.17.0

[RFC PATCH 2/2] xfs: do not allow reflinking inodes with the dax flag set

[Eric Sandeen]

Today, xfs_reflink_remap_prep() will reject inodes which are in the CPU
direct access state, i.e. IS_DAX() is true.  However, it is possible to
have inodes with the XFS_DIFLAG2_DAX set, but which are not activated as
dax, due to the dax=never mount option, or due to the flag being set after
the inode was loaded.

To avoid confusion and make the lack of dax+reflink crystal clear for the
user, reject reflink requests for both IS_DAX and XFS_DIFLAG2_DAX inodes.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

This is RFC because as Darrick says, it introduces a new failure mode for
reflink. On the flip side, today the user can reflink a chattr +x'd file,
but cannot chattr +x a reflinked file, which seems a best a bit asymmetrical
and confusing... see xfs_ioctl_setattr_xflags()

 fs/xfs/xfs_reflink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 6fa05fb78189..b69dbb992b0c 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1308,6 +1308,11 @@ xfs_reflink_remap_prep(
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
 		goto out_unlock;
 
+	/* Until we have dax+reflink don't even allow the flags to co-exist */
+	if (src->i_d.di_flags2 & XFS_DIFLAG2_DAX ||
+	    dest->i_d.di_flags2 & XFS_DIFLAG2_DAX)
+		goto out_unlock;
+
 	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
 			len, remap_flags);
 	if (ret || *len == 0)
-- 
2.17.0

Re: [PATCH 0/2] xfs: fix up some reflink+dax interactions

[Eric Sandeen]

On 12/1/20 1:10 PM, Eric Sandeen wrote:
> dax behavior has changed semi-recently, most notably that per-inode dax
> flags are back, which opens the possibility of dax-capable files existing on
> reflink-capable filesystems.
> 
> While we still have a reflink-vs-dax-on-the-same-file incompatibilty, and for
> the most part this is handled correctly, there are a couple of known issues:
> 
> 1) xfs_dinode_verify will trap an inode with reflink+dax flags as corrupted;
>    this needs to be removed, because we actually can get into this state today,
>    and eventually that state will be supported in future kernels.
> 
> 2) (more RFC) until we actually support reflink+dax files, perhaps we should
>    prevent the flags from co-existing in a kernel that cannot support both
>    states.  patch 2 stops us from reflinking files with the dax flag set,
>    whether or not the file is actually "in the CPU direct access state"
> 
> -Eric

Also yes I owe xfstests for these but wanted to see if the patches fly, first.

-Eric

Re: [PATCH 1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

[Christoph Hellwig]

On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> We don't yet support dax on reflinked files, but that is in the works.
> 
> Further, having the flag set does not automatically mean that the inode
> is actually "in the CPU direct access state," which depends on several
> other conditions in addition to the flag being set.
> 
> As such, we should not catch this as corruption in the verifier - simply
> not actually enabling S_DAX on reflinked files is enough for now.

Looks good,

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

Re: [RFC PATCH 2/2] xfs: do not allow reflinking inodes with the dax flag set

[Christoph Hellwig]

On Tue, Dec 01, 2020 at 01:20:55PM -0600, Eric Sandeen wrote:
> Today, xfs_reflink_remap_prep() will reject inodes which are in the CPU
> direct access state, i.e. IS_DAX() is true.  However, it is possible to
> have inodes with the XFS_DIFLAG2_DAX set, but which are not activated as
> dax, due to the dax=never mount option, or due to the flag being set after
> the inode was loaded.
> 
> To avoid confusion and make the lack of dax+reflink crystal clear for the
> user, reject reflink requests for both IS_DAX and XFS_DIFLAG2_DAX inodes.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> This is RFC because as Darrick says, it introduces a new failure mode for
> reflink. On the flip side, today the user can reflink a chattr +x'd file,
> but cannot chattr +x a reflinked file, which seems a best a bit asymmetrical
> and confusing... see xfs_ioctl_setattr_xflags()

This seems confusing.  IMHO for now we should just for non-dax access
to any reflink file even if XFS_DIFLAG2_DAX is set.  The only place
where we cannot do that is if a file has XFS_DIFLAG2_DAX set and is in
use and we want to reflink it.  Note that "in use" is kinda murky and
potentially racy.  So IMHO not allowing reflink when XFS_DIFLAG2_DAX
is set and dax=never is not set makes sense, but we should not go
further.

Re: [RFC PATCH 2/2] xfs: do not allow reflinking inodes with the dax flag set

[Eric Sandeen]

On 12/2/20 4:22 AM, Christoph Hellwig wrote:
> On Tue, Dec 01, 2020 at 01:20:55PM -0600, Eric Sandeen wrote:
>> Today, xfs_reflink_remap_prep() will reject inodes which are in the CPU
>> direct access state, i.e. IS_DAX() is true.  However, it is possible to
>> have inodes with the XFS_DIFLAG2_DAX set, but which are not activated as
>> dax, due to the dax=never mount option, or due to the flag being set after
>> the inode was loaded.
>>
>> To avoid confusion and make the lack of dax+reflink crystal clear for the
>> user, reject reflink requests for both IS_DAX and XFS_DIFLAG2_DAX inodes.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> This is RFC because as Darrick says, it introduces a new failure mode for
>> reflink. On the flip side, today the user can reflink a chattr +x'd file,
>> but cannot chattr +x a reflinked file, which seems a best a bit asymmetrical
>> and confusing... see xfs_ioctl_setattr_xflags()
> 
> This seems confusing.  IMHO for now we should just for non-dax access
> to any reflink file even if XFS_DIFLAG2_DAX is set.  The only place
> where we cannot do that is if a file has XFS_DIFLAG2_DAX set and is in
> use and we want to reflink it.  Note that "in use" is kinda murky and
> potentially racy.  So IMHO not allowing reflink when XFS_DIFLAG2_DAX
> is set and dax=never is not set makes sense, but we should not go
> further.

Hm, trying to parse that...

Would it be correct to restate your last sentence as "Disallowing reflink
when XFS_DIFLAG2_DAX is set and dax=inode is set makes sense?"

If so, then the only change you're suggesting to this patch is to /allow/
reflinking if dax=never is set?

I just figured a very clear statementa bout incompatible flags was simplest,
but I get it that it's overly restrictive, functionally.

Thanks,
-Eric

Re: [RFC PATCH 2/2] xfs: do not allow reflinking inodes with the dax flag set

[Christoph Hellwig]

On Wed, Dec 02, 2020 at 08:44:24AM -0600, Eric Sandeen wrote:
> Would it be correct to restate your last sentence as "Disallowing reflink
> when XFS_DIFLAG2_DAX is set and dax=inode is set makes sense?"
> 
> If so, then the only change you're suggesting to this patch is to /allow/
> reflinking if dax=never is set?

Yes, I think we should.

> I just figured a very clear statementa bout incompatible flags was simplest,
> but I get it that it's overly restrictive, functionally.

The simplest in terms of semantics is to make sure reflink+DAX works,
and while we are on the way we'll still need a workaround until that
happen.

Re: [RFC PATCH 2/2] xfs: do not allow reflinking inodes with the dax flag set

[Christoph Hellwig]

On Wed, Dec 02, 2020 at 05:15:30PM +0000, Christoph Hellwig wrote:
> On Wed, Dec 02, 2020 at 08:44:24AM -0600, Eric Sandeen wrote:
> > Would it be correct to restate your last sentence as "Disallowing reflink
> > when XFS_DIFLAG2_DAX is set and dax=inode is set makes sense?"
> > 
> > If so, then the only change you're suggesting to this patch is to /allow/
> > reflinking if dax=never is set?
> 
> Yes, I think we should.
> 
> > I just figured a very clear statementa bout incompatible flags was simplest,
> > but I get it that it's overly restrictive, functionally.
> 
> The simplest in terms of semantics is to make sure reflink+DAX works,
> and while we are on the way we'll still need a workaround until that
> happen.
happens.

Re: [PATCH 1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

[Darrick J. Wong]

On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> We don't yet support dax on reflinked files, but that is in the works.
> 
> Further, having the flag set does not automatically mean that the inode
> is actually "in the CPU direct access state," which depends on several
> other conditions in addition to the flag being set.
> 
> As such, we should not catch this as corruption in the verifier - simply
> not actually enabling S_DAX on reflinked files is enough for now.
> 
> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index c667c63f2cb0..4d7410e49db4 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -547,10 +547,6 @@ xfs_dinode_verify(
>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
>  		return __this_address;
>  
> -	/* don't let reflink and dax mix */
> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
> -		return __this_address;

If we're going to let in inodes with the DAX and REFLINK iflags set,
doesn't that mean that xfs_inode_should_enable_dax needs to return false
if REFLINK is set?

--D

> -
>  	/* COW extent size hint validation */
>  	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
>  			mode, flags, flags2);
> -- 
> 2.17.0
> 

Re: [PATCH 1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

[Eric Sandeen]

On 12/3/20 3:44 PM, Darrick J. Wong wrote:
> On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
>> We don't yet support dax on reflinked files, but that is in the works.
>>
>> Further, having the flag set does not automatically mean that the inode
>> is actually "in the CPU direct access state," which depends on several
>> other conditions in addition to the flag being set.
>>
>> As such, we should not catch this as corruption in the verifier - simply
>> not actually enabling S_DAX on reflinked files is enough for now.
>>
>> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index c667c63f2cb0..4d7410e49db4 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -547,10 +547,6 @@ xfs_dinode_verify(
>>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
>>  		return __this_address;
>>  
>> -	/* don't let reflink and dax mix */
>> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
>> -		return __this_address;
> 
> If we're going to let in inodes with the DAX and REFLINK iflags set,
> doesn't that mean that xfs_inode_should_enable_dax needs to return false
> if REFLINK is set?

I think it does already, no?

static bool
xfs_inode_should_enable_dax(
        struct xfs_inode *ip)
{
        if (!IS_ENABLED(CONFIG_FS_DAX))
                return false;
        if (ip->i_mount->m_flags & XFS_MOUNT_DAX_NEVER)
                return false;
        if (!xfs_inode_supports_dax(ip)) <------
                return false;


----> xfs_inode_supports_dax ---> 

static bool
xfs_inode_supports_dax(
        struct xfs_inode        *ip)
{
        struct xfs_mount        *mp = ip->i_mount;

        /* Only supported on regular files. */
        if (!S_ISREG(VFS_I(ip)->i_mode))
                return false;

        /* Only supported on non-reflinked files. */
        if (xfs_is_reflink_inode(ip))
                return false;

Re: [PATCH 1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

[Darrick J. Wong]

On Thu, Dec 03, 2020 at 04:19:35PM -0600, Eric Sandeen wrote:
> On 12/3/20 3:44 PM, Darrick J. Wong wrote:
> > On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> >> We don't yet support dax on reflinked files, but that is in the works.
> >>
> >> Further, having the flag set does not automatically mean that the inode
> >> is actually "in the CPU direct access state," which depends on several
> >> other conditions in addition to the flag being set.
> >>
> >> As such, we should not catch this as corruption in the verifier - simply
> >> not actually enabling S_DAX on reflinked files is enough for now.
> >>
> >> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> >> index c667c63f2cb0..4d7410e49db4 100644
> >> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> >> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> >> @@ -547,10 +547,6 @@ xfs_dinode_verify(
> >>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
> >>  		return __this_address;
> >>  
> >> -	/* don't let reflink and dax mix */
> >> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
> >> -		return __this_address;
> > 
> > If we're going to let in inodes with the DAX and REFLINK iflags set,
> > doesn't that mean that xfs_inode_should_enable_dax needs to return false
> > if REFLINK is set?
> 
> I think it does already, no?
> 
> static bool
> xfs_inode_should_enable_dax(
>         struct xfs_inode *ip)
> {
>         if (!IS_ENABLED(CONFIG_FS_DAX))
>                 return false;
>         if (ip->i_mount->m_flags & XFS_MOUNT_DAX_NEVER)
>                 return false;
>         if (!xfs_inode_supports_dax(ip)) <------
>                 return false;
> 
> 
> ----> xfs_inode_supports_dax ---> 
> 
> static bool
> xfs_inode_supports_dax(
>         struct xfs_inode        *ip)
> {
>         struct xfs_mount        *mp = ip->i_mount;
> 
>         /* Only supported on regular files. */
>         if (!S_ISREG(VFS_I(ip)->i_mode))
>                 return false;
> 
>         /* Only supported on non-reflinked files. */
>         if (xfs_is_reflink_inode(ip))
>                 return false;
> 

DOH  yes it does,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D