diff mbox series

[5/7] blockdev: refactor block-dirty-bitmap-clear transaction

Message ID 20180326112056.8420-6-vsementsov@virtuozzo.com
State New
Headers show
Series Dirty bitmaps fixing and refactoring | expand

Commit Message

Vladimir Sementsov-Ogievskiy March 26, 2018, 11:20 a.m. UTC 
bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
commit, avoiding any rollback.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

Comments

Paolo Bonzini March 26, 2018, 11:44 a.m. UTC | #1 
On 26/03/2018 13:20, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
> commit, avoiding any rollback.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  blockdev.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index c31bf3d98d..88eae60c1c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>      BlkActionState common;
>      BdrvDirtyBitmap *bitmap;
>      BlockDriverState *bs;
> -    HBitmap *backup;
>      bool prepared;
>  } BlockDirtyBitmapState;
>  
> @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>          error_setg(errp, "Cannot clear a readonly bitmap");
>          return;
>      }
> -
> -    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
> -}
> -
> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
> -{
> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> -                                             common, common);
> -
> -    if (state->backup) {
> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
> -    }

Isn't bdrv_undo_clear_dirty_bitmap new unused?

Thanks,

Paolo

>  }
>  
>  static void block_dirty_bitmap_clear_commit(BlkActionState *common)
> @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
>  
> -    hbitmap_free(state->backup);
> +    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_clear_prepare,
>          .commit = block_dirty_bitmap_clear_commit,
> -        .abort = block_dirty_bitmap_clear_abort,
>      }
>  };
>  
>
Vladimir Sementsov-Ogievskiy March 26, 2018, 11:48 a.m. UTC | #2 
26.03.2018 14:44, Paolo Bonzini wrote:
> On 26/03/2018 13:20, Vladimir Sementsov-Ogievskiy wrote:
>> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
>> commit, avoiding any rollback.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   blockdev.c | 16 +---------------
>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index c31bf3d98d..88eae60c1c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>>       BlkActionState common;
>>       BdrvDirtyBitmap *bitmap;
>>       BlockDriverState *bs;
>> -    HBitmap *backup;
>>       bool prepared;
>>   } BlockDirtyBitmapState;
>>   
>> @@ -2129,18 +2128,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>           error_setg(errp, "Cannot clear a readonly bitmap");
>>           return;
>>       }
>> -
>> -    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
>> -}
>> -
>> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
>> -{
>> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> -                                             common, common);
>> -
>> -    if (state->backup) {
>> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
>> -    }
> Isn't bdrv_undo_clear_dirty_bitmap new unused?

hmm, yes. I'll remove it in v2.

>
> Thanks,
>
> Paolo
>
>>   }
>>   
>>   static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>> @@ -2148,7 +2135,7 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>>       BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>                                                common, common);
>>   
>> -    hbitmap_free(state->backup);
>> +    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>>   }
>>   
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>>           .instance_size = sizeof(BlockDirtyBitmapState),
>>           .prepare = block_dirty_bitmap_clear_prepare,
>>           .commit = block_dirty_bitmap_clear_commit,
>> -        .abort = block_dirty_bitmap_clear_abort,
>>       }
>>   };
>>   
>>
Vladimir Sementsov-Ogievskiy March 26, 2018, 11:53 a.m. UTC | #3 
26.03.2018 14:48, Vladimir Sementsov-Ogievskiy wrote:
> 26.03.2018 14:44, Paolo Bonzini wrote:
>> On 26/03/2018 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>> bdrv_clear_dirty_bitmap do not fail, so we can call it in transaction
>>> commit, avoiding any rollback.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   blockdev.c | 16 +---------------
>>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index c31bf3d98d..88eae60c1c 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2050,7 +2050,6 @@ typedef struct BlockDirtyBitmapState {
>>>       BlkActionState common;
>>>       BdrvDirtyBitmap *bitmap;
>>>       BlockDriverState *bs;
>>> -    HBitmap *backup;
>>>       bool prepared;
>>>   } BlockDirtyBitmapState;
>>>   @@ -2129,18 +2128,6 @@ static void 
>>> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>>           error_setg(errp, "Cannot clear a readonly bitmap");
>>>           return;
>>>       }
>>> -
>>> -    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
>>> -}
>>> -
>>> -static void block_dirty_bitmap_clear_abort(BlkActionState *common)
>>> -{
>>> -    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>> -                                             common, common);
>>> -
>>> -    if (state->backup) {
>>> -        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
>>> -    }
>> Isn't bdrv_undo_clear_dirty_bitmap new unused?
>
> hmm, yes. I'll remove it in v2.

or, here, as 5.5/7

>
>>
>> Thanks,
>>
>> Paolo
>>
>>>   }
>>>     static void block_dirty_bitmap_clear_commit(BlkActionState *common)
>>> @@ -2148,7 +2135,7 @@ static void 
>>> block_dirty_bitmap_clear_commit(BlkActionState *common)
>>>       BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>>                                                common, common);
>>>   -    hbitmap_free(state->backup);
>>> +    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
>>>   }
>>>     static void abort_prepare(BlkActionState *common, Error **errp)
>>> @@ -2210,7 +2197,6 @@ static const BlkActionOps actions[] = {
>>>           .instance_size = sizeof(BlockDirtyBitmapState),
>>>           .prepare = block_dirty_bitmap_clear_prepare,
>>>           .commit = block_dirty_bitmap_clear_commit,
>>> -        .abort = block_dirty_bitmap_clear_abort,
>>>       }
>>>   };
>>>
>
>
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index c31bf3d98d..88eae60c1c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2050,7 +2050,6 @@  typedef struct BlockDirtyBitmapState {
     BlkActionState common;
     BdrvDirtyBitmap *bitmap;
     BlockDriverState *bs;
-    HBitmap *backup;
     bool prepared;
 } BlockDirtyBitmapState;
 
@@ -2129,18 +2128,6 @@  static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         error_setg(errp, "Cannot clear a readonly bitmap");
         return;
     }
-
-    bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
-}
-
-static void block_dirty_bitmap_clear_abort(BlkActionState *common)
-{
-    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
-                                             common, common);
-
-    if (state->backup) {
-        bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
-    }
 }
 
 static void block_dirty_bitmap_clear_commit(BlkActionState *common)
@@ -2148,7 +2135,7 @@  static void block_dirty_bitmap_clear_commit(BlkActionState *common)
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
 
-    hbitmap_free(state->backup);
+    bdrv_clear_dirty_bitmap(state->bitmap, NULL);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2210,7 +2197,6 @@  static const BlkActionOps actions[] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_clear_prepare,
         .commit = block_dirty_bitmap_clear_commit,
-        .abort = block_dirty_bitmap_clear_abort,
     }
 };