[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 6/7] blockjobs: split interface into public/priv
From: |
Jeff Cody |
Subject: |
Re: [Qemu-block] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 |
Date: |
Wed, 26 Oct 2016 00:51:29 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Thu, Oct 13, 2016 at 06:57:01PM -0400, John Snow wrote:
> To make it a little more obvious which functions are intended to be
> public interface and which are intended to be for use only by jobs
> themselves, split the interface into "public" and "private" files.
>
> Convert blockjobs (e.g. block/backup) to using the private interface.
> Leave blockdev and others on the public interface.
>
> There are remaining uses of private state by qemu-img, and several
> cases in blockdev.c and block/io.c where we grab job->blk for the
> purposes of acquiring an AIOContext.
>
> These will be corrected in future patches.
>
> Signed-off-by: John Snow <address@hidden>
> ---
> block/backup.c | 2 +-
> block/commit.c | 2 +-
> block/mirror.c | 2 +-
> block/stream.c | 2 +-
> blockjob.c | 2 +-
> include/block/block.h | 3 +-
> include/block/blockjob.h | 205 +-------------------------------------
> include/block/blockjob_int.h | 232
> +++++++++++++++++++++++++++++++++++++++++++
> tests/test-blockjob-txn.c | 2 +-
> tests/test-blockjob.c | 2 +-
> 10 files changed, 244 insertions(+), 210 deletions(-)
> create mode 100644 include/block/blockjob_int.h
>
> diff --git a/block/backup.c b/block/backup.c
> index 6a60ca8..6d12100 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -16,7 +16,7 @@
> #include "trace.h"
> #include "block/block.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_backup.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> diff --git a/block/commit.c b/block/commit.c
> index 475a375..d555600 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -15,7 +15,7 @@
> #include "qemu/osdep.h"
> #include "trace.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/ratelimit.h"
> diff --git a/block/mirror.c b/block/mirror.c
> index 4374fb4..c81b5e0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -13,7 +13,7 @@
>
> #include "qemu/osdep.h"
> #include "trace.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_int.h"
> #include "sysemu/block-backend.h"
> #include "qapi/error.h"
> diff --git a/block/stream.c b/block/stream.c
> index 7d6877d..906f7f3 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -14,7 +14,7 @@
> #include "qemu/osdep.h"
> #include "trace.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/ratelimit.h"
> diff --git a/blockjob.c b/blockjob.c
> index d118a1f..e6f0d97 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -27,7 +27,7 @@
> #include "qemu-common.h"
> #include "trace.h"
> #include "block/block.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_int.h"
> #include "sysemu/block-backend.h"
> #include "qapi/qmp/qerror.h"
> diff --git a/include/block/block.h b/include/block/block.h
> index 107c603..89b5feb 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -7,16 +7,15 @@
> #include "qemu/coroutine.h"
> #include "block/accounting.h"
> #include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
> #include "qapi/qmp/qobject.h"
> #include "qapi-types.h"
> #include "qemu/hbitmap.h"
>
> /* block.c */
> typedef struct BlockDriver BlockDriver;
> -typedef struct BlockJob BlockJob;
> typedef struct BdrvChild BdrvChild;
> typedef struct BdrvChildRole BdrvChildRole;
> -typedef struct BlockJobTxn BlockJobTxn;
>
> typedef struct BlockDriverInfo {
> /* in bytes, 0 if irrelevant */
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 5b61140..bfc8233 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -28,78 +28,15 @@
>
> #include "block/block.h"
>
> -/**
> - * BlockJobDriver:
> - *
> - * A class type for block job driver.
> - */
> -typedef struct BlockJobDriver {
> - /** Derived BlockJob struct size */
> - size_t instance_size;
> -
> - /** String describing the operation, part of query-block-jobs QMP API */
> - BlockJobType job_type;
> -
> - /** Optional callback for job types that support setting a speed limit */
> - void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> -
> - /** Optional callback for job types that need to forward I/O status
> reset */
> - void (*iostatus_reset)(BlockJob *job);
> -
> - /**
> - * Optional callback for job types whose completion must be triggered
> - * manually.
> - */
> - void (*complete)(BlockJob *job, Error **errp);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when all the jobs
> - * belonging to the same transaction complete; or upon this job's
> - * completion if it is not in a transaction. Skipped if NULL.
> - *
> - * All jobs will complete with a call to either .commit() or .abort() but
> - * never both.
> - */
> - void (*commit)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when any job in the
> - * same transaction fails; or upon this job's failure (due to error or
> - * cancellation) if it is not in a transaction. Skipped if NULL.
> - *
> - * All jobs will complete with a call to either .commit() or .abort() but
> - * never both.
> - */
> - void (*abort)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when the job
> transitions
> - * into the paused state. Paused jobs must not perform any asynchronous
> - * I/O or event loop activity. This callback is used to quiesce jobs.
> - */
> - void coroutine_fn (*pause)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when the job
> transitions
> - * out of the paused state. Any asynchronous I/O or event loop activity
> - * should be restarted from this callback.
> - */
> - void coroutine_fn (*resume)(BlockJob *job);
> -
> - /*
> - * If the callback is not NULL, it will be invoked before the job is
> - * resumed in a new AioContext. This is the place to move any resources
> - * besides job->blk to the new AioContext.
> - */
> - void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> -} BlockJobDriver;
> +typedef struct BlockJobDriver BlockJobDriver;
> +typedef struct BlockJobTxn BlockJobTxn;
>
> /**
> * BlockJob:
> *
> * Long-running operation on a BlockDriverState.
> */
> -struct BlockJob {
> +typedef struct BlockJob {
> /** The job type, including the job vtable. */
> const BlockJobDriver *driver;
>
> @@ -198,7 +135,7 @@ struct BlockJob {
> /** Non-NULL if this job is part of a transaction */
> BlockJobTxn *txn;
> QLIST_ENTRY(BlockJob) txn_list;
> -};
> +} BlockJob;
>
> typedef enum BlockJobCreateFlags {
> BLOCK_JOB_DEFAULT = 0x00,
> @@ -227,76 +164,6 @@ BlockJob *block_job_next(BlockJob *job);
> BlockJob *block_job_get(const char *id);
>
> /**
> - * block_job_create:
> - * @job_id: The id of the newly-created job, or %NULL to have one
> - * generated automatically.
> - * @job_type: The class object for the newly-created job.
> - * @bs: The block
> - * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> - * @errp: Error object.
> - *
> - * Create a new long-running block device job and return it. The job
> - * will call @cb asynchronously when the job completes. Note that
> - * @bs may have been closed at the time the @cb it is called. If
> - * this is the case, the job may be reported as either cancelled or
> - * completed.
> - *
> - * This function is not part of the public job interface; it should be
> - * called from a wrapper that is specific to the job type.
> - */
> -void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> - BlockDriverState *bs, int64_t speed, int flags,
> - BlockCompletionFunc *cb, void *opaque, Error **errp);
> -
> -/**
> - * block_job_sleep_ns:
> - * @job: The job that calls the function.
> - * @clock: The clock to sleep on.
> - * @ns: How many nanoseconds to stop for.
> - *
> - * Put the job to sleep (assuming that it wasn't canceled) for @ns
> - * nanoseconds. Canceling the job will interrupt the wait immediately.
> - */
> -void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> -
> -/**
> - * block_job_yield:
> - * @job: The job that calls the function.
> - *
> - * Yield the block job coroutine.
> - */
> -void block_job_yield(BlockJob *job);
> -
> -/**
> - * block_job_ref:
> - * @bs: The block device.
> - *
> - * Grab a reference to the block job. Should be paired with block_job_unref.
> - */
> -void block_job_ref(BlockJob *job);
> -
> -/**
> - * block_job_unref:
> - * @bs: The block device.
> - *
> - * Release reference to the block job and release resources if it is the last
> - * reference.
> - */
> -void block_job_unref(BlockJob *job);
> -
> -/**
> - * block_job_completed:
> - * @job: The job being completed.
> - * @ret: The status code.
> - *
> - * Call the completion function that was registered at creation time, and
> - * free @job.
> - */
> -void block_job_completed(BlockJob *job, int ret);
> -
> -/**
> * block_job_set_speed:
> * @job: The job to set the speed for.
> * @speed: The new value
> @@ -325,14 +192,6 @@ void block_job_cancel(BlockJob *job);
> void block_job_complete(BlockJob *job, Error **errp);
>
> /**
> - * block_job_is_cancelled:
> - * @job: The job being queried.
> - *
> - * Returns whether the job is scheduled for cancellation.
> - */
> -bool block_job_is_cancelled(BlockJob *job);
> -
> -/**
> * block_job_query:
> * @job: The job to get information about.
> *
> @@ -341,15 +200,6 @@ bool block_job_is_cancelled(BlockJob *job);
> BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>
> /**
> - * block_job_pause_point:
> - * @job: The job that is ready to pause.
> - *
> - * Pause now if block_job_pause() has been called. Block jobs that perform
> - * lots of I/O must call this between requests so that the job can be paused.
> - */
> -void coroutine_fn block_job_pause_point(BlockJob *job);
> -
> -/**
> * block_job_pause:
> * @job: The job to be paused.
> *
> @@ -392,22 +242,6 @@ void block_job_resume(BlockJob *job);
> void block_job_user_resume(BlockJob *job);
>
> /**
> - * block_job_enter:
> - * @job: The job to enter.
> - *
> - * Continue the specified job by entering the coroutine.
> - */
> -void block_job_enter(BlockJob *job);
> -
> -/**
> - * block_job_ready:
> - * @job: The job which is now ready to complete.
> - *
> - * Send a BLOCK_JOB_READY event for the specified job.
> - */
> -void block_job_event_ready(BlockJob *job);
> -
> -/**
> * block_job_cancel_sync:
> * @job: The job to be canceled.
> *
> @@ -453,37 +287,6 @@ int block_job_complete_sync(BlockJob *job, Error **errp);
> void block_job_iostatus_reset(BlockJob *job);
>
> /**
> - * block_job_error_action:
> - * @job: The job to signal an error for.
> - * @on_err: The error action setting.
> - * @is_read: Whether the operation was a read.
> - * @error: The error that was reported.
> - *
> - * Report an I/O error for a block job and possibly stop the VM. Return the
> - * action that was selected based on @on_err and @error.
> - */
> -BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError
> on_err,
> - int is_read, int error);
> -
> -typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> -
> -/**
> - * block_job_defer_to_main_loop:
> - * @job: The job
> - * @fn: The function to run in the main loop
> - * @opaque: The opaque value that is passed to @fn
> - *
> - * Execute a given function in the main loop with the BlockDriverState
> - * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
> - * anything that uses bdrv_drain_all() in the main loop.
> - *
> - * The @job AioContext is held while @fn executes.
> - */
> -void block_job_defer_to_main_loop(BlockJob *job,
> - BlockJobDeferToMainLoopFn *fn,
> - void *opaque);
> -
> -/**
> * block_job_txn_new:
> *
> * Allocate and return a new block job transaction. Jobs can be added to the
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> new file mode 100644
> index 0000000..8eced19
> --- /dev/null
> +++ b/include/block/blockjob_int.h
> @@ -0,0 +1,232 @@
> +/*
> + * Declarations for long-running block device operations
> + *
> + * Copyright (c) 2011 IBM Corp.
> + * Copyright (c) 2012 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef BLOCKJOB_INT_H
> +#define BLOCKJOB_INT_H
> +
> +#include "block/blockjob.h"
> +#include "block/block.h"
> +
> +/**
> + * BlockJobDriver:
> + *
> + * A class type for block job driver.
> + */
> +struct BlockJobDriver {
> + /** Derived BlockJob struct size */
> + size_t instance_size;
> +
> + /** String describing the operation, part of query-block-jobs QMP API */
> + BlockJobType job_type;
> +
> + /** Optional callback for job types that support setting a speed limit */
> + void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> +
> + /** Optional callback for job types that need to forward I/O status
> reset */
> + void (*iostatus_reset)(BlockJob *job);
> +
> + /**
> + * Optional callback for job types whose completion must be triggered
> + * manually.
> + */
> + void (*complete)(BlockJob *job, Error **errp);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when all the jobs
> + * belonging to the same transaction complete; or upon this job's
> + * completion if it is not in a transaction. Skipped if NULL.
> + *
> + * All jobs will complete with a call to either .commit() or .abort() but
> + * never both.
> + */
> + void (*commit)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when any job in the
> + * same transaction fails; or upon this job's failure (due to error or
> + * cancellation) if it is not in a transaction. Skipped if NULL.
> + *
> + * All jobs will complete with a call to either .commit() or .abort() but
> + * never both.
> + */
> + void (*abort)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when the job
> transitions
> + * into the paused state. Paused jobs must not perform any asynchronous
> + * I/O or event loop activity. This callback is used to quiesce jobs.
> + */
> + void coroutine_fn (*pause)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when the job
> transitions
> + * out of the paused state. Any asynchronous I/O or event loop activity
> + * should be restarted from this callback.
> + */
> + void coroutine_fn (*resume)(BlockJob *job);
> +
> + /*
> + * If the callback is not NULL, it will be invoked before the job is
> + * resumed in a new AioContext. This is the place to move any resources
> + * besides job->blk to the new AioContext.
> + */
> + void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> +};
> +
> +/**
> + * block_job_create:
> + * @job_id: The id of the newly-created job, or %NULL to have one
> + * generated automatically.
> + * @job_type: The class object for the newly-created job.
> + * @bs: The block
> + * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @cb: Completion function for the job.
> + * @opaque: Opaque pointer value passed to @cb.
> + * @errp: Error object.
> + *
> + * Create a new long-running block device job and return it. The job
> + * will call @cb asynchronously when the job completes. Note that
> + * @bs may have been closed at the time the @cb it is called. If
> + * this is the case, the job may be reported as either cancelled or
> + * completed.
> + *
> + * This function is not part of the public job interface; it should be
> + * called from a wrapper that is specific to the job type.
> + */
> +void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> + BlockDriverState *bs, int64_t speed, int flags,
> + BlockCompletionFunc *cb, void *opaque, Error **errp);
> +
> +/**
> + * block_job_sleep_ns:
> + * @job: The job that calls the function.
> + * @clock: The clock to sleep on.
> + * @ns: How many nanoseconds to stop for.
> + *
> + * Put the job to sleep (assuming that it wasn't canceled) for @ns
> + * nanoseconds. Canceling the job will interrupt the wait immediately.
> + */
> +void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> +
> +/**
> + * block_job_yield:
> + * @job: The job that calls the function.
> + *
> + * Yield the block job coroutine.
> + */
> +void block_job_yield(BlockJob *job);
> +
> +/**
> + * block_job_ref:
> + * @bs: The block device.
> + *
> + * Grab a reference to the block job. Should be paired with block_job_unref.
> + */
> +void block_job_ref(BlockJob *job);
> +
> +/**
> + * block_job_unref:
> + * @bs: The block device.
> + *
> + * Release reference to the block job and release resources if it is the last
> + * reference.
> + */
> +void block_job_unref(BlockJob *job);
> +
> +/**
> + * block_job_completed:
> + * @job: The job being completed.
> + * @ret: The status code.
> + *
> + * Call the completion function that was registered at creation time, and
> + * free @job.
> + */
> +void block_job_completed(BlockJob *job, int ret);
> +
> +/**
> + * block_job_is_cancelled:
> + * @job: The job being queried.
> + *
> + * Returns whether the job is scheduled for cancellation.
> + */
> +bool block_job_is_cancelled(BlockJob *job);
> +
> +/**
> + * block_job_pause_point:
> + * @job: The job that is ready to pause.
> + *
> + * Pause now if block_job_pause() has been called. Block jobs that perform
> + * lots of I/O must call this between requests so that the job can be paused.
> + */
> +void coroutine_fn block_job_pause_point(BlockJob *job);
> +
> +/**
> + * block_job_enter:
> + * @job: The job to enter.
> + *
> + * Continue the specified job by entering the coroutine.
> + */
> +void block_job_enter(BlockJob *job);
> +
> +/**
> + * block_job_ready:
> + * @job: The job which is now ready to complete.
> + *
> + * Send a BLOCK_JOB_READY event for the specified job.
> + */
> +void block_job_event_ready(BlockJob *job);
> +
> +/**
> + * block_job_error_action:
> + * @job: The job to signal an error for.
> + * @on_err: The error action setting.
> + * @is_read: Whether the operation was a read.
> + * @error: The error that was reported.
> + *
> + * Report an I/O error for a block job and possibly stop the VM. Return the
> + * action that was selected based on @on_err and @error.
> + */
> +BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError
> on_err,
> + int is_read, int error);
> +
> +typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> +
> +/**
> + * block_job_defer_to_main_loop:
> + * @job: The job
> + * @fn: The function to run in the main loop
> + * @opaque: The opaque value that is passed to @fn
> + *
> + * Execute a given function in the main loop with the BlockDriverState
> + * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
> + * anything that uses bdrv_drain_all() in the main loop.
> + *
> + * The @job AioContext is held while @fn executes.
> + */
> +void block_job_defer_to_main_loop(BlockJob *job,
> + BlockJobDeferToMainLoopFn *fn,
> + void *opaque);
> +
> +#endif
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index b79e0c6..f9afc3b 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -13,7 +13,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "sysemu/block-backend.h"
>
> typedef struct {
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 18bf850..60b78a3 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -13,7 +13,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "sysemu/block-backend.h"
>
> static const BlockJobDriver test_block_job_driver = {
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <address@hidden>
- Re: [Qemu-block] [PATCH 5/7] Blockjobs: Internalize user_pause logic, (continued)
- [Qemu-block] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal, John Snow, 2016/10/13
- [Qemu-block] [PATCH 4/7] blockjob: centralize QMP event emissions, John Snow, 2016/10/13
- [Qemu-block] [PATCH 6/7] blockjobs: split interface into public/private, Part 1, John Snow, 2016/10/13
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1, no-reply, 2016/10/14
- Re: [Qemu-block] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1, John Snow, 2016/10/14