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

* [RFC 0/3] block/file-posix: Work around XFS bug
@ 2019-10-25  9:58 Max Reitz
  2019-10-25  9:58 ` [RFC 1/3] block: Make wait/mark serialising requests public Max Reitz
                   ` (7 more replies)
  0 siblings, 8 replies; 47+ messages in thread
From: Max Reitz @ 2019-10-25  9:58 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Anton Nefedov, Alberto Garcia, qemu-devel, Max Reitz,
	Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi

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.)

Max Reitz (3):
  block: Make wait/mark serialising requests public
  block/file-posix: Detect XFS with CONFIG_FALLOCATE
  block/file-posix: Let post-EOF fallocate serialize

 include/block/block_int.h |  3 +++
 block/file-posix.c        | 46 +++++++++++++++++++++++++++++++++++++--
 block/io.c                | 24 ++++++++++----------
 3 files changed, 59 insertions(+), 14 deletions(-)

-- 
2.21.0

^ permalink raw reply	[flat|nested] 47+ messages in thread