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