Re: [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems

[Eric Sandeen <sandeen@redhat.com>]

On 12/1/20 11:39 AM, Darrick J. Wong wrote:
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index 1414ab79eacf..56deda7042fd 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -575,10 +575,13 @@ xfs_vn_getattr(
>>  		stat->attributes |= STATX_ATTR_APPEND;
>>  	if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
>>  		stat->attributes |= STATX_ATTR_NODUMP;
>> +	if (IS_DAX(inode))
>> +		stat->attributes |= STATX_ATTR_DAX;
>>  
>>  	stat->attributes_mask |= (STATX_ATTR_IMMUTABLE |
>>  				  STATX_ATTR_APPEND |
>> -				  STATX_ATTR_NODUMP);
>> +				  STATX_ATTR_NODUMP |
>> +				  STATX_ATTR_DAX);
> TBH I preferred your previous iteration on this, which only set the DAX
> bit in the attributes_mask if the underlying storage was pmem and the
> blocksize was correct, etc. since it made it easier to distinguish
> between a filesystem where you /could/ have DAX (but it wasn't currently
> enabled) and a filesystem where it just isn't possible.
> 
> That might be enough to satisfy any critics who want to be able to
> detect DAX support from an installer program.

(nb: that previous iteration wasn't in public, just something I talked to
Darrick about)

I'm sympathetic to that argument, but the exact details of what the mask means
in general, and for dax in particular, seems to be subject to ongoing debate.

I'd like to just set it with the simplest definition "the fileystem supports
the feature" for now, so that we aren't ever setting a feature that's omitted
from the mask, and refine the mask-setting for the dax flag in another
iteration if/when we reach agreement.

-Eric

> --D
>