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

[Vladimir Sementsov-Ogievskiy]

26.10.2019 20:37, Nir Soffer wrote:
> On Fri, Oct 25, 2019 at 1:11 PM Max Reitz <address@hidden> 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
> 
> Correctness is more important than performance, so this is my
> preference as a user.
> 

Hmm, still, incorrect is XFS, not Qemu. This bug may be triggered by another
software, or may be another scenario in Qemu (not sure).

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


-- 
Best regards,
Vladimir