On Tue, Dec 1, 2020 at 1:04 PM David Howells <[email protected]> wrote:
>
> Linus Torvalds <[email protected]> wrote:
>
> > And if IS_DAX() is correct, then why shouldn't this just be done in
> > generic code? Why move it to every individual filesystem?
>
> One way of looking at it is that the check is then done for every filesystem -
> most of which don't support it. Not sure whether that's a big enough problem
> to worry about. The same is true of the automount test too, I suppose...
So I'd rather have it in one single place than spread out in the filesystems.
Especially when it turns out that the STATX_ATTR_DAX bitmask value was
wrong - now clearly it doesn't seem to currently *matter* to anything,
but imagine if we had to have some strange compat rule to fix things
up with stat() versioning or similar. That's exactly the kind of code
we would _not_ want in every filesystem.
So basically, the thing that argues against this patch is that it
seems to just duplicate things inside filesystems, when the VFS layter
already has the information.
Now, if the VFS information was possibly stale or wrong, that woudl be
one thing. But then we'd have other and bigger problems elsewhere as
far as I can tell.
IOW - make generic what can be made generic, and try to avoid having
filesystems do their own thing.
[ Replace "filesystems" by "architectures" or whatever else, this is
obviously not a filesystem-specific rule in general. ]
And don't get me wrong - I don't _hate_ the patch, and I don't care
_that_ deeply, but it just doesn't seem to make any sense to me. My
initial query was really about "what am I missing - can you please
flesh out the commit message because I don't understand what's wrong".
Linus
On 12/1/20 4:09 PM, Linus Torvalds wrote:
> So basically, the thing that argues against this patch is that it
> seems to just duplicate things inside filesystems, when the VFS layter
> already has the information.
>
> Now, if the VFS information was possibly stale or wrong, that woudl be
> one thing. But then we'd have other and bigger problems elsewhere as
> far as I can tell.
>
> IOW - make generic what can be made generic, and try to avoid having
> filesystems do their own thing.
>
> [ Replace "filesystems" by "architectures" or whatever else, this is
> obviously not a filesystem-specific rule in general. ]
>
> And don't get me wrong - I don't _hate_ the patch, and I don't care
> _that_ deeply, but it just doesn't seem to make any sense to me. My
> initial query was really about "what am I missing - can you please
> flesh out the commit message because I don't understand what's wrong".
Backing way up, my motivation was: Only the filesystem can appropriately
set the statx->attributes_mask, so it has to be done there. Since that
has to be done in the filesystem, set the actual attribute flag adjacent
to it, as is done for ~every other flag.
*shrug*
In any case I resent the flag value clash fix on a separate thread as
V2, hopefully that one is straightforward enough to go in.
Thanks,
-Eric