Clarification of statx->attributes_mask meaning?

Clarification of statx->attributes_mask meaning?

[Eric Sandeen]

The way attributes_mask is used in various filesystems seems a bit
inconsistent.

Most filesystems set only the bits for features that are possible to enable
on that filesystem, i.e. XFS:

        if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
                stat->attributes |= STATX_ATTR_IMMUTABLE;
        if (ip->i_d.di_flags & XFS_DIFLAG_APPEND)
                stat->attributes |= STATX_ATTR_APPEND;
        if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
                stat->attributes |= STATX_ATTR_NODUMP;

        stat->attributes_mask |= (STATX_ATTR_IMMUTABLE |
                                  STATX_ATTR_APPEND |
                                  STATX_ATTR_NODUMP);

btrfs, cifs, erofs, ext4, f2fs, hfsplus, orangefs and ubifs are similar.

But others seem to set the mask to everything it can definitively answer,
i.e. "Encryption and compression are off, and we really mean it" even though
it will never be set to one in ->attributes, i.e. on gfs2:

        if (gfsflags & GFS2_DIF_APPENDONLY)
                stat->attributes |= STATX_ATTR_APPEND;
        if (gfsflags & GFS2_DIF_IMMUTABLE)
                stat->attributes |= STATX_ATTR_IMMUTABLE;

        stat->attributes_mask |= (STATX_ATTR_APPEND |
                                  STATX_ATTR_COMPRESSED |
                                  STATX_ATTR_ENCRYPTED |
                                  STATX_ATTR_IMMUTABLE |
                                  STATX_ATTR_NODUMP);

ext2 is similar (it adds STATX_ATTR_ENCRYPTED to the mask but will never set
it in attributes)

The commit 3209f68b3ca4 which added attributes_mask says:

"Include a mask in struct stat to indicate which bits of stx_attributes the
filesystem actually supports."

The manpage says:

"A mask indicating which bits in stx_attributes are supported by the VFS and
the filesystem."

-and-

"Note that any attribute that is not indicated as supported by stx_attributes_mask
has no usable value here."

So is this intended to indicate which bits of statx->attributes are valid, whether
they are 1 or 0, or which bits could possibly be set to 1 by the filesystem?

If the former, then we should move attributes_mask into the VFS to set all flags
known by the kernel, but David's original commit did not do that so I'm left
wondering...

Thanks,
-Eric

Re: Clarification of statx->attributes_mask meaning?

[Darrick J. Wong]

On Wed, Nov 25, 2020 at 01:19:48PM -0600, Eric Sandeen wrote:
> The way attributes_mask is used in various filesystems seems a bit
> inconsistent.
> 
> Most filesystems set only the bits for features that are possible to enable
> on that filesystem, i.e. XFS:
> 
>         if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
>                 stat->attributes |= STATX_ATTR_IMMUTABLE;
>         if (ip->i_d.di_flags & XFS_DIFLAG_APPEND)
>                 stat->attributes |= STATX_ATTR_APPEND;
>         if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
>                 stat->attributes |= STATX_ATTR_NODUMP;
> 
>         stat->attributes_mask |= (STATX_ATTR_IMMUTABLE |
>                                   STATX_ATTR_APPEND |
>                                   STATX_ATTR_NODUMP);
> 
> btrfs, cifs, erofs, ext4, f2fs, hfsplus, orangefs and ubifs are similar.
> 
> But others seem to set the mask to everything it can definitively answer,
> i.e. "Encryption and compression are off, and we really mean it" even though
> it will never be set to one in ->attributes, i.e. on gfs2:
> 
>         if (gfsflags & GFS2_DIF_APPENDONLY)
>                 stat->attributes |= STATX_ATTR_APPEND;
>         if (gfsflags & GFS2_DIF_IMMUTABLE)
>                 stat->attributes |= STATX_ATTR_IMMUTABLE;
> 
>         stat->attributes_mask |= (STATX_ATTR_APPEND |
>                                   STATX_ATTR_COMPRESSED |
>                                   STATX_ATTR_ENCRYPTED |
>                                   STATX_ATTR_IMMUTABLE |
>                                   STATX_ATTR_NODUMP);
> 
> ext2 is similar (it adds STATX_ATTR_ENCRYPTED to the mask but will never set
> it in attributes)
> 
> The commit 3209f68b3ca4 which added attributes_mask says:
> 
> "Include a mask in struct stat to indicate which bits of stx_attributes the
> filesystem actually supports."
> 
> The manpage says:
> 
> "A mask indicating which bits in stx_attributes are supported by the VFS and
> the filesystem."
> 
> -and-
> 
> "Note that any attribute that is not indicated as supported by stx_attributes_mask
> has no usable value here."
> 
> So is this intended to indicate which bits of statx->attributes are valid, whether
> they are 1 or 0, or which bits could possibly be set to 1 by the filesystem?
> 
> If the former, then we should move attributes_mask into the VFS to set all flags
> known by the kernel, but David's original commit did not do that so I'm left
> wondering...

