Re: [PATCH 11/47] xfs: move deferred operations into a separate file

Dave Chinner
2016-08-03T22:58:01Z

On Wed, Aug 03, 2016 at 02:16:27AM -0700, Christoph Hellwig wrote:
> On Wed, Aug 03, 2016 at 08:39:50AM +1000, Dave Chinner wrote:
> > Rather than have to make major changes to core infrastructure now,
> > let's work this out as a separate patchset to clean up the rmap and
> > reflink code in the next couple of releases. It's going to be better
> > to get working code out there now under the experimental tag than it
> > is is to keep it as an out of tree patchset for another cycle.
>
> The problm is that this does not only affect the rmap code (for which
> I suspect it actually is fine), but also regresses the freed extent
> logging. If you want minimal changes we should simply drop the patches
> to move over the freed extent tracking to the new deferred ops
> mechanism for now.

I haven't said I want "minimal changes".

What I don't want to do is make large, untested changes at the last
minute, especially when there's no evidence that there is a
regression caused by the code being changed. We need this kind of
infrastructure for reflink, and there's no point rewriting the rmap
code to remove it just to then have to immediately re-introduce it
back into to the rmap code so it works with reflink. It's pointless
churn and invalidates all the testing we've done on rmap up to this
point.

I don't understand what problem or regression you think this code
causes, mainly because it hasn't been explained or demonstrated. I
don't understand the solution being proposed, because the little
that has been described tells me nothing about what the problem it
solves is, nor how it solves the atomicity and ordering requirements
that updating the bmbt, refcountbt and rmapbt have.

I have no issues with fixing the code if it is broken, but first
everyone needs to undestand how it is broken. It's time to make
progress on this functionality - if we wait for the code to be
perfect before it gets merged, we'll never make any progress.

So, please explain in more detail what the problem is and what the
proposed solution is so I (and probably Darrick, too) have some
understanding of the issue you see with this code.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com