Re: [PATCH 1/2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Eric Sandeen <sandeen@redhat.com>]

On 12/1/20 12:31 PM, Andreas Dilger wrote:
> On Dec 1, 2020, at 10:44 AM, Eric Sandeen <sandeen@redhat.com> wrote:
>>
>> On 12/1/20 11:32 AM, Darrick J. Wong wrote:
>>> On Tue, Dec 01, 2020 at 10:57:11AM -0600, Eric Sandeen wrote:
>>>> STATX_ATTR_MOUNT_ROOT and STATX_ATTR_DAX got merged with the same value,
>>>> so one of them needs fixing. Move STATX_ATTR_DAX.
>>>>
>>>> While we're in here, clarify the value-matching scheme for some of the
>>>> attributes, and explain why the value for DAX does not match.
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>> include/uapi/linux/stat.h | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
>>>> index 82cc58fe9368..9ad19eb9bbbf 100644
>>>> --- a/include/uapi/linux/stat.h
>>>> +++ b/include/uapi/linux/stat.h
>>>> @@ -171,9 +171,10 @@ struct statx {
>>>>  * be of use to ordinary userspace programs such as GUIs or ls rather than
>>>>  * specialised tools.
>>>>  *
>>>> - * Note that the flags marked [I] correspond to generic FS_IOC_FLAGS
>>>> + * Note that the flags marked [I] correspond to the FS_IOC_SETFLAGS flags
>>>>  * semantically.  Where possible, the numerical value is picked to correspond
>>>> - * also.
>>>> + * also. Note that the DAX attribute indicates that the inode is currently
>>>> + * DAX-enabled, not simply that the per-inode flag has been set.
>>>
>>> I don't really like using "DAX-enabled" to define "DAX attribute".  How
>>> about cribbing from the statx manpage?
>>>
>>> "Note that the DAX attribute indicates that the file is in the CPU
>>> direct access state.  It does not correspond to the per-inode flag that
>>> some filesystems support."
>>
>> Sure.  Consistency and specificity is good, I'll change that.
>>
>>>>  */
>>>> #define STATX_ATTR_COMPRESSED		0x00000004 /* [I] File is compressed by the fs */
>>>> #define STATX_ATTR_IMMUTABLE		0x00000010 /* [I] File is marked immutable */
>>>> @@ -183,7 +184,7 @@ struct statx {
>>>> #define STATX_ATTR_AUTOMOUNT		0x00001000 /* Dir: Automount trigger */
>>>> #define STATX_ATTR_MOUNT_ROOT		0x00002000 /* Root of a mount */
>>>> #define STATX_ATTR_VERITY		0x00100000 /* [I] Verity protected file */
>>>> -#define STATX_ATTR_DAX			0x00002000 /* [I] File is DAX */
>>>> +#define STATX_ATTR_DAX			0x00400000 /* File is currently DAX-enabled */
>>>
>>> Why not use the next bit in the series (0x200000)?  Did someone already
>>> claim it in for-next?
>>
>> Since it didn't match the FS_IOC_SETFLAGS flag, I was trying to pick one that
>> seemed unlikely to ever gain a corresponding statx flag, and since 0x00400000 is
>> "reserved for ext4" it seemed like a decent choice.
>>
>> But 0x200000 corresponds to FS_EA_INODE_FL/EXT4_EA_INODE_FL which is ext4-specific
>> as well, so sure, I'll change to that.
> 
> If you look a few lines up in the context, this is supposed to be using the
> same value as the other inode flags:
> 
>  * Note that the flags marked [I] correspond to generic FS_IOC_FLAGS
>  * semantically.  Where possible, the numerical value is picked to correspond
>  * also.
> 
> #define FS_DAX_FL                       0x02000000 /* Inode is DAX */
> #define EXT4_DAX_FL                     0x02000000 /* Inode is DAX */
> 
> (FS_DAX_FL also used by XFS) so this should really be "0x02000000" instead
> of some other value.

Setting this attribute in statx means that "the file is in the CPU direct access
state," not simply that the flag is set on disk.  There is not a 1:1 correspondence
in state, so I intentionally did not choose the same flag value.

(Honestly, no idea why we try to match the values in any case, all it leads to is...
this, AFAICT.)

-Eric



-Eric