[patch v4 00/13] xfs: remove the xfssyncd mess

[Dave Chinner]

On Sat, Oct 06, 2012 at 12:37:45PM -0500, Ben Myers wrote:
> Hi Dave,
> 
> On Sat, Oct 06, 2012 at 11:31:22AM +1000, Dave Chinner wrote:
> > On Fri, Oct 05, 2012 at 12:18:53PM -0500, Ben Myers wrote:
> > > Hi Dave,
> > > 
> > > Here I am reposting your xfssyncd series.  I want to make sure that
> > > we're all on the same page.  In particular, are we all happy with patch
> > > 6, 'xfs: xfs_sync_data is redundant'?
> > > 
> > > Version 4:
> > > - updated 'xfs: xfs_sync_data is redundant' with cleanups to the 
> > >   xfs_flush_inodes interface as per Christoph's request,
> > > - updated 'xfs: xfs_sync_data is redundant', folding in changes from 
> > >   http://oss.sgi.com/archives/xfs/2012-10/msg00036.html 
> > > - fixed a minor typo in xfs: 'syncd workqueue is no more', renaming the 
> > >   log worker from 'xfs-reclaim' to 'xfs-log'.
> > > 
> > > I was going to rush this in for the 3.7 merge window.  However in the
> > > light of the issues with patch 6 and Linus's comment here:
> > > http://lkml.org/lkml/2012/9/30/152 and Stephen's comment here:
> > > https://lkml.org/lkml/2012/9/23/144, I think it wiser to behave.  3.7
> > > is stable without this series, so I will merge it for 3.8.
> > > 
> > > Once we have an agreement that patch 6 is ready I will pull this in to the
> > > master branch first thing after the 3.7-rc1 merge from upstream.
> > 
> > <sigh>
> > 
> > Seriously?
> 
> Take it as good news. 

Not really.

> Brian found the regression and put the brakes on before
> I pulled it in.

Right, and I posted an initial fix for it 4 hours after Brian
reported it. That was 10am Tuesday, my time. Brian reported the
warning overnight, and the version of the patch that fixed that I
posted at 6:50am Wednesday morning. You asked about folding it at 10am
Thursday morning, and that's the last I heard until Saturday morning
after you reposted the entire series....

Indeed, the reason you wanted to fold it was this:

"I'd prefer to apply this to patch 6 itself rather than check in the
regression followed by the fix."

Which really isn't something that should be a concern. regressions
get checked in all the time, and fixes for them get checked in later
all the time. Indeed, I think that having commits that explain
regressions and how they were found and fixed is far more important
to have in the revision history than to try to avoid them
altogether. People learn more from mistakes and documenting (so that
others can learn from them, too) than trying pretending they never
occurred.

> My regret in this is that SGI didn't find it first.  We have a
> very stable 3.7 release, one less regression, and a full release cycle to test
> this series in the master branch.  That is a pretty good outcome for XFS users.

Actually, it's not.

It now will be 5-6 months before the change gets to release and
users will actually start testing it, rather than using the 2
months after the -rc1 merge to send small fixes to Linus to fix
minor regressions. In 6 months time, I expect that the code base is
going to be fully CRC enabled, and finding and fixing problems with
stuff we committed now is going to be the least of our problem.

Upstream is supposed to be agile, fast and respond quickly to
changes and challenges. If you want something that is "very stable",
then that's what the long-term stable releases (like 3.0.x) and
downstream kernel consumers like enterprise distros are for. Indeed,
in software terms "very stable" is the equivalent of "did not
change", and that is a fitting description of the 3.7 merge:

Release			diffstat
v3.6..xfs-oss/master	7 files changed, 447 insertions(+), 80 deletions(-)
v3.5..v3.6		51 files changed, 2607 insertions(+), 2455 deletions(-)
v3.4..v3.5		81 files changed, 2633 insertions(+), 3004 deletions(-)
v3.3..v3.4		61 files changed, 1692 insertions(+), 2356 deletions(-)
v3.2..v3.3		45 files changed, 954 insertions(+), 2182 deletions(-)
v3.1..v3.2		54 files changed, 2414 insertions(+), 2625 deletions(-)
v3.0..v3.1		111 files changed, 3237 insertions(+), 5169 deletions(-)

An order of magnitude lower changes than the average over the past 6
releases. IOWs, "did not change" is a good description for this release.

> > Why am I only finding out that there needs to be more
> > rework to patches in this series after someone else reposted them?
> 
> You aren't.  There are currently two pending suggestions for the series, and
> apparently one has been around for awhile:
> 
> 1) 'xfs: sync work is now only periodic log work'
> HCH: "I still think queueing the work item here if we return a failure is
> the wrong thing to do."

Sure, but it also has:

Reviewed-by: Mark Tinguely <tinguely at sgi.com>

on it. Which meant when I last went through the series, I saw this
tag and didn't look any further. I figured that I'd already
addressed the review comments. That's my mistake, but one you, as a
maintainer, need to point out and ask me to address.

> 2) 'xfs: xfs_sync_data is redundant.'
> HCH: (on xfs_flush_inodes) "It's more than a trivial wrapper now, so I'd
> suggest to move out of line to e.g.  xfs_super.c"
> 
> This one is my fault.  I might have considered moving xfs_flush_inodes out of
> xfs_mount.h when I folded your other patch in.

And this is precisely what I'm concerned about. The moment there's a
"do I need to?" type of concern about folding two patches, then it
should be punted straight back to the patch series owner with a
relevant direction/question.