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).
--
Best regards,
Vladimir
