[Devel] [PATCH v4 VZ9 3/5] dm-qcow2: make merge_backward command asyncronous

[Alexander Atanasov]

On 5.03.25 5:53, Pavel Tikhomirov wrote:
> 
>>> @@ -63,7 +65,7 @@ static int qcow2_service_iter(struct qcow2_target 
>>> *tgt, struct qcow2 *qcow2,
>>>       WRITE_ONCE(service_status, BLK_STS_OK);
>>>       for (pos = 0; pos < end; pos += step) {
>>> -        if (fatal_signal_pending(current)) {
>>> +        if (qcow2_backward_merge_should_stop(tgt)) {
>>>               ret = -EINTR;
>>>               break;
>>>           }
>>
>> Is it okay to remove termination on signal - here and the killable 
>> mutex? Without signal handling it can prevent clean shutdown or leave it
>> stuck if something goes wrong in the code.
> 
> Sorry, I should've probably explained it in commit message as a note.
> 
> Yes, it's ok, and it is intentional:
> 
> 1) Now this code always runs in workqueue, so we change the way how it 
> can be interrupted to checking the state variable. Sending signal to 
> workqueue worker thread does not have much sense as when signal comes 
> worker may have already switched to execute different work.
> 
> 2) Now the ctl_mutex is not held for a long time anymore, so there is no 
> point in taking it with interruptible primitive as we won't wait long 
> for mutex.

Ok. Makes sense. Note - If merge can not be stopped with SIGKILL, then 
stop/cancelation should be implemented somewhere in userspace unless 
that change of behaviour is acceptable.

> 
>>> @@ -494,6 +497,9 @@ static struct qcow2_target 
>>> *alloc_qcow2_target(struct dm_target *ti)
>>>       timer_setup(&tgt->enospc_timer, qcow2_enospc_timer, 0);
>>>       ti->private = tgt;
>>>       tgt->ti = ti;
>>> +
>>> +    INIT_WORK(&tgt->backward_merge.work, qcow2_merge_backward_work);
>>> +
>>>       qcow2_set_service_operations(ti, false);
>>>       return tgt;
>>> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
>>> index a89fe3db2196d..bebfdc50ed6d4 100644
>>> --- a/drivers/md/dm-qcow2.h
>>> +++ b/drivers/md/dm-qcow2.h
>>> @@ -149,6 +149,20 @@ struct md_page {
>>>       struct list_head wpc_readers_wait_list;
>>>   };
>>> +enum qcow2_backward_merge_state {
>>> +    BACKWARD_MERGE_STOPPED = 0,
>>
>> nit: this init is excess
> 
> Agreed. I will remove it here.
> 
> Note: that in case of same thing in qcow2_backward_merge_stage it is 
> intentional to identify that the enum values are used as an array index, 
> and starting from 0 is important.

Agree. for array it can used as a hint to preserve specific values 
and/o/ order when it is changed and it is required. Either zero init or 
a comment can be used.



-- 
Regards,
Alexander Atanasov