| Message ID | 1323784351-25531-3-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 13.12.2011 14:52, schrieb Stefan Hajnoczi: > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > block_int.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 83 insertions(+), 0 deletions(-) Why are these functions static inline in the header instead of being implemented in block.c? The other thing I was going to ask, but don't really know to which patch I should reply, is whether we need to take care of bdrv_close/delete while a block job is still running. What happens if you unplug a device that is being streamed? Kevin
On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote: > Am 13.12.2011 14:52, schrieb Stefan Hajnoczi: > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > --- > > block_int.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 83 insertions(+), 0 deletions(-) > > Why are these functions static inline in the header instead of being > implemented in block.c? You are right, they should be moved. > The other thing I was going to ask, but don't really know to which patch > I should reply, is whether we need to take care of bdrv_close/delete > while a block job is still running. What happens if you unplug a device > that is being streamed? Hotplug is protected by the bdrv_set_in_use() we issue during block job creation/completion. The 'change', 'resize', and 'snapshot_blkdev' operations are not protected AFAICT. I'll take a close look at them and send patches. Stefan
On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote: > Am 13.12.2011 14:52, schrieb Stefan Hajnoczi: > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > --- > > block_int.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 83 insertions(+), 0 deletions(-) > > Why are these functions static inline in the header instead of being > implemented in block.c? > > The other thing I was going to ask, but don't really know to which patch > I should reply, is whether we need to take care of bdrv_close/delete > while a block job is still running. What happens if you unplug a device > that is being streamed? There is an additional reference, its not deleted until the operation completes. Or at least it should be like that, Stefan?
On Thu, Dec 15, 2011 at 08:40:08AM -0200, Marcelo Tosatti wrote: > On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote: > > Am 13.12.2011 14:52, schrieb Stefan Hajnoczi: > > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > > --- > > > block_int.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 files changed, 83 insertions(+), 0 deletions(-) > > > > Why are these functions static inline in the header instead of being > > implemented in block.c? > > > > The other thing I was going to ask, but don't really know to which patch > > I should reply, is whether we need to take care of bdrv_close/delete > > while a block job is still running. What happens if you unplug a device > > that is being streamed? > > There is an additional reference, its not deleted until the operation > completes. Or at least it should be like that, Stefan? Right, the patch uses bdrv_set_in_use(). In my other reply to this sub-thread I mentioned that certain operations like 'change', 'resize', and 'snapshot_blkdev' do not seem to check bdrv_in_use() yet. So I think in order to make image streaming safe additional checks will need to be added in blockdev.c. Stefan
diff --git a/block_int.h b/block_int.h index 89a860c..bc397c4 100644 --- a/block_int.h +++ b/block_int.h @@ -69,6 +69,36 @@ typedef struct BlockIOBaseValue { uint64_t ios[2]; } BlockIOBaseValue; +typedef void BlockJobCancelFunc(void *opaque); +typedef struct BlockJob BlockJob; +typedef struct BlockJobType { + /** Derived BlockJob struct size */ + size_t instance_size; + + /** String describing the operation, part of query-block-jobs QMP API */ + const char *job_type; + + /** Optional callback for job types that support setting a speed limit */ + int (*set_speed)(BlockJob *job, int64_t value); +} BlockJobType; + +/** + * Long-running operation on a BlockDriverState + */ +struct BlockJob { + const BlockJobType *job_type; + BlockDriverState *bs; + bool cancelled; + + /* These fields are published by the query-block-jobs QMP API */ + int64_t offset; + int64_t len; + int64_t speed; + + BlockDriverCompletionFunc *cb; + void *opaque; +}; + struct BlockDriver { const char *format_name; int instance_size; @@ -270,6 +300,9 @@ struct BlockDriverState { void *private; QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; + + /* long-running background operation */ + BlockJob *job; }; struct BlockDriverAIOCB { @@ -293,4 +326,54 @@ void bdrv_set_io_limits(BlockDriverState *bs, int is_windows_drive(const char *filename); #endif +static inline void *block_job_create(const BlockJobType *job_type, + BlockDriverState *bs, + BlockDriverCompletionFunc *cb, + void *opaque) +{ + BlockJob *job; + + if (bdrv_in_use(bs)) { + return NULL; + } + bdrv_set_in_use(bs, 1); + + job = g_malloc0(job_type->instance_size); + job->job_type = job_type; + job->bs = bs; + job->cb = cb; + job->opaque = opaque; + bs->job = job; + return job; +} + +static inline void block_job_complete(BlockJob *job, int ret) +{ + BlockDriverState *bs = job->bs; + + assert(bs->job == job); + job->cb(job->opaque, ret); + bs->job = NULL; + g_free(job); + bdrv_set_in_use(bs, 0); +} + +static inline int block_job_set_speed(BlockJob *job, int64_t value) +{ + if (!job->job_type->set_speed) { + return -ENOTSUP; + } + return job->job_type->set_speed(job, value); +} + +static inline void block_job_cancel(BlockJob *job) +{ + job->cancelled = true; +} + +static inline bool block_job_is_cancelled(BlockJob *job) +{ + return job->cancelled; +} + #endif /* BLOCK_INT_H */
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- block_int.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 83 insertions(+), 0 deletions(-)