Re: [PATCH 5.10 v2 8/8] xfs: assert in xfs_btree_del_cursor should take into account error

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Jun 07, 2022 at 01:01:47PM +1000, Dave Chinner wrote:
> On Tue, Jun 07, 2022 at 01:33:06AM +0300, Amir Goldstein wrote:
> > On Tue, Jun 7, 2022 at 12:30 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > On Mon, Jun 06, 2022 at 05:32:55PM +0300, Amir Goldstein wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > commit 56486f307100e8fc66efa2ebd8a71941fa10bf6f upstream.
> > > >
> > > > xfs/538 on a 1kB block filesystem failed with this assert:
> > > >
> > > > XFS: Assertion failed: cur->bc_btnum != XFS_BTNUM_BMAP || cur->bc_ino.allocated == 0 || xfs_is_shutdown(cur->bc_mp), file: fs/xfs/libxfs/xfs_btree.c, line: 448
> > >
> > > You haven't mentioned that you combined a second upstream
> > > commit into this patch to fix the bug in this commit.....
> > >
> > 
> > I am confused.
> > 
> > patch [5.10 7/8] xfs: consider shutdown in bmapbt cursor delete assert
> > is the patch that I backported from 5.12 and posted for review.
> > This patch [5.10 8/8] is the patch from 5.19-rc1 that you pointed out
> > that I should take to fix the bug in patch [5.10 7/8].
> 
> Sorry, I missed that this was a new patch because the set looked
> the same as the last posting and you said in the summary letter:
> 
> "These fixes have been posted to review on xfs list [1]."
> 
> Except this patch *wasn't part of that set*. I mistook it for the
> patch that introduced the assert because I assumed from the above
> statement, the absence of a changelog in cover letter and that you'd
> sent it to Greg meant for inclusion meant *no new patches had been
> added*.
> 
> Add to that the fact I rushed through them because I saw that Greg
> has already queued these before anyone had any time to actually
> review the posting. Hence the timing of the release of unreviewed
> patches has been taken completely out of our control, and so I
> rushed through them and misinterpreted what I was seeing.
> 
> That's not how the review process is supposed to work. You need to
> wait for people to review the changes and ACK them before then
> asking for them to be merged into the stable trees. You need to have
> changelogs in your summary letters. You need to do all the things
> that you've been complaining bitterly about over the past month that
> upstream developers weren't doing 18 months ago.

I thought these had already been reviewed, which is why I took them.

And there still are days before these go anywhere, just adding them to
the stable queue doesn't mean they are "set in stone".

Heck, even if they do get merged into a stable release, 'git revert' is
our friend here, and we can easily revert anything that is found to be
wrong.

> And I notice that you've already sent out yet another set of stable
> patches for review despite the paint not even being dry on these
> ones. Not to mention that there's a another set of different 5.15
> stable patches out for review as well.
> 
> This is not a sustainable process.
> 
> Seriously: slow down and stop being so damn aggressive. Let everyone
> catch up and build sustainable processes and timetables. If you keep
> going like this, you are going break people.

What am I supposed to do here, not take patches you all send me?  Wait
X number of days?

totally confused,

greg k-h