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

[Eric Sandeen <sandeen@sandeen.net>]

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