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
signature.asc
Description: PGP signature
