block/stream:add flush l2_table_cache, ensure data integrity

[PATCH v1] block/stream:add flush l2_table_cache, ensure data integrity

Posted by Evanzhang 9 months, 1 week ago

block_stream will not actively flush l2_table_cache,when qemu
process exception exit,causing disk data loss

Signed-off-by: Evanzhang <Evanzhang@archeros.com>
---
 block/stream.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/stream.c b/block/stream.c
index e522bbd..a5e08da 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -207,6 +207,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
     }
 
+    /*
+     * Complete stream_populate,force flush l2_table_cache,to
+     * avoid unexpected termination of process, l2_table loss
+     */
+    qcow2_cache_flush(bs, ((BDRVQcow2State *)bs->opaque)->l2_table_cache);
+
     /* Do not remove the backing file if an error was there but ignored. */
     return error;
 }
-- 
2.9.5

Re: [PATCH v1] block/stream:add flush l2_table_cache,ensure data integrity

Posted by Vladimir Sementsov-Ogievskiy 9 months, 1 week ago

On 24.07.23 10:30, Evanzhang wrote:
> block_stream will not actively flush l2_table_cache,when qemu
> process exception exit,causing disk data loss
> 
> Signed-off-by: Evanzhang <Evanzhang@archeros.com>
> ---
>   block/stream.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/block/stream.c b/block/stream.c
> index e522bbd..a5e08da 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -207,6 +207,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>           }
>       }
>   
> +    /*
> +     * Complete stream_populate,force flush l2_table_cache,to
> +     * avoid unexpected termination of process, l2_table loss
> +     */
> +    qcow2_cache_flush(bs, ((BDRVQcow2State *)bs->opaque)->l2_table_cache);
> +
>       /* Do not remove the backing file if an error was there but ignored. */
>       return error;
>   }

Hi!

I think, it's more correct just call bdrv_co_flush(bs), which should do all the job. Also, stream_run() should fail if flush fails.

Also, I remember I've done it for all (or at least several) blockjobs generically, so that any blockjob must succesfully flush target to report success.. But now I can find neither my patches nor the code :( Den, Kevin, Hanna, don't you remember this topic?

-- 
Best regards,
Vladimir

Re: [PATCH v1] block/stream:add flush l2_table_cache,ensure data integrity

Posted by Denis V. Lunev 9 months, 1 week ago

On 7/25/23 16:25, Vladimir Sementsov-Ogievskiy wrote:
> On 24.07.23 10:30, Evanzhang wrote:
>> block_stream will not actively flush l2_table_cache,when qemu
>> process exception exit,causing disk data loss
>>
>> Signed-off-by: Evanzhang <Evanzhang@archeros.com>
>> ---
>>   block/stream.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index e522bbd..a5e08da 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -207,6 +207,12 @@ static int coroutine_fn stream_run(Job *job, 
>> Error **errp)
>>           }
>>       }
>>   +    /*
>> +     * Complete stream_populate,force flush l2_table_cache,to
>> +     * avoid unexpected termination of process, l2_table loss
>> +     */
>> +    qcow2_cache_flush(bs, ((BDRVQcow2State 
>> *)bs->opaque)->l2_table_cache);
>> +
>>       /* Do not remove the backing file if an error was there but 
>> ignored. */
>>       return error;
>>   }
>
> Hi!
>
> I think, it's more correct just call bdrv_co_flush(bs), which should 
> do all the job. Also, stream_run() should fail if flush fails.
>
> Also, I remember I've done it for all (or at least several) blockjobs 
> generically, so that any blockjob must succesfully flush target to 
> report success.. But now I can find neither my patches nor the code :( 
> Den, Kevin, Hanna, don't you remember this topic?
>
This was a part of compressed write cache series, which was postponed.

https://lore.kernel.org/all/20210305173507.393137-1-vsementsov@virtuozzo.com/T/#m87315593ed5ab16e5d0e4e7a5ae6d776fbbaec77

We have it ported to 7.0 QEMU.

Not a problem to port to master and resend.
Will this make a sense?

Den

Re: [PATCH v1] block/stream:add flush l2_table_cache,ensure data integrity

Posted by Vladimir Sementsov-Ogievskiy 9 months, 1 week ago

On 25.07.23 18:13, Denis V. Lunev wrote:
> On 7/25/23 16:25, Vladimir Sementsov-Ogievskiy wrote:
>> On 24.07.23 10:30, Evanzhang wrote:
>>> block_stream will not actively flush l2_table_cache,when qemu
>>> process exception exit,causing disk data loss
>>>
>>> Signed-off-by: Evanzhang <Evanzhang@archeros.com>
>>> ---
>>>   block/stream.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/block/stream.c b/block/stream.c
>>> index e522bbd..a5e08da 100644
>>> --- a/block/stream.c
>>> +++ b/block/stream.c
>>> @@ -207,6 +207,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
>>>           }
>>>       }
>>>   +    /*
>>> +     * Complete stream_populate,force flush l2_table_cache,to
>>> +     * avoid unexpected termination of process, l2_table loss
>>> +     */
>>> +    qcow2_cache_flush(bs, ((BDRVQcow2State *)bs->opaque)->l2_table_cache);
>>> +
>>>       /* Do not remove the backing file if an error was there but ignored. */
>>>       return error;
>>>   }
>>
>> Hi!
>>
>> I think, it's more correct just call bdrv_co_flush(bs), which should do all the job. Also, stream_run() should fail if flush fails.
>>
>> Also, I remember I've done it for all (or at least several) blockjobs generically, so that any blockjob must succesfully flush target to report success.. But now I can find neither my patches nor the code :( Den, Kevin, Hanna, don't you remember this topic?
>>
> This was a part of compressed write cache series, which was postponed.
> 
> https://lore.kernel.org/all/20210305173507.393137-1-vsementsov@virtuozzo.com/T/#m87315593ed5ab16e5d0e4e7a5ae6d776fbbaec77
> 
> We have it ported to 7.0 QEMU.
> 
> Not a problem to port to master and resend.
> Will this make a sense?
> 