Personally I thought that attributes_mask tells you which bits actually
make any sense for the given filesystem, which means:

mask=1 bit=0: "attribute not set on this file"
mask=1 bit=1: "attribute is set on this file"
mask=0 bit=0: "attribute doesn't fit into the design of this fs"
mask=0 bit=1: "filesystem is lying snake"

It's up to the fs driver and not the vfs to set attributes_mask, and
therefore (as I keep pointing out to XiaoLi Feng) xfs_vn_getattr should
be setting the mask.

--D

> 
> Thanks,
> -Eric

Re: Clarification of statx->attributes_mask meaning?

[Eric Sandeen]

On 11/25/20 3:25 PM, Darrick J. Wong wrote:
> On Wed, Nov 25, 2020 at 01:19:48PM -0600, Eric Sandeen wrote:
>> The way attributes_mask is used in various filesystems seems a bit
>> inconsistent.
>>
>> Most filesystems set only the bits for features that are possible to enable
>> on that filesystem, i.e. XFS:
>>
>>         if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
>>                 stat->attributes |= STATX_ATTR_IMMUTABLE;
>>         if (ip->i_d.di_flags & XFS_DIFLAG_APPEND)
>>                 stat->attributes |= STATX_ATTR_APPEND;
>>         if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
>>                 stat->attributes |= STATX_ATTR_NODUMP;
>>
>>         stat->attributes_mask |= (STATX_ATTR_IMMUTABLE |
>>                                   STATX_ATTR_APPEND |
>>                                   STATX_ATTR_NODUMP);
>>
>> btrfs, cifs, erofs, ext4, f2fs, hfsplus, orangefs and ubifs are similar.
>>
>> But others seem to set the mask to everything it can definitively answer,
>> i.e. "Encryption and compression are off, and we really mean it" even though
>> it will never be set to one in ->attributes, i.e. on gfs2:
>>
>>         if (gfsflags & GFS2_DIF_APPENDONLY)
>>                 stat->attributes |= STATX_ATTR_APPEND;
>>         if (gfsflags & GFS2_DIF_IMMUTABLE)
>>                 stat->attributes |= STATX_ATTR_IMMUTABLE;
>>
>>         stat->attributes_mask |= (STATX_ATTR_APPEND |
>>                                   STATX_ATTR_COMPRESSED |
>>                                   STATX_ATTR_ENCRYPTED |
>>                                   STATX_ATTR_IMMUTABLE |
>>                                   STATX_ATTR_NODUMP);
>>
>> ext2 is similar (it adds STATX_ATTR_ENCRYPTED to the mask but will never set
>> it in attributes)
>>
>> The commit 3209f68b3ca4 which added attributes_mask says:
>>
>> "Include a mask in struct stat to indicate which bits of stx_attributes the
>> filesystem actually supports."
>>
>> The manpage says:
>>
>> "A mask indicating which bits in stx_attributes are supported by the VFS and
>> the filesystem."
>>
>> -and-
>>
>> "Note that any attribute that is not indicated as supported by stx_attributes_mask
>> has no usable value here."
>>
>> So is this intended to indicate which bits of statx->attributes are valid, whether
>> they are 1 or 0, or which bits could possibly be set to 1 by the filesystem?
>>
>> If the former, then we should move attributes_mask into the VFS to set all flags
>> known by the kernel, but David's original commit did not do that so I'm left
>> wondering...
> 
> Personally I thought that attributes_mask tells you which bits actually
> make any sense for the given filesystem, which means:
> 
> mask=1 bit=0: "attribute not set on this file"
> mask=1 bit=1: "attribute is set on this file"
> mask=0 bit=0: "attribute doesn't fit into the design of this fs"
> mask=0 bit=1: "filesystem is lying snake"
> 
> It's up to the fs driver and not the vfs to set attributes_mask, and
> therefore (as I keep pointing out to XiaoLi Feng) xfs_vn_getattr should
> be setting the mask.

