[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/5] block, migration: add bdrv_flush_vmstate helper
From: |
Denis V. Lunev |
Subject: |
Re: [PATCH 4/5] block, migration: add bdrv_flush_vmstate helper |
Date: |
Wed, 17 Jun 2020 00:29:40 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
On 6/17/20 12:11 AM, Eric Blake wrote:
> On 6/16/20 11:20 AM, Denis V. Lunev wrote:
>> Right now bdrv_fclose() is just calling bdrv_flush().
>>
>> The problem is that migration code is working inefficently from black
>
> inefficiently, block
>
>> layer terms and are frequently called for very small pieces of not
>> properly aligned data. Block layer is capable to work this way, but
>
> s/not properly aligned/unaligned/
>
>> this is very slow.
>>
>> This patch is a preparation for the introduction of the intermediate
>> buffer at block driver state. It would be beneficial to separate
>> conventional bdrv_flush() from closing QEMU file from migration code.
>>
>> The patch also forces bdrv_flush_vmstate() operation inside
>> synchronous blk_save_vmstate() operation. This helper is used from
>> qemu-io only.
>>
>
>> +++ b/block/block-backend.c
>> @@ -2177,16 +2177,20 @@ int blk_truncate(BlockBackend *blk, int64_t
>> offset, bool exact,
>> int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
>> int64_t pos, int size)
>> {
>> - int ret;
>> + int ret, ret2;
>> if (!blk_is_available(blk)) {
>> return -ENOMEDIUM;
>> }
>> ret = bdrv_save_vmstate(blk_bs(blk), buf, pos, size);
>> + ret2 = bdrv_flush_vmstate(blk_bs(blk));
>
> Do you really want to be attempting bdrv_flush_vmstate() even after
> bdrv_save_vmstate() failed? Better might be...
>
>> if (ret < 0) {
>> return ret;
>> }
>
> ...attempting it here, at which point it looks like the only reason
> you need ret2 is to preserve ret long enough...
no, I would like to be sure that intermediate state is always cleared at
the end.
In the next patch I am going to put intermediate buffer to
BlockDriverState and thus it has to be removed in any case.
May be flush would be a bad name... clean or finalize?
>
>> + if (ret2 < 0) {
>> + return ret2;
>> + }
>> if (ret == size && !blk->enable_write_cache) {
>
> ...for this check. But a quick look at bdrv_save_vmstate() says this
> check is dead: the function can only return negative error or exactly
> size, and we already filtered out negative error above.
>
>
hmm. you are right. good point. this check is dead.
Worth to remove :) I'll add this as a separate patch.
Den
- [PATCH v4 0/4] block: seriously improve savevm performance, Denis V. Lunev, 2020/06/16
- [PATCH 1/5] migration/savevm: respect qemu_fclose() error code in save_snapshot(), Denis V. Lunev, 2020/06/16
- [PATCH 3/5] block/aio_task: drop aio_task_pool_wait_one() helper, Denis V. Lunev, 2020/06/16
- [PATCH 2/5] block/aio_task: allow start/wait task from any coroutine, Denis V. Lunev, 2020/06/16
- [PATCH 4/5] block, migration: add bdrv_flush_vmstate helper, Denis V. Lunev, 2020/06/16
- Re: [PATCH 4/5] block, migration: add bdrv_flush_vmstate helper, Vladimir Sementsov-Ogievskiy, 2020/06/18
- [PATCH 5/5] block/io: improve savevm performance, Denis V. Lunev, 2020/06/16
- Re: [PATCH v4 0/4] block: seriously improve savevm performance, no-reply, 2020/06/16