[Devel] [PATCH v4 VZ9 4/5] dm-qcow2: add merge_backward set_eventfd command

[Pavel Tikhomirov]

On 3/4/25 19:41, Andrey Zhadchenko wrote:
> 
> 
> On 3/3/25 10:37, Pavel Tikhomirov wrote:
>> This eventfd can be used to get an event when merge_backward start work
>> have finished and is waiting for completion.
>>
>> Note: The eventfd can be changed even while work is running.
>>
>> Locking:
>>
>> The backward_merge.eventfd_ctx is protected from being released by
>> tgt->ctl_mutex.
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-100466
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>
>> -- 
>> v2: Always report that work finished, e.g. also on error or then it was
>> canceled, this should be more consistent from the userspace perspective.
>> v4: Address Andrey's reveiw: signal that we are at completion waiting on
>> change of eventfd.
>> ---
>>   drivers/md/dm-qcow2-cmd.c | 42 ++++++++++++++++++++++++++++++++++++++-
>>   drivers/md/dm-qcow2.h     |  2 ++
>>   2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
>> index 04a992f3ebba6..f16b4f731ca5a 100644
>> --- a/drivers/md/dm-qcow2-cmd.c
>> +++ b/drivers/md/dm-qcow2-cmd.c
>> @@ -5,6 +5,8 @@
>>   #include <linux/device-mapper.h>
>>   #include <linux/sched/signal.h>
>>   #include <linux/file.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/minmax.h>
>>   #include <linux/error-injection.h>
>>   #include "dm-qcow2.h"
>> @@ -197,6 +199,8 @@ void qcow2_merge_backward_work(struct work_struct 
>> *work)
>>       mutex_lock(&tgt->ctl_mutex);
>>       if (tgt->backward_merge.state != BACKWARD_MERGE_START) {
>> +        if (tgt->backward_merge.eventfd_ctx)
>> +            eventfd_signal(tgt->backward_merge.eventfd_ctx, 1);
>>           mutex_unlock(&tgt->ctl_mutex);
>>           return;
>>       }
>> @@ -249,6 +253,8 @@ void qcow2_merge_backward_work(struct work_struct 
>> *work)
>>           /* Finish merge */
>>           tgt->backward_merge.state = BACKWARD_MERGE_WAIT_COMPLETION;
>>       }
>> +    if (tgt->backward_merge.eventfd_ctx)
>> +        eventfd_signal(tgt->backward_merge.eventfd_ctx, 1);
> 
> It would be a bit better if we also set a different values for error or 
> success, but it is not necessary, as either complete will fail or we do 
> get_progress and see error

I don't think that it is such a good idea

1) The commit https://github.com/torvalds/linux/commit/3652117f854819a1 
removes second argument of eventfd_signal in mainstream kernel.

2) In `man 2 eventfd` examples one can see that read from eventfd reads 
the sum of all previous writes, so even if we pass something meaningful 
(n) there instead of 1, it would be hard to distinguish such a case from 
writing 1 n times.

> 
>>       mutex_unlock(&tgt->ctl_mutex);
>>   }
>> @@ -312,6 +318,27 @@ static bool 
>> qcow2_backward_merge_should_stop(struct qcow2_target *tgt)
>>       return READ_ONCE(tgt->backward_merge.state) == BACKWARD_MERGE_STOP;
>>   }
>> +#define QCOW2_FILE_UNBIND -1
>> +
>> +static int qcow2_merge_backward_set_eventfd(struct qcow2_target *tgt, 
>> int efd)
>> +{
>> +    struct eventfd_ctx *ctx = NULL;
>> +
>> +    ctx = efd == QCOW2_FILE_UNBIND ? NULL : eventfd_ctx_fdget(efd);
>> +    if (IS_ERR(ctx))
>> +        return PTR_ERR(ctx);
>> +
>> +    mutex_lock(&tgt->ctl_mutex);
>> +    swap(ctx, tgt->backward_merge.eventfd_ctx);
>> +    if (ctx)
>> +        eventfd_ctx_put(ctx);
>> +    if (tgt->backward_merge.eventfd_ctx &&
>> +        tgt->backward_merge.state == BACKWARD_MERGE_WAIT_COMPLETION)
>> +        eventfd_signal(tgt->backward_merge.eventfd_ctx, 1);
>> +    mutex_unlock(&tgt->ctl_mutex);
>> +    return 0;
>> +}
>> +
>>   static struct qcow2 *qcow2_get_img(struct qcow2_target *tgt, u32 
>> img_id, u8 *ref_index)
>>   {
>>       struct qcow2 *qcow2;
>> @@ -470,14 +497,27 @@ int qcow2_message(struct dm_target *ti, unsigned 
>> int argc, char **argv,
>>           ret = qcow2_get_event(tgt, result, maxlen);
>>           goto out;
>>       } else if (!strcmp(argv[0], "merge_backward")) {
>> -        if (argc != 2) {
>> +        if (argc < 2) {
>>               ret = -EINVAL;
>>               goto out;
>>           }
>>           if (!strcmp(argv[1], "cancel")) {
>> +            if (argc != 2) {
>> +                ret = -EINVAL;
>> +                goto out;
>> +            }
>>               qcow2_merge_backward_cancel(tgt);
>>               ret = 0;
>>               goto out;
>> +        } else if (!strcmp(argv[1], "set_eventfd")) {
>> +            int efd;
>> +
>> +            if (argc != 3 || kstrtoint(argv[2], 10, &efd)) {
>> +                ret = -EINVAL;
>> +                goto out;
>> +            }
>> +            ret = qcow2_merge_backward_set_eventfd(tgt, efd);
>> +            goto out;
>>           }
>>       }
>> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
>> index bebfdc50ed6d4..c4956e3fd0eb7 100644
>> --- a/drivers/md/dm-qcow2.h
>> +++ b/drivers/md/dm-qcow2.h
>> @@ -5,6 +5,7 @@
>>   #include <linux/percpu-refcount.h>
>>   #include <linux/device-mapper.h>
>>   #include <linux/fs.h>
>> +#include <linux/eventfd.h>
>>   #include "dm-core.h"
>>   #define DM_MSG_PREFIX "qcow2"
>> @@ -161,6 +162,7 @@ struct qcow2_backward_merge {
>>       struct work_struct work;
>>       enum qcow2_backward_merge_state state;
>>       int error;
>> +    struct eventfd_ctx *eventfd_ctx;
>>   };
>>   struct qcow2_target {
> 

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.