[PATCH V2] xfs: do not allow reflinking inodes with the dax flag set

[PATCH V2] 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 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
unless DAX mode is impossible due to mounting with -o dax=never.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: Allow reflinking dax-flagged inodes in "mount -o dax=never" mode

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 6fa05fb78189..e238a5b7b722 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1308,6 +1308,15 @@ xfs_reflink_remap_prep(
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
 		goto out_unlock;
 
+	/*
+	 * Until we have dax+reflink, don't allow reflinking dax-flagged
+	 * inodes unless we are in dax=never mode.
+	 */
+	if (!(mp->m_flags & XFS_MOUNT_DAX_NEVER) &&
+	     (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)

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

[Darrick J. Wong]

On Thu, Jan 07, 2021 at 03:36:34PM -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 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
> unless DAX mode is impossible due to mounting with -o dax=never.

I thought we were allowing arbitrary combinations of DAX & REFLINK inode
flags now, since we're now officially ok with "you set the inode flag
but you don't get cpu direct access because $reasons"?

--D

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> V2: Allow reflinking dax-flagged inodes in "mount -o dax=never" mode
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 6fa05fb78189..e238a5b7b722 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1308,6 +1308,15 @@ xfs_reflink_remap_prep(
>  	if (IS_DAX(inode_in) || IS_DAX(inode_out))
>  		goto out_unlock;
>  
> +	/*
> +	 * Until we have dax+reflink, don't allow reflinking dax-flagged
> +	 * inodes unless we are in dax=never mode.
> +	 */
> +	if (!(mp->m_flags & XFS_MOUNT_DAX_NEVER) &&
> +	     (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)
> 

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

[Eric Sandeen]

On 1/7/21 7:29 PM, Darrick J. Wong wrote:
> On Thu, Jan 07, 2021 at 03:36:34PM -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 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
>> unless DAX mode is impossible due to mounting with -o dax=never.
> 
> I thought we were allowing arbitrary combinations of DAX & REFLINK inode
> flags now, since we're now officially ok with "you set the inode flag
> but you don't get cpu direct access because $reasons"?

*shrug* I think "haha depending on the order and the state we may or may
not let you reflink files with the dax flag set on disk so good luck" is
pretty confusing, and I figured this made things more obvious.

I thought that should be an absolute, hch thought it should be ignored
for dax=never, and now ... ?

I think the the current behavior is a bad user experience violating=
principle of least surprise, but I guess we don't have agreement on that.

-Eric

> --D
> 
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>> V2: Allow reflinking dax-flagged inodes in "mount -o dax=never" mode
>>
>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>> index 6fa05fb78189..e238a5b7b722 100644
>> --- a/fs/xfs/xfs_reflink.c
>> +++ b/fs/xfs/xfs_reflink.c
>> @@ -1308,6 +1308,15 @@ xfs_reflink_remap_prep(
>>  	if (IS_DAX(inode_in) || IS_DAX(inode_out))
>>  		goto out_unlock;
>>  
>> +	/*
>> +	 * Until we have dax+reflink, don't allow reflinking dax-flagged
>> +	 * inodes unless we are in dax=never mode.
>> +	 */
>> +	if (!(mp->m_flags & XFS_MOUNT_DAX_NEVER) &&
>> +	     (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)
>>
> 

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

[Darrick J. Wong]

On Thu, Jan 07, 2021 at 08:54:29PM -0600, Eric Sandeen wrote:
> On 1/7/21 7:29 PM, Darrick J. Wong wrote:
> > On Thu, Jan 07, 2021 at 03:36:34PM -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 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
> >> unless DAX mode is impossible due to mounting with -o dax=never.
> > 
> > I thought we were allowing arbitrary combinations of DAX & REFLINK inode
> > flags now, since we're now officially ok with "you set the inode flag
> > but you don't get cpu direct access because $reasons"?
> 
> *shrug* I think "haha depending on the order and the state we may or may
> not let you reflink files with the dax flag set on disk so good luck" is
> pretty confusing, and I figured this made things more obvious.
> 
> I thought that should be an absolute, hch thought it should be ignored
> for dax=never, and now ... ?
> 
> I think the the current behavior is a bad user experience violating=
> principle of least surprise, but I guess we don't have agreement on that.

I guess not...? :(

In /me's head, S_DAX is a best-effort affair -- if the storage supports
it, and the cpu supports it, and the fs supports it, and the sysadmin
didn't forbid it, and there's no file state preventing it, *then* you
actually get S_DAX.  DIFLAG2_DAX is merely advisory, so it's perfectly
valid for reflink to come along and add a file state that (on this
kernel) prevents DIFLAG2_DAX from causing S_DAX to get set.

(We've probably already gone around and around on this elsewhere, but
I'm catching up on 6 weeks of email...)

> -Eric
> 
> > --D
> > 
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >> V2: Allow reflinking dax-flagged inodes in "mount -o dax=never" mode
> >>
> >> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> >> index 6fa05fb78189..e238a5b7b722 100644
> >> --- a/fs/xfs/xfs_reflink.c
> >> +++ b/fs/xfs/xfs_reflink.c
> >> @@ -1308,6 +1308,15 @@ xfs_reflink_remap_prep(
> >>  	if (IS_DAX(inode_in) || IS_DAX(inode_out))
> >>  		goto out_unlock;
> >>  
> >> +	/*
> >> +	 * Until we have dax+reflink, don't allow reflinking dax-flagged
> >> +	 * inodes unless we are in dax=never mode.
> >> +	 */
> >> +	if (!(mp->m_flags & XFS_MOUNT_DAX_NEVER) &&
> >> +	     (src->i_d.di_flags2 & XFS_DIFLAG2_DAX ||
> >> +	      dest->i_d.di_flags2 & XFS_DIFLAG2_DAX))

I think the bitflag tests need parentheses around them, right?

--D

> >> +		goto out_unlock;
> >> +
> >>  	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
> >>  			len, remap_flags);
> >>  	if (ret || *len == 0)
> >>
> >