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;
Date:   Thu, 6 Jul 2023 12:40:55 -0400
From:   Josef Bacik <josef@toxicpanda.com>
To:     Kent Overstreet <kent.overstreet@linux.dev>
Cc:     torvalds@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-bcachefs@vger.kernel.org,
        djwong@kernel.org, dchinner@redhat.com, sandeen@redhat.com,
        willy@infradead.org, tytso@mit.edu, bfoster@redhat.com,
        jack@suse.cz, andreas.gruenbacher@gmail.com, brauner@kernel.org,
        peterz@infradead.org, akpm@linux-foundation.org,
        dhowells@redhat.com
Subject: Re: [GIT PULL] bcachefs
Message-ID: <20230706164055.GA2306489@perftesting>
References: <20230626214656.hcp4puionmtoloat@moria.home.lan>
 <20230706155602.mnhsylo3pnief2of@moria.home.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230706155602.mnhsylo3pnief2of@moria.home.lan>
Precedence: bulk

On Thu, Jul 06, 2023 at 11:56:02AM -0400, Kent Overstreet wrote:
> On Mon, Jun 26, 2023 at 05:47:01PM -0400, Kent Overstreet wrote:
> > Hi Linus,
> > 
> > Here it is, the bcachefs pull request. For brevity the list of patches
> > below is only the initial part of the series, the non-bcachefs prep
> > patches and the first bcachefs patch, but the diffstat is for the entire
> > series.
> > 
> > Six locks has all the changes you suggested, text size went down
> > significantly. If you'd still like this to see more review from the
> > locking people, I'm not against them living in fs/bcachefs/ as an
> > interim; perhaps Dave could move them back to kernel/locking when he
> > starts using them or when locking people have had time to look at them -
> > I'm just hoping for this to not block the merge.
> > 
> > Recently some people have expressed concerns about "not wanting a repeat
> > of ntfs3" - from what I understand the issue there was just severe
> > buggyness, so perhaps showing the bcachefs automated test results will
> > help with that:
> > 
> >   https://evilpiepirate.org/~testdashboard/ci
> > 
> > The main bcachefs branch runs fstests and my own test suite in several
> > varations, including lockdep+kasan, preempt, and gcov (we're at 82% line
> > coverage); I'm not currently seeing any lockdep or kasan splats (or
> > panics/oopses, for that matter).
> > 
> > (Worth noting the bug causing the most test failures by a wide margin is
> > actually an io_uring bug that causes random umount failures in shutdown
> > tests. Would be great to get that looked at, it doesn't just affect
> > bcachefs).
> > 
> > Regarding feature status - most features are considered stable and ready
> > for use, snapshots and erasure coding are both nearly there. But a
> > filesystem on this scale is a massive project, adequately conveying the
> > status of every feature would take at least a page or two.
> > 
> > We may want to mark it as EXPERIMENTAL for a few releases, I haven't
> > done that as yet. (I wouldn't consider single device without snapshots
> > to be experimental, but - given that the number of users and bug reports
> > is about to shoot up, perhaps I should...).
> 
> Restarting the discussion after the holiday weekend, hoping to get
> something more substantive going:
> 
> Hoping to get:
>  - Thoughts from people who have been following bcachefs development,
>    and people who have looked at the code
>  - Continuation of the LSF discussion - maybe some people could repeat
>    here what they said there (re: code review, iomap, etc.)
>  - Any concerns about how this might impact the rest of the kernel, or
>    discussion about what impact merging a new filesystem is likely to
>    have on other people's work
> 
> AFAIK the only big ask that hasn't happened yet is better documentation:
> David Howells wanted (better) a man page, which is definitely something
> that needs to happen but it'll be some months before I'm back to working
> on documentation - I'm happy to share my current list of priorities if
> that would be helpful.
> 
> In the meantime, the Latex principles of operation is reasonably up to
> date (and I intend to greatly expand the sections on on disk data
> structures, I think that'll be great reference documentation for
> developers getting up to speed on the code)
> 
> https://bcachefs.org/bcachefs-principles-of-operation.pdf
> 
> I feel that bcachefs is in a pretty mature state at this point, but it's
> also _huge_, which is a bit different than e.g. the btrfs merger; it's
> hard to know where to start to get a meaninful discussion/review process
> going.
> 
> Patch bombing the mailing list with 90k loc is clearly not going to be
> productive, which is why I've been trying to talk more about development
> process and status - but all suggestions and feedback are welcome.

I've been watching this from the sidelines sort of busy with other things, but I
realize that comments I made at LSFMMBPF have been sort of taken as the gospel
truth and I want to clear some of that up.

I said this at LSFMMBPF, and I haven't said it on list before so I'll repeat it
here.

I'm of the opinion that me and any other outsider reviewing the bcachefs code in
bulk is largely useless.  I could probably do things like check for locking
stuff and other generic things.

You have patches that are outside of fs/bcachefs.  Get those merged and then do
a pull with just fs/bcachefs, because again posting 90k loc is going to be
unwieldy and the quality of review just simply will not make a difference.

Alternatively rework your code to not have any dependencies outside of
fs/bcachefs.  This is what btrfs did.  That merge didn't touch anything outside
of fs/btrfs.

This merge attempt has gone off the rails, for what appears to be a few common
things.

1) The external dependencies.  There's a reason I was really specific about what
I said at LSFMMBPF, both this year and in 2022.  Get these patches merged first,
the rest will be easier.  You are burning a lot of good will being combative
with people over these dependencies.  This is not the hill to die on.  You want
bcachefs in the kernel and to get back to bcachefs things.  Make the changes you
need to make to get these dependencies in, or simply drop the need for them and
come back to it later after bcachefs is merged.

2) We already have recent examples of merge and disappear.  Yes of course you've
been around for a long time, you aren't the NTFS developers.  But as you point
out it's 90k of code.  When btrfs was merged there were 3 large contributors,
Chris, myself, and Yanzheng.  If Chris got hit by a bus we could still drive the
project forward.  Can the same be said for bachefs?  I know others have chimed
in and done some stuff, but as it's been stated elsewhere it would be good to
have somebody else in the MAINTAINERS file with you.

I am really, really wanting you to succeed here Kent.  If the general consensus
is you need to have some idiot review fs/bcachefs I will happily carve out some
time and dig in.

At this point however it's time to be pragmatic.  Stop dying on every hill, it's
not worth it.  Ruthlessly prioritize and do what needs to be done to get this
thing merged.  Christian saying he's almost ready to stop replying should be a
wakeup call that your approach is not working.  Thanks,

Josef