O, thanks! Patch 01 applies with a little conflict to master, so I'll just resend it myself.

-- 
Best regards,
Vladimir

回复: [PATCH v1] block/stream:add flush l2_table_cache,ensure data integrity

Posted by 张承 9 months, 1 week ago

>On 25.07.23 18:13, Denis V. Lunev wrote:
>>On 7/25/23 16:25, Vladimir Sementsov-Ogievskiy wrote:
>>>On 24.07.23 10:30, Evanzhang wrote:
>>>> On 7/26/23 01:41, Vladimir Sementsov-Ogievskiy wrote:
>>>>block_stream will not actively flush l2_table_cache,when qemu 
>>>> process exception exit,causing disk data loss
>>>>
>>>>Signed-off-by: Evanzhang <Evanzhang@archeros.com>
>>>>---
>>>>   block/stream.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>> >diff --git a/block/stream.c b/block/stream.c index e522bbd..a5e08da 
>>>>100644
>>>> --- a/block/stream.c
>>>> +++ b/block/stream.c
>>>>@@ -207,6 +207,12 @@ static int coroutine_fn stream_run(Job *job, 
>>>> Error **errp)
>>>>           }
>>>>       }
>>>>   +    /*
>>>> +     * Complete stream_populate,force flush l2_table_cache,to
>>>> +     * avoid unexpected termination of process, l2_table loss
>>>> +     */
>>>> +    qcow2_cache_flush(bs, ((BDRVQcow2State 
>>>> +*)bs->opaque)->l2_table_cache);
>>>> +
>>>>       /* Do not remove the backing file if an error was there but 
>>>> ignored. */
>>>>       return error;
>>>>   }
>>>
>>> Hi!
>>>
>>> I think, it's more correct just call bdrv_co_flush(bs), which should do all the job. Also, stream_run() should fail if flush fails.
>>>
>>> Also, I remember I've done it for all (or at least several) blockjobs generically, so that any blockjob must succesfully flush target to report success.. But now I can find neither my patches nor the code :( Den, Kevin, Hanna, don't you remember this topic?
>>>
>> This was a part of compressed write cache series, which was postponed.
>> 
>> https://lore.kernel.org/all/20210305173507.393137-1-vsementsov@virtuoz
>> zo.com/T/#m87315593ed5ab16e5d0e4e7a5ae6d776fbbaec77
>> 
>> We have it ported to 7.0 QEMU.
>> 
>> Not a problem to port to master and resend.
>> Will this make a sense?
>> 

>O, thanks! Patch 01 applies with a little conflict to master, so I'll just resend it myself.
>

Thanks all ! 
With best regards !