Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220811152446.281723-1-idryomov@gmail.com>
In-Reply-To: <20220811152446.281723-1-idryomov@gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 11 Aug 2022 13:03:57 -0700
Message-ID: <CAHk-=wifgq59uru6xDB=nY-1p6aQ-1YB8nVhW7T-N2ctK3m1gw@mail.gmail.com>
Subject: Re: [GIT PULL] Ceph updates for 5.20-rc1
To:     Ilya Dryomov <idryomov@gmail.com>
Cc:     ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 11, 2022 at 8:25 AM Ilya Dryomov <idryomov@gmail.com> wrote:
>
>    [..] Several patches
> touch files outside of our normal purview to set the stage for bringing
> in Jeff's long awaited ceph+fscrypt series in the near future.  All of
> them have appropriate acks and sat in linux-next for a while.

What? No.

I'm looking at the fs/dcache.c change, for example, and don't see the
relevant maintainers having acked it at all.

The mmdebug.h file similarly seems to not have the actual maintainer
acks, and seems just plain stupid (why does it add that new folio
warning macro, when the existing folio warning macro
VM_WARN_ON_ONCE_FOLIO() is *better*?

Those are some very core files, and while the changes seem harmless,
they sure don't seem obviously ok.

What's the point of warning about bogus folios more than once? That's
a debug warning - if it hits even once, that's already "uhhuh,
something is bad". Showing the warning more than once is likely just
going to cause more problems, not give you more information.

And did somebody verify that d_same_name() is still inlined in the
place that truly *matters*?  Because from my quick test, that patch
broke it. Now __d_lookup() does a function call.

And I _suspect_ it's all ok, because it turns out that
__d_lookup_rcu() is the *really* hot case, and that one has inlined it
all manually.

But this kind of "we touch some *truly* core functionality, without
the acks from the maintainers, and then we *claim* to have relevant
acks" is really not even remotely ok.

I've pulled this because I suspect that d_same_name() thing is fine,
and I think the VM_WARN_ON_FOLIO() addition is completely wrong but
not horrendous.

But you're on my tentative shit-list just for having claimed to have
appropriate acks and having been found wanting.

Just for your information: fs/dcache.c is some of the most optimized
code in the kernel, and some of the subtlest. That RCU pathname lookup
is serious business. You don't make changes to pathname lookup just
willy nilly. There's a reason I start looking at individual patches
when I see it in the diffstat.

And please just get rid of VM_WARN_ON_FOLIO() again, and use the
VM_WARN_ON_ONCE_FOLIO() version. Because if you expect to have
multiple, then you probably shouldn't have that warning at ALL.

                 Linus