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

[Vladimir Sementsov-Ogievskiy]

29.10.2019 14:55, Max Reitz wrote:
> On 29.10.19 12:48, Vladimir Sementsov-Ogievskiy wrote:
>> 29.10.2019 11:50, Max Reitz wrote:
>>> On 28.10.19 12:25, Vladimir Sementsov-Ogievskiy wrote:
>>>> 28.10.2019 14:04, Kevin Wolf wrote:
>>>>> Am 27.10.2019 um 13:35 hat Stefan Hajnoczi geschrieben:
>>>>>> On Fri, Oct 25, 2019 at 11:58:46AM +0200, Max Reitz wrote:
>>>
>>> [...]
>>>
>>>>>>> (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
>>>>>>
>>>>>> My favorite because it is clean and simple, but Vladimir has a valid
>>>>>> use-case for requiring this performance optimization so reverting isn't
>>>>>> an option.
>>>>>
>>>>> Vladimir also said that qcow2 subclusters would probably also solve his
>>>>> problem, so maybe reverting and applying the subcluster patches instead
>>>>> is a possible solution, too?
>>>>
>>>> I'm not sure about ssd case, it may need write-zero optimization anyway.
>>>
>>> What exactly do you need?  Do you actually need to write zeroes (e.g.
>>> because you’re storing images on block devices) or would it be
>>> sufficient to just drop the COW areas when bdrv_has_zero_init() and
>>> bdrv_has_zero_init_truncate() are true?
>>
>> Hmm, what do you mean? We need to zero COW areas. So, original way is to
>> write real zeroes, optimized way is fallocate.. What do you mean by drop?
>> Mark sublusters as zeroes by metadata?
> 
> Why do you need to zero COW areas?  For normal files, any data will read
> as zero if you didn’t write anything there.

Hmm, but when allocating new cluster in qcow2, it's not guaranteed to be zero,
as it may be reused previously allocated cluster..

> 
>> But still we'll have COW areas in subcluster, and we'll need to directly zero
>> them.. And fallocate will most probably be faster on ssd ext4 case..
>>
>>>
>>> I’m asking because Dave Chinner said
>>> (https://bugzilla.redhat.com/show_bug.cgi?id=1765547#c7) that
>>> fallocate() is always slow at least with aio=native because it needs to
>>> wait for all concurrent AIO writes to finish, and so it causes the AIO
>>> pipeline to stall.
>>>
>>> (He suggested using XFS extent size hints to get the same effect as
>>> write-zeroes for free, basically, but I don’t know whether that’s really
>>> useful to us; as unallocated areas on XFS read back as zero anyway.)
>>>
>>>>> We already have some cases where the existing handle_alloc_space()
>>>>> causes performance to actually become worse, and serialising requests as
>>>>> a workaround isn't going to make performance any better. So even on
>>>>> these grounds, keeping commit c8bb23cbdbe32f is questionable.
>>>>>
>>>>
>>>> Can keeping handle_alloc_space under some config option be an option?
>>>
>>> Hm.  A config option is weird if you’re the only one who’s going to
>>> enable it.  But other than that I don’t have anything against it.
>>>
>>
>> It's just a bit easier for us to maintain config option, than out-of-tree 
>> patch.
>> On the other hand, it's not a real problem to maintain this one patch in 
>> separate.
>> It may return again to the tree, when XFS bug fixed.
> 
> We’ll still have the problem that fallocate() must stall aio=native
> requests.
> 

Does it mean that fallocate is bad in general? Practice shows the opposite..
At least I have my examples with qemu-img bench. Can that thing be shown with
qemu-img bench or something?


-- 
Best regards,
Vladimir