Re: [PATCH 2/8] xfs: separate CIL commit record IO

[Brian Foster <bfoster@redhat.com>]

On Mon, Mar 01, 2021 at 10:46:42AM +1100, Dave Chinner wrote:
> On Sun, Feb 28, 2021 at 11:36:13AM -0500, Brian Foster wrote:
> > On Thu, Feb 25, 2021 at 06:48:28PM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 25, 2021 at 09:34:47AM +0100, Christoph Hellwig wrote:
> > > > On Thu, Feb 25, 2021 at 08:44:17AM +1100, Dave Chinner wrote:
> > > > > > Also, do you have any idea what was Christoph talking about wrt devices
> > > > > > with no-op flushes the last time this patch was posted?  This change
> > > > > > seems straightforward to me (assuming the answers to my two question are
> > > > > > 'yes') but I didn't grok what subtlety he was alluding to...?
> > > > > 
> > > > > He was wondering what devices benefited from this. It has no impact
> > > > > on highspeed devices that do not require flushes/FUA (e.g. high end
> > > > > intel optane SSDs) but those are not the devices this change is
> > > > > aimed at. There are no regressions on these high end devices,
> > > > > either, so they are largely irrelevant to the patch and what it
> > > > > targets...
> > > > 
> > > > I don't think it is that simple.  Pretty much every device aimed at
> > > > enterprise use does not enable a volatile write cache by default.  That
> > > > also includes hard drives, arrays and NAND based SSDs.
> > > > 
> > > > Especially for hard drives (or slower arrays) the actual I/O wait might
> > > > matter.  What is the argument against making this conditional?
> > > 
> > > I still don't understand what you're asking about here --
> > > 
> > > AFAICT the net effect of this patchset is that it reduces the number of
> > > preflushes and FUA log writes.  To my knowledge, on a high end device
> > > with no volatile write cache, flushes are a no-op (because all writes
> > > are persisted somewhere immediately) and a FUA write should be the exact
> > > same thing as a non-FUA write.  Because XFS will now issue fewer no-op
> > > persistence commands to the device, there should be no effect at all.
> > > 
> > 
> > Except the cost of the new iowaits used to implement iclog ordering...
> > which I think is what Christoph has been asking about..?
> 
> And I've already answered - it is largely just noise.
> 
> > IOW, considering the storage configuration noted above where the impact
> > of the flush/fua optimizations is neutral, the net effect of this change
> > is whatever impact is introduced by intra-checkpoint iowaits and iclog
> > ordering. What is that impact?
> 
> All I've really noticed is that long tail latencies on operations go
> down a bit. That seems to correlate with spending less time waiting
> for log space when the log is full, but it's a marginal improvement
> at best.
> 
> Otherwise I cannot measure any significant difference in performance
> or behaviour across any of the metrics I monitor during performance
> testing.
> 

Ok.

> > Note that it's not clear enough to me to suggest whether that impact
> > might be significant or not. Hopefully it's neutral (?), but that seems
> > like best case scenario so I do think it's a reasonable question.
> 
> Yes, It's a reasonable question, but I answered it entirely and in
> great detail the first time.  Repeating the same question multiple
> times just with slightly different phrasing does not change the
> answer, nor explain to me what the undocumented concern might be...
> 

Darrick noted he wasn't clear on the question being asked. I rephrased
it to hopefully add some clarity, not change the answer (?).

(FWIW, the response in the previous version of this series didn't
clearly answer the question from my perspective either, so perhaps that
is why you're seeing it repeated by multiple reviewers. Regardless,
Christoph already replied with more detail so I'll just follow along in
that sub-thread..)

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>