On 17.02.2016 16:51, Kevin Wolf wrote:
> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>> Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>> block.c | 20 ------------
>> block/block-backend.c | 44
>> +++++++++++++++++++++++----
>> block/io.c | 20 ------------
>> include/block/block.h | 2 --
>> stubs/Makefile.objs | 2 +-
>> stubs/{bdrv-commit-all.c => blk-commit-all.c} | 4 +--
>> 6 files changed, 41 insertions(+), 51 deletions(-)
>> rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)
>>
[...]
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index a10fe44..a918c35 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
[...]
>> @@ -1361,5 +1356,42 @@ BlockBackendRootState
>> *blk_get_root_state(BlockBackend *blk)
>>
>> int blk_commit_all(void)
>> {
>> - return bdrv_commit_all();
>> + BlockBackend *blk = NULL;
>> +
>> + while ((blk = blk_all_next(blk)) != NULL) {
>> + AioContext *aio_context = blk_get_aio_context(blk);
>> +
>> + aio_context_acquire(aio_context);
>> + if (blk_is_available(blk) && blk->bs->drv && blk->bs->backing) {
>
> blk->bs->drv is already implied by blk_is_available(), so it's
> unnecessary, even though it doesn't actively hurt.
>
> Also, using blk_is_available() instead of blk_is_inserted() as in
> blk_flush_all() means that a drive with an open tray, but inserted
> medium isn't committed. I assume you did this on purpose because you're
> using two different functions in this patch, but isn't this a change in
> behaviour? If so, and we really want it, it should at least be mentioned
> in the commit message.
Drives with open trays are strange.
I found it reasonable that every medium in the system should be flushed
on blk_flush_all(), because flushing data should only bring the on-disk
state to a state it is supposed to be in anyway.
On the other hand, the commit operation actively changes data.
Therefore, I decided to treat it like a guest write operation. However,
considering that blk_commit_all() is basically only available through
HMP and is thus more of a legacy operation, I don't think its behavior
should be changed here. I'm therefore going to change this to
blk_is_inserted(), thanks.
Max
signature.asc
Description: OpenPGP digital signature
