Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <e388f379-cd11-a5d2-db82-aa1aa518a582@redhat.com>
 <05a0f4fd-7f62-8fbc-378d-886ccd5b3f11@redhat.com> <CAHk-=wgOu9vgUfOSsjO3hHHxGDn4BKhitC_8XCfgmGKiiSm_ag@mail.gmail.com>
 <300456.1606856642@warthog.procyon.org.uk>
In-Reply-To: <300456.1606856642@warthog.procyon.org.uk>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 1 Dec 2020 14:09:16 -0800
Message-ID: <CAHk-=wgB_e1anR0b4B5p3qxR9nq1-xrRponA6Q6WbGTOSFNmPw@mail.gmail.com>
Subject: Re: [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems
To:     David Howells <dhowells@redhat.com>
Cc:     Eric Sandeen <sandeen@redhat.com>,
        Miklos Szeredi <mszeredi@redhat.com>,
        Ira Weiny <ira.weiny@intel.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-man <linux-man@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        xfs <linux-xfs@vger.kernel.org>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Xiaoli Feng <xifeng@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Dec 1, 2020 at 1:04 PM David Howells <dhowells@redhat.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> 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