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

On 12/1/20 2:04 PM, Linus Torvalds wrote:

On Tue, Dec 1, 2020 at 8:59 AM Eric Sandeen [off-list ref] wrote:

quoted

It's a bit odd to set STATX_ATTR_DAX into the statx attributes in the VFS;
while the VFS can detect the current DAX state, it is the filesystem which
actually sets S_DAX on the inode, and the filesystem is the place that
knows whether DAX is something that the "filesystem actually supports" [1]
so that the statx attributes_mask can be properly set.

So, move STATX_ATTR_DAX attribute setting to the individual dax-capable
filesystems, and update the attributes_mask there as well.

I'm not really understanding the logic behind this.

The whole IS_DAX(inode) thing exists in various places outside the
low-level filesystem, why shouldn't stat() do this?

If IS_DAX() is incorrect, then we have much bigger problems than some
stat results. We have core functions like generic_file_read_iter() etc
all making actual behavioral judgements on IS_DAX().

It's not incorrect, I didn't mean to imply that. Current code does accurately
set the DAX flag in the statx attributes.
 

And if IS_DAX() is correct, then why shouldn't this just be done in
generic code? Why move it to every individual filesystem?

At the end of the day, it's because only the individual filesystems can
manage the dax flag in the statx attributes_mask. (That's only place that
knows if dax "is available" in general, as opposed to being set on a specific
inode) So if they have to do that, they may as well set the actual attribute
as well, like they do for every other flag they manage...

I mean, we could leave the statx->attributes setting in the vfs, and add
the statx->attributes_mask setting to each dax-capable filesystem. That just
felt a bit asymmetric vs. the way every other filesystem-specific flag gets
handled.

-Eric