That's what the original commit did, and that's what /most/ of the filesystems do.
And I agree with you on the dax flag there.

So I think ext2 & gfs2 are inconsistent and wrong, and should probably be changed
to remove flags from the mask which are unsupported by the filesystem.

-Eric

Re: Clarification of statx->attributes_mask meaning?

[David Howells]

Darrick J. Wong <darrick.wong@oracle.com> wrote:

> mask=1 bit=0: "attribute not set on this file"
> mask=1 bit=1: "attribute is set on this file"
> mask=0 bit=0: "attribute doesn't fit into the design of this fs"

Or is "not supported by the filesystem driver in this kernel version".

> mask=0 bit=1: "filesystem is lying snake"

I like your phrasing :-)

> It's up to the fs driver and not the vfs to set attributes_mask, and
> therefore (as I keep pointing out to XiaoLi Feng) xfs_vn_getattr should
> be setting the mask.

Agreed.  I think there's some confusion stemming from STATX_ATTR_MOUNT_ROOT -
but that's supported by the *vfs* not by the filesystem.

David

Re: Clarification of statx->attributes_mask meaning?

[Eric Sandeen]

On 11/25/20 3:50 PM, David Howells wrote:
> Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
>> mask=1 bit=0: "attribute not set on this file"
>> mask=1 bit=1: "attribute is set on this file"
>> mask=0 bit=0: "attribute doesn't fit into the design of this fs"
> 
> Or is "not supported by the filesystem driver in this kernel version".

For a concrete example, let's talk about the DAX statx attribute.

If the kernel is configured w/o DAX support, should the DAX attr be in the mask?
If the block device has no DAX support, should the DAX attr be in the mask?
If the filesystem is mounted with dax=never, should the DAX attr be in the mask?

About to send a patch for xfs which answers "no" to all of those, but I'm still
not quite sure if that's what's expected.  I'll be sure to cc: dhowells, Ira, and
others who may care...

-Eric

>> mask=0 bit=1: "filesystem is lying snake"
> 
> I like your phrasing :-)
> 
>> It's up to the fs driver and not the vfs to set attributes_mask, and
>> therefore (as I keep pointing out to XiaoLi Feng) xfs_vn_getattr should
>> be setting the mask.
> 
> Agreed.  I think there's some confusion stemming from STATX_ATTR_MOUNT_ROOT -
> but that's supported by the *vfs* not by the filesystem.
> 
> David
> 

Re: Clarification of statx->attributes_mask meaning?

