Re: [Qemu-devel] [PATCH] job: drop job_drain

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

16.08.2019 20:10, Vladimir Sementsov-Ogievskiy wrote:
> 16.08.2019 20:04, Vladimir Sementsov-Ogievskiy wrote:
>> In job_finish_sync job_enter should be enough for a job to make some
>> progress and draining is a wrong tool for it. So use job_enter directly
>> here and drop job_drain with all related staff not used more.
>>
>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> It's a continuation for
>>     [PATCH v4] blockjob: drain all job nodes in block_job_drain
>>
>>   include/block/blockjob_int.h | 19 -------------------
>>   include/qemu/job.h           | 13 -------------
>>   block/backup.c               | 19 +------------------
>>   block/commit.c               |  1 -
>>   block/mirror.c               | 28 +++-------------------------
>>   block/stream.c               |  1 -
>>   blockjob.c                   | 13 -------------
>>   job.c                        | 12 +-----------
>>   8 files changed, 5 insertions(+), 101 deletions(-)
>>
>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>> index e4a318dd15..e2824a36a8 100644
>> --- a/include/block/blockjob_int.h
>> +++ b/include/block/blockjob_int.h
>> @@ -52,17 +52,6 @@ struct BlockJobDriver {
>>        * besides job->blk to the new AioContext.
>>        */
>>       void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
>> -
>> -    /*
>> -     * If the callback is not NULL, it will be invoked when the job has to be
>> -     * synchronously cancelled or completed; it should drain BlockDriverStates
>> -     * as required to ensure progress.
>> -     *
>> -     * Block jobs must use the default implementation for job_driver.drain,
>> -     * which will in turn call this callback after doing generic block job
>> -     * stuff.
>> -     */
>> -    void (*drain)(BlockJob *job);
>>   };
>>   /**
>> @@ -107,14 +96,6 @@ void block_job_free(Job *job);
>>    */
>>   void block_job_user_resume(Job *job);
>> -/**
>> - * block_job_drain:
>> - * Callback to be used for JobDriver.drain in all block jobs. Drains the main
>> - * block node associated with the block jobs and calls BlockJobDriver.drain for
>> - * job-specific actions.
>> - */
>> -void block_job_drain(Job *job);
>> -
>>   /**
>>    * block_job_ratelimit_get_delay:
>>    *
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 9e7cd1e4a0..09739b8dd9 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -220,13 +220,6 @@ struct JobDriver {
>>        */
>>       void (*complete)(Job *job, Error **errp);
>> -    /*
>> -     * If the callback is not NULL, it will be invoked when the job has to be
>> -     * synchronously cancelled or completed; it should drain any activities
>> -     * as required to ensure progress.
>> -     */
>> -    void (*drain)(Job *job);
>> -
>>       /**
>>        * If the callback is not NULL, prepare will be invoked when all the jobs
>>        * belonging to the same transaction complete; or upon this job's completion
>> @@ -470,12 +463,6 @@ bool job_user_paused(Job *job);
>>    */
>>   void job_user_resume(Job *job, Error **errp);
>> -/*
>> - * Drain any activities as required to ensure progress. This can be called in a
>> - * loop to synchronously complete a job.
>> - */
>> -void job_drain(Job *job);
>> -
>>   /**
>>    * Get the next element from the list of block jobs after @job, or the
>>    * first one if @job is %NULL.
>> diff --git a/block/backup.c b/block/backup.c
>> index 715e1d3be8..d1ecdfa9aa 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -320,21 +320,6 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
>>       hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
>>   }
>> -static void backup_drain(BlockJob *job)
>> -{
>> -    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
>> -
>> -    /* Need to keep a reference in case blk_drain triggers execution
>> -     * of backup_complete...
>> -     */
>> -    if (s->target) {
>> -        BlockBackend *target = s->target;
>> -        blk_ref(target);
>> -        blk_drain(target);
>> -        blk_unref(target);
>> -    }
>> -}
>> -
>>   static BlockErrorAction backup_error_action(BackupBlockJob *job,
>>                                               bool read, int error)
>>   {
>> @@ -488,13 +473,11 @@ static const BlockJobDriver backup_job_driver = {
>>           .job_type               = JOB_TYPE_BACKUP,
>>           .free                   = block_job_free,
>>           .user_resume            = block_job_user_resume,
>> -        .drain                  = block_job_drain,
>>           .run                    = backup_run,
>>           .commit                 = backup_commit,
>>           .abort                  = backup_abort,
>>           .clean                  = backup_clean,
>> -    },
>> -    .drain                  = backup_drain,
>> +    }
>>   };
>>   static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>> diff --git a/block/commit.c b/block/commit.c
>> index 2c5a6d4ebc..697a779d8e 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -216,7 +216,6 @@ static const BlockJobDriver commit_job_driver = {
>>           .job_type      = JOB_TYPE_COMMIT,
>>           .free          = block_job_free,
>>           .user_resume   = block_job_user_resume,
>> -        .drain         = block_job_drain,
>>           .run           = commit_run,
>>           .prepare       = commit_prepare,
>>           .abort         = commit_abort,
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 8cb75fb409..b91abe0288 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -644,14 +644,11 @@ static int mirror_exit_common(Job *job)
>>       bdrv_ref(mirror_top_bs);
>>       bdrv_ref(target_bs);
>> -    /* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
>> +    /*
>> +     * Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
>>        * inserting target_bs at s->to_replace, where we might not be able to get
>>        * these permissions.
>> -     *
>> -     * Note that blk_unref() alone doesn't necessarily drop permissions because
>> -     * we might be running nested inside mirror_drain(), which takes an extra
>> -     * reference, so use an explicit blk_set_perm() first. */
>> -    blk_set_perm(s->target, 0, BLK_PERM_ALL, &error_abort);
>> +     */
> 
> Somehow this hunk lives here from the very beginning of this patch versioning,
> but nobody noticed it. I'll resend.

Ah no, it's OK, as mirror_drain don't exists now.


-- 
Best regards,
Vladimir