On 12/1/20 2:04 PM, Linus Torvalds wrote: > On Tue, Dec 1, 2020 at 8:59 AM Eric Sandeen <sandeen@xxxxxxxxxx> wrote: >> >> 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