[Theodore Y. Ts'o]

On Mon, Nov 30, 2020 at 05:29:47PM -0600, Eric Sandeen wrote:
> On 11/25/20 3:50 PM, David Howells wrote:
> > Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > 
> >> mask=1 bit=0: "attribute not set on this file"
> >> mask=1 bit=1: "attribute is set on this file"
> >> mask=0 bit=0: "attribute doesn't fit into the design of this fs"
> > 
> > Or is "not supported by the filesystem driver in this kernel version".
> 
> For a concrete example, let's talk about the DAX statx attribute.
> 
> If the kernel is configured w/o DAX support, should the DAX attr be in the mask?
> If the block device has no DAX support, should the DAX attr be in the mask?
> If the filesystem is mounted with dax=never, should the DAX attr be in the mask?
> 
> About to send a patch for xfs which answers "no" to all of those, but I'm still
> not quite sure if that's what's expected.  I'll be sure to cc: dhowells, Ira, and
> others who may care...

So you're basically proposing that the mask is indicating whether or
not the attribute is supported by a particular on-disk file system
image and/or how it is currently configured/mounted --- and not
whether an attribute is supported by a particular file system
*implementation*.

For example, for ext4, if the extents feature is not enabled (for
example, when the ext4 file system code is used mount a file system
whose feature bitmask is consistent with a historic ext2 file system)
the extents flag should be cleared from the attribute mask?

This adds a fair amount of complexity to the file system since there
are a number of flags that might have similar issues --- for example,
FS_CASEFOLD_FL, and I could imagine for some file systems, where
different revisions might or might not support reflink FS_NOCOW_FL,
etc.

We should be really clear how applications are supposed to use the
attributes_mask.  Does it mean that they will always be able to set a
flag which is set in the attribute mask?  That can't be right, since
there will be a number of flags that may have some more complex checks
(you must be root, or the file must be zero length, etc.)  I'm a bit
unclear about what are the useful ways in which an attribute_mask can
be used by a userspace application --- and under what circumstances
might an application be depending on the semantics of attribute_mask,
so we don't accidentally give them an opportunity to complain and
whine, thus opening ourselves to another O_PONIES controversy.

> >> mask=0 bit=1: "filesystem is lying snake"
> > 
> > I like your phrasing :-)
> > 
> >> It's up to the fs driver and not the vfs to set attributes_mask, and
> >> therefore (as I keep pointing out to XiaoLi Feng) xfs_vn_getattr should
> >> be setting the mask.

... or maybe the on-disk file system is inconsistent....

       	     	 	      	     	- Ted

Re: Clarification of statx->attributes_mask meaning?

[Eric Sandeen]

On 11/30/20 9:20 PM, Theodore Y. Ts'o wrote:
> On Mon, Nov 30, 2020 at 05:29:47PM -0600, Eric Sandeen wrote:
>> On 11/25/20 3:50 PM, David Howells wrote:
>>> Darrick J. Wong <darrick.wong@oracle.com> wrote:
>>>
>>>> mask=1 bit=0: "attribute not set on this file"
>>>> mask=1 bit=1: "attribute is set on this file"
>>>> mask=0 bit=0: "attribute doesn't fit into the design of this fs"
>>>
>>> Or is "not supported by the filesystem driver in this kernel version".
>>
>> For a concrete example, let's talk about the DAX statx attribute.
>>
>> If the kernel is configured w/o DAX support, should the DAX attr be in the mask?
>> If the block device has no DAX support, should the DAX attr be in the mask?
>> If the filesystem is mounted with dax=never, should the DAX attr be in the mask?
>>
>> About to send a patch for xfs which answers "no" to all of those, but I'm still
>> not quite sure if that's what's expected.  I'll be sure to cc: dhowells, Ira, and
>> others who may care...
> 
> So you're basically proposing that the mask is indicating whether or
> not the attribute is supported by a particular on-disk file system
> image and/or how it is currently configured/mounted --- and not
> whether an attribute is supported by a particular file system
> *implementation*.

Well, not trying to propose anything new, just trying to understand
the intent of the mask to get it set correctly for the dax attribute.

> For example, for ext4, if the extents feature is not enabled (for
> example, when the ext4 file system code is used mount a file system
> whose feature bitmask is consistent with a historic ext2 file system)
> the extents flag should be cleared from the attribute mask?
> 
> This adds a fair amount of complexity to the file system since there
> are a number of flags that might have similar issues --- for example,
> FS_CASEFOLD_FL, and I could imagine for some file systems, where
> different revisions might or might not support reflink FS_NOCOW_FL,
> etc.

I've been told that I'm over-complicating this, yes.

> We should be really clear how applications are supposed to use the
> attributes_mask.  Does it mean that they will always be able to set a
> flag which is set in the attribute mask?  That can't be right, since
> there will be a number of flags that may have some more complex checks
> (you must be root, or the file must be zero length, etc.)  I'm a bit
> unclear about what are the useful ways in which an attribute_mask can
> be used by a userspace application --- and under what circumstances
> might an application be depending on the semantics of attribute_mask,
> so we don't accidentally give them an opportunity to complain and
> whine, thus opening ourselves to another O_PONIES controversy.

Hah, indeed.

Sorry if I've over-complicated this, I'm honestly just confused now.

-Eric

Re: Clarification of statx->attributes_mask meaning?

[Eric Sandeen]

On 11/30/20 9:37 PM, Eric Sandeen wrote:
> On 11/30/20 9:20 PM, Theodore Y. Ts'o wrote:

...

>> We should be really clear how applications are supposed to use the
>> attributes_mask.  Does it mean that they will always be able to set a
>> flag which is set in the attribute mask?  That can't be right, since
>> there will be a number of flags that may have some more complex checks
>> (you must be root, or the file must be zero length, etc.)  I'm a bit
>> unclear about what are the useful ways in which an attribute_mask can
>> be used by a userspace application --- and under what circumstances
>> might an application be depending on the semantics of attribute_mask,
>> so we don't accidentally give them an opportunity to complain and
>> whine, thus opening ourselves to another O_PONIES controversy.
> 
> Hah, indeed.
> 
> Sorry if I've over-complicated this, I'm honestly just confused now.

hch warned us, I guess:

https://lore.kernel.org/linux-fsdevel/20170404071252.GA30966@infradead.org/

At this point I guess I'll just set the dax attribute into the mask
unconditionally for xfs, because xfs "supports" dax, and stop navel-gazing
over this.

-Eric

Re: Clarification of statx->attributes_mask meaning?

[Theodore Y. Ts'o]

On Mon, Nov 30, 2020 at 09:37:29PM -0600, Eric Sandeen wrote:
> > We should be really clear how applications are supposed to use the
> > attributes_mask.  Does it mean that they will always be able to set a
> > flag which is set in the attribute mask?  That can't be right, since
> > there will be a number of flags that may have some more complex checks
> > (you must be root, or the file must be zero length, etc.)  I'm a bit
> > unclear about what are the useful ways in which an attribute_mask can
> > be used by a userspace application --- and under what circumstances
> > might an application be depending on the semantics of attribute_mask,
> > so we don't accidentally give them an opportunity to complain and
> > whine, thus opening ourselves to another O_PONIES controversy.
> 
> Hah, indeed.
> 
> Sorry if I've over-complicated this, I'm honestly just confused now.

Yeah, I'm honestly confused too how applications can use the
attributes mask, too.

Presumably there is some case where the flag not being set *and* the
file system can support that attribute, that the application could
infer something interesting.  I just can't figure out what that case
would be.

Yes, I see your pointer to Cristoph's question on this very issue back
in April 2017.  Pity it was never answered, at least that was archived
on lore.

					- Ted

Re: Clarification of statx->attributes_mask meaning?

[Miklos Szeredi]

On Tue, Dec 1, 2020 at 4:42 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Nov 30, 2020 at 09:37:29PM -0600, Eric Sandeen wrote:
> > > We should be really clear how applications are supposed to use the
> > > attributes_mask.  Does it mean that they will always be able to set a
> > > flag which is set in the attribute mask?  That can't be right, since
> > > there will be a number of flags that may have some more complex checks
> > > (you must be root, or the file must be zero length, etc.)  I'm a bit
> > > unclear about what are the useful ways in which an attribute_mask can
> > > be used by a userspace application --- and under what circumstances
> > > might an application be depending on the semantics of attribute_mask,
> > > so we don't accidentally give them an opportunity to complain and
> > > whine, thus opening ourselves to another O_PONIES controversy.
> >
> > Hah, indeed.
> >
> > Sorry if I've over-complicated this, I'm honestly just confused now.
>
> Yeah, I'm honestly confused too how applications can use the
> attributes mask, too.

If the meaning is "the flags value is valid" then the use case would be:

 - look in mask if set, if yes, then can use the corresponding flag

 - if mask is not set, then ignore flag, and try to find out the value
of the property some other (possibly more expensive) way.

For STATX_ATTR_DAX it makes sense, since the value can be determined
in alternative ways on old kernels, so application can fall back if
DAX is not in the mask.

As noted upthread any other use would be ambiguous.

Thanks,
Miklos