Re: [RFC 0/3] block/file-posix: Work around XFS bug

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10538 bytes --]

On Fri, Oct 25, 2019 at 02:36:49PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 25.10.2019 17:19, Max Reitz wrote:
> > On 25.10.19 15:56, Vladimir Sementsov-Ogievskiy wrote:
> >> 25.10.2019 16:40, Vladimir Sementsov-Ogievskiy wrote:
> >>> 25.10.2019 12:58, Max Reitz wrote:
> >>>> Hi,
> >>>>
> >>>> It seems to me that there is a bug in Linux’s XFS kernel driver, as
> >>>> I’ve explained here:
> >>>>
> >>>> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01429.html
> >>>>
> >>>> In combination with our commit c8bb23cbdbe32f, this may lead to guest
> >>>> data corruption when using qcow2 images on XFS with aio=native.
> >>>>
> >>>> We can’t wait until the XFS kernel driver is fixed, we should work
> >>>> around the problem ourselves.
> >>>>
> >>>> This is an RFC for two reasons:
> >>>> (1) I don’t know whether this is the right way to address the issue,
> >>>> (2) Ideally, we should detect whether the XFS kernel driver is fixed and
> >>>>       if so stop applying the workaround.
> >>>>       I don’t know how we would go about this, so this series doesn’t do
> >>>>       it.  (Hence it’s an RFC.)
> >>>> (3) Perhaps it’s a bit of a layering violation to let the file-posix
> >>>>       driver access and modify a BdrvTrackedRequest object.
> >>>>
> >>>> As for how we can address the issue, I see three ways:
> >>>> (1) The one presented in this series: On XFS with aio=native, we extend
> >>>>       tracked requests for post-EOF fallocate() calls (i.e., write-zero
> >>>>       operations) to reach until infinity (INT64_MAX in practice), mark
> >>>>       them serializing and wait for other conflicting requests.
> >>>>
> >>>>       Advantages:
> >>>>       + Limits the impact to very specific cases
> >>>>         (And that means it wouldn’t hurt too much to keep this workaround
> >>>>         even when the XFS driver has been fixed)
> >>>>       + Works around the bug where it happens, namely in file-posix
> >>>>
> >>>>       Disadvantages:
> >>>>       - A bit complex
> >>>>       - A bit of a layering violation (should file-posix have access to
> >>>>         tracked requests?)
> >>>>
> >>>> (2) Always skip qcow2’s handle_alloc_space() on XFS.  The XFS bug only
> >>>>       becomes visible due to that function: I don’t think qcow2 writes
> >>>>       zeroes in any other I/O path, and raw images are fixed in size so
> >>>>       post-EOF writes won’t happen.
> >>>>
> >>>>       Advantages:
> >>>>       + Maybe simpler, depending on how difficult it is to handle the
> >>>>         layering violation
> >>>>       + Also fixes the performance problem of handle_alloc_space() being
> >>>>         slow on ppc64+XFS.
> >>>>
> >>>>       Disadvantages:
> >>>>       - Huge layering violation because qcow2 would need to know whether
> >>>>         the image is stored on XFS or not.
> >>>>       - We’d definitely want to skip this workaround when the XFS driver
> >>>>         has been fixed, so we need some method to find out whether it has
> >>>>
> >>>> (3) Drop handle_alloc_space(), i.e. revert c8bb23cbdbe32f.
> >>>>       To my knowledge I’m the only one who has provided any benchmarks for
> >>>>       this commit, and even then I was a bit skeptical because it performs
> >>>>       well in some cases and bad in others.  I concluded that it’s
> >>>>       probably worth it because the “some cases” are more likely to occur.
> >>>>
> >>>>       Now we have this problem of corruption here (granted due to a bug in
> >>>>       the XFS driver), and another report of massively degraded
> >>>>       performance on ppc64
> >>>>       (https://bugzilla.redhat.com/show_bug.cgi?id=1745823 – sorry, a
> >>>>       private BZ; I hate that :-/  The report is about 40 % worse
> >>>>       performance for an in-guest fio write benchmark.)
> >>>>
> >>>>       So I have to ask the question about what the justification for
> >>>>       keeping c8bb23cbdbe32f is.  How much does performance increase with
> >>>>       it actually?  (On non-(ppc64+XFS) machines, obviously)
> >>>>
> >>>>       Advantages:
> >>>>       + Trivial
> >>>>       + No layering violations
> >>>>       + We wouldn’t need to keep track of whether the kernel bug has been
> >>>>         fixed or not
> >>>>       + Fixes the ppc64+XFS performance problem
> >>>>
> >>>>       Disadvantages:
> >>>>       - Reverts cluster allocation performance to pre-c8bb23cbdbe32f
> >>>>         levels, whatever that means
> >>>>
> >>>> So this is the main reason this is an RFC: What should we do?  Is (1)
> >>>> really the best choice?
> >>>>
> >>>>
> >>>> In any case, I’ve ran the test case I showed in
> >>>> https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01282.html
> >>>> more than ten times with this series applied and the installation
> >>>> succeeded every time.  (Without this series, it fails like every other
> >>>> time.)
> >>>>
> >>>>
> >>>
> >>> Hi!
> >>>
> >>> First, great thanks for your investigation!
> >>>
> >>> We need c8bb23cbdbe3 patch, because we use 1M clusters, and zeroing 1M is significant
> >>> in time.
> >>>
> >>> I've tested a bit:
> >>>
> >>> test:
> >>> for img in /ssd/test.img /test.img; do for cl in 64K 1M; do for step in 4K 64K 1M; do ./qemu-img create -f qcow2 -o cluster_size=$cl $img 15G > /dev/null; printf '%-15s%-7s%-10s : ' $img cl=$cl step=$step; ./qemu-img bench -c $((15 * 1024)) -n -s 4K -S $step -t none -w $img | tail -1 | awk '{print $4}'; done; done; done
> >>>
> >>> on master:
> >>>
> >>> /ssd/test.img  cl=64K step=4K    : 0.291
> >>> /ssd/test.img  cl=64K step=64K   : 0.813
> >>> /ssd/test.img  cl=64K step=1M    : 2.799
> >>> /ssd/test.img  cl=1M  step=4K    : 0.217
> >>> /ssd/test.img  cl=1M  step=64K   : 0.332
> >>> /ssd/test.img  cl=1M  step=1M    : 0.685
> >>> /test.img      cl=64K step=4K    : 1.751
> >>> /test.img      cl=64K step=64K   : 14.811
> >>> /test.img      cl=64K step=1M    : 18.321
> >>> /test.img      cl=1M  step=4K    : 0.759
> >>> /test.img      cl=1M  step=64K   : 13.574
> >>> /test.img      cl=1M  step=1M    : 28.970
> >>>
> >>> rerun on master:
> >>>
> >>> /ssd/test.img  cl=64K step=4K    : 0.295
> >>> /ssd/test.img  cl=64K step=64K   : 0.803
> >>> /ssd/test.img  cl=64K step=1M    : 2.921
> >>> /ssd/test.img  cl=1M  step=4K    : 0.233
> >>> /ssd/test.img  cl=1M  step=64K   : 0.321
> >>> /ssd/test.img  cl=1M  step=1M    : 0.762
> >>> /test.img      cl=64K step=4K    : 1.873
> >>> /test.img      cl=64K step=64K   : 15.621
> >>> /test.img      cl=64K step=1M    : 18.428
> >>> /test.img      cl=1M  step=4K    : 0.883
> >>> /test.img      cl=1M  step=64K   : 13.484
> >>> /test.img      cl=1M  step=1M    : 26.244
> >>>
> >>>
> >>> on master + revert c8bb23cbdbe32f5c326
> >>>
> >>> /ssd/test.img  cl=64K step=4K    : 0.395
> >>> /ssd/test.img  cl=64K step=64K   : 4.231
> >>> /ssd/test.img  cl=64K step=1M    : 5.598
> >>> /ssd/test.img  cl=1M  step=4K    : 0.352
> >>> /ssd/test.img  cl=1M  step=64K   : 2.519
> >>> /ssd/test.img  cl=1M  step=1M    : 38.919
> >>> /test.img      cl=64K step=4K    : 1.758
> >>> /test.img      cl=64K step=64K   : 9.838
> >>> /test.img      cl=64K step=1M    : 13.384
> >>> /test.img      cl=1M  step=4K    : 1.849
> >>> /test.img      cl=1M  step=64K   : 19.405
> >>> /test.img      cl=1M  step=1M    : 157.090
> >>>
> >>> rerun:
> >>>
> >>> /ssd/test.img  cl=64K step=4K    : 0.407
> >>> /ssd/test.img  cl=64K step=64K   : 3.325
> >>> /ssd/test.img  cl=64K step=1M    : 5.641
> >>> /ssd/test.img  cl=1M  step=4K    : 0.346
> >>> /ssd/test.img  cl=1M  step=64K   : 2.583
> >>> /ssd/test.img  cl=1M  step=1M    : 39.692
> >>> /test.img      cl=64K step=4K    : 1.727
> >>> /test.img      cl=64K step=64K   : 10.058
> >>> /test.img      cl=64K step=1M    : 13.441
> >>> /test.img      cl=1M  step=4K    : 1.926
> >>> /test.img      cl=1M  step=64K   : 19.738
> >>> /test.img      cl=1M  step=1M    : 158.268
> >>>
> >>>
> >>> So, it's obvious that c8bb23cbdbe32f5c326 is significant for 1M cluster-size, even on rotational
> >>> disk, which means that previous assumption about calling handle_alloc_space() only for ssd is
> >>> wrong, we need smarter heuristics..
> >>>
> >>> So, I'd prefer (1) or (2).
> > 
> > OK.  I wonder whether that problem would go away with Berto’s subcluster
> > series, though.
> 
> Very possible, I thought about it too.
> 
> > 
> >> About degradation in some cases: I think the problem is that one (a bit larger)
> >> write may be faster than fast-write-zeroes + small write, as the latter means
> >> additional write to metadata. And it's expected for small clusters in
> >> conjunction with rotational disk. But the actual limit is dependent on specific
> >> disk. So, I think possible solution is just sometimes try work with
> >> handle_alloc_space and sometimes without, remember time and length of request
> >> and make dynamic limit...
> > 
> > Maybe make a decision based both on the ratio of data size to COW area
> > length (only invoke handle_alloc_space() under a certain threshold), and
> > the absolute COW area length (always invoke it above a certain
> > threshold, unless the ratio doesn’t allow it)?
> > 
> 
> Yes, something like this..
> 
> without handle_alloc_space, time = time(write aligned up to cluster)
> with handle_alloc_space, time = time(fast zero write) + time(original write)
> 
> If we have some statistics on normal-write vs zero-write timing, we can just
> calculate both variants and choose faster.
> 
> if (predict_zero_write_time(aligned up request) + predict_write_time(request) < predict_write_time(aligned up request)) {
>     use handle_alloc_space()
> }

Self-tuning based on request latency works great on a quiet host.  If
there are other processes submitting I/O to the disk then measured
latencies may be bogus.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]