Re: [PATCH 00/39 v5] xfs: CIL and log optimisations

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 00/39 v5] xfs: CIL and log optimisations
Date: Fri, 4 Jun 2021 08:43:35 +1000	[thread overview]
Message-ID: <20210603224335.GT664593@dread.disaster.area> (raw)
In-Reply-To: <20210603170513.GH26402@locust>

On Thu, Jun 03, 2021 at 10:05:13AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 03, 2021 at 03:22:01PM +1000, Dave Chinner wrote:
> > Hi folks,
> > 
> > This is an update of the consolidated log scalability patchset I've been working
> > on. Version 4 was posted here:
> > 
> > https://lore.kernel.org/linux-xfs/20210519121317.585244-1-david@fromorbit.com/
> > 
> > This version contains the changes Darrick requested during review. The only
> > patch remaining without at least one RVB tag is patch 30.
> > 
> > Performance improvements are largely documented in the change logs of the
> > individual patches. Headline numbers are an increase in transaction rate from
> > 700k commits/s to 1.7M commits/s, and a reduction in fua/flush operations by
> > 2-3 orders of magnitude on metadata heavy workloads that don't use fsync.
> > 
> > Summary of series:
> > 
> > Patches		Modifications
> > -------		-------------
> > 1-7:		log write FUA/FLUSH optimisations
> > 8:		bug fix
> > 9-11:		Async CIL pushes
> > 12-25:		xlog_write() rework
> > 26-39:		CIL commit scalability
> 
> From this latest posting, I see that the first nine patches all have
> multiple reviews.  Some of patches 10-19 have review tags split between
> Brian and Christoph, but neither have added them all the way through.
> I think I'm the only one who has supplied RVB tags for all forty.
> 
> So my question is: at what point would you like me to pull the segments
> of this patchset into upstream?  "The maintainer reviewed everything" is
> of course our usual standard, but this touches a /lot/ of core logging
> code, and logging isn't one of my stronger areas of familiarity.
> 
> I think 1-8 look fine for 5.14.  Do you want me to wait for Brian and/or
> Christoph (or really, any third pair of eyes) to finish working their
> way through 9-11 and 12-25 before merging them?

Quite frankly, I don't think waiting longer for more review is going
to do much to improve the code further. It's largely unchanged since
the last merge cycle went by when I was already waiting for reviews
and it was considered "not enough time to review in this cycle".
It's got enough reviews now to pass the merge bar, and the only way
we are going to shake any remaining problems with this code out is
to merge it and get it out to a wider testing base....

Indeed, if it is merged now it is going to sit another 3 months in
for-next+rc kernels before it is released to users, so I don't think
having it sit for another 3 months only in my test tree before it
gets wider testing benefits anyone. All it does is slow me down and
start pushing me towards having an entirely unmanageable review
backlog like you already have, Darrick.

Given that our rate-of-merge limitations are largely caused by a
lack of review bandwidth, putting off merging code that has already
met the review bar so it can have "more review" seems like a big
step backwards in terms of working through our review backlog. We
need to review and merge stuff faster, not block more stuff by
trying to make review capture every possible problem before we merge
the code.

So, yeah, if I were maintainer and I saw every patch had a RVB on
it, I'd be merging it straight away. But I'm not the maintainer, so
I'll do whatever you want...

I've fixed the little issues with the last posting, and it ran
through fstests just fine last night, so I'm just about ready to
send you a pull request for this series.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com