linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
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>
Subject: Re: [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems
Date: Tue, 1 Dec 2020 14:09:16 -0800	[thread overview]
Message-ID: <CAHk-=wgB_e1anR0b4B5p3qxR9nq1-xrRponA6Q6WbGTOSFNmPw@mail.gmail.com> (raw)
In-Reply-To: <300456.1606856642@warthog.procyon.org.uk>

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

  reply	other threads:[~2020-12-01 22:10 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-01 16:54 [PATCH 0/2] statx: Fix DAX attribute collision and handling Eric Sandeen
2020-12-01 16:57 ` [PATCH 1/2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT Eric Sandeen
2020-12-01 17:32   ` Darrick J. Wong
2020-12-01 17:44     ` Eric Sandeen
2020-12-01 18:31       ` Andreas Dilger
2020-12-01 18:36         ` Eric Sandeen
2020-12-02  2:16   ` Ira Weiny
2020-12-02 20:42     ` Linus Torvalds
2020-12-03  2:45       ` Ira Weiny
2020-12-03 18:04         ` Linus Torvalds
2020-12-01 16:59 ` [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems Eric Sandeen
2020-12-01 17:39   ` Darrick J. Wong
2020-12-01 17:53     ` Eric Sandeen
2020-12-01 20:52     ` Dave Chinner
2020-12-01 22:03       ` Eric Sandeen
2020-12-01 22:12         ` Linus Torvalds
2020-12-01 22:26           ` Eric Sandeen
2020-12-02  2:29             ` Ira Weiny
2020-12-02  8:03           ` Christoph Hellwig
2020-12-01 20:04   ` Linus Torvalds
2020-12-01 20:50     ` Eric Sandeen
2020-12-01 21:04   ` David Howells
2020-12-01 22:09     ` Linus Torvalds [this message]
2020-12-02  0:11       ` Eric Sandeen
2020-12-01 17:18 ` [PATCH 1/2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT David Howells
2020-12-01 17:20 ` [PATCH 2/2] statx: move STATX_ATTR_DAX attribute handling to filesystems David Howells
2020-12-01 17:28 ` David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wgB_e1anR0b4B5p3qxR9nq1-xrRponA6Q6WbGTOSFNmPw@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-man@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=mszeredi@redhat.com \
    --cc=sandeen@redhat.com \
    --cc=xifeng@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).