| Message ID | 7027520f-7c79-087e-1d00-743bdefa1a1e@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | statx: Fix DAX attribute collision and handling | expand |
Eric Sandeen <sandeen@redhat.com> 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> You should probably have one or two Fixes: lines in it. It might be worth referencing both of the patches that added conflicting bits. Fixes: 80340fe3605c ("statx: add mount_root") Fixes: 712b2698e4c0 ("fs/stat: Define DAX statx attribute") It should probably have: Reported-by: David Howells <dhowells@redhat.com> also as you mentioned that in the cover. You can also add: Reviewed-by: David Howells <dhowells@redhat.com> David
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." > */ > #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? --D > > > #endif /* _UAPI_LINUX_STAT_H */ > -- > 2.17.0 >
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. Thanks, -Eric > --D > >> >> >> #endif /* _UAPI_LINUX_STAT_H */ >> -- >> 2.17.0 >> >
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. Cheers, Andreas
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
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. > */ > #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 */ This will force a change to xfstests at a minimum. And I do know of users who have been using this value. But I have gotten inquires about using the feature so there are users out there. Darrick, do we have someone doing the patches for xfstest? Ira > > > #endif /* _UAPI_LINUX_STAT_H */ > -- > 2.17.0 >
On Tue, Dec 1, 2020 at 6:16 PM Ira Weiny <ira.weiny@intel.com> wrote: > > This will force a change to xfstests at a minimum. And I do know of users who > have been using this value. But I have gotten inquires about using the feature > so there are users out there. If it's only a few tests that fail, I wouldn't worry about it, and the tests should just be updated. But if there are real user concerns, we may need to have some kind of compat code. Because of the whole "no regressions" thing. What would the typical failure cases be in practice? Linus
On Wed, Dec 02, 2020 at 12:42:08PM -0800, Linus Torvalds wrote: > On Tue, Dec 1, 2020 at 6:16 PM Ira Weiny <ira.weiny@intel.com> wrote: > > > > This will force a change to xfstests at a minimum. And I do know of users who > > have been using this value. But I have gotten inquires about using the feature > > so there are users out there. > > If it's only a few tests that fail, I wouldn't worry about it, and the > tests should just be updated. Done[1] > > But if there are real user concerns, we may need to have some kind of > compat code. Because of the whole "no regressions" thing. > > What would the typical failure cases be in practice? The failure will be a user not seeing their file operating in DAX mode when they expect it to. I discussed this with Dan Williams today. He and I agreed the flag is new enough that we don't think users have any released code to the API just yet. So I think we will be ok. Also, after learning what the other flag was for I agree with Christoph that the impact is going to be minimal since users are not typically operating on the root inode. So I think we are ok with just making the change and getting it into stable quickly. Thanks, Ira [1] https://lore.kernel.org/lkml/20201202214629.1563760-1-ira.weiny@intel.com/
On Wed, Dec 2, 2020 at 6:45 PM Ira Weiny <ira.weiny@intel.com> wrote: > > > > What would the typical failure cases be in practice? > > The failure will be a user not seeing their file operating in DAX mode when > they expect it to. > > I discussed this with Dan Williams today. He and I agreed the flag is new > enough that we don't think users have any released code to the API just yet. > So I think we will be ok. Ok, thanks for verification. I've applied it locally in my tree, it will be pushed out later today with other work.. Linus
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. */ #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 */ #endif /* _UAPI_LINUX_STAT_H */
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(-)