| Message ID | 20111230100503.809473621@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti <mtosatti@redhat.com> wrote: > +int bdrv_is_allocated_base(BlockDriverState *top, > + BlockDriverState *base, > + int64_t sector_num, > + int nb_sectors, int *pnum) Since this function runs in coroutine context it should be marked: int coroutine_fn bdrv_co_is_allocated_base(...) The _co_ in the filename is just a convention to identify block layer functions that execute in coroutine context. The coroutine_fn annotation is useful if we ever want static checker support that verifies that coroutine functions are always called from coroutine context. > +{ > + BlockDriverState *intermediate; > + int ret, n; > + > + ret = bdrv_co_is_allocated(top, sector_num, nb_sectors, &n); > + if (ret) { > + *pnum = n; > + return ret; > + } > + > + /* > + * Is the unallocated chunk [sector_num, n] also > + * unallocated between base and top? > + */ > + intermediate = top->backing_hd; This reaches into BlockDriverState->backing_hd. In practice this is probably the simplest and best way to do it. But if we're going to do this then do we even need the BlockDriver .bdrv_find_backing_file() abstraction? We could implement generic code which traverses ->backing_hd in block.c and if you don't use BlockDriverState->backing_hd then you're out of luck. > @@ -129,7 +141,10 @@ retry: > bdrv_disable_zero_detection(bs); > > if (sector_num == end && ret == 0) { > - bdrv_change_backing_file(bs, NULL, NULL); > + const char *base_id = NULL; > + if (base) > + base_id = s->backing_file_id; > + bdrv_change_backing_file(bs, base_id, NULL); This makes me want to move the bdrv_change_backing_file() call out to blockdev.c where we would perform it on successful completion. That way we don't need to pass base_id into stream.c at all. A streaming block job would no longer automatically change the backing file on completion. > @@ -166,6 +182,8 @@ int stream_start(BlockDriverState *bs, B > > s = block_job_create(&stream_job_type, bs, cb, opaque); > s->base = base; > + if (base_id) > + strncpy(s->backing_file_id, base_id, sizeof(s->backing_file_id) - 1); cutils.c:pstrcpy() is nicer than strncpy(), it does not need the size - 1.
On Wed, Jan 04, 2012 at 12:39:57PM +0000, Stefan Hajnoczi wrote: > On Fri, Dec 30, 2011 at 10:03 AM, Marcelo Tosatti <mtosatti@redhat.com> wrote: > > +int bdrv_is_allocated_base(BlockDriverState *top, > > + BlockDriverState *base, > > + int64_t sector_num, > > + int nb_sectors, int *pnum) > > Since this function runs in coroutine context it should be marked: > int coroutine_fn bdrv_co_is_allocated_base(...) > > The _co_ in the filename is just a convention to identify block layer > functions that execute in coroutine context. The coroutine_fn > annotation is useful if we ever want static checker support that > verifies that coroutine functions are always called from coroutine > context. Done. > > +{ > > + BlockDriverState *intermediate; > > + int ret, n; > > + > > + ret = bdrv_co_is_allocated(top, sector_num, nb_sectors, &n); > > + if (ret) { > > + *pnum = n; > > + return ret; > > + } > > + > > + /* > > + * Is the unallocated chunk [sector_num, n] also > > + * unallocated between base and top? > > + */ > > + intermediate = top->backing_hd; > > This reaches into BlockDriverState->backing_hd. In practice this is > probably the simplest and best way to do it. But if we're going to do > this then do we even need the BlockDriver .bdrv_find_backing_file() > abstraction? We could implement generic code which traverses > ->backing_hd in block.c and if you don't use > BlockDriverState->backing_hd then you're out of luck. Right. Then it becomes necessary for drivers to fill in ->backing_hd and ->backing_file properly, which is reasonable. Will drop the abstractions. > > @@ -129,7 +141,10 @@ retry: > > bdrv_disable_zero_detection(bs); > > > > if (sector_num == end && ret == 0) { > > - bdrv_change_backing_file(bs, NULL, NULL); > > + const char *base_id = NULL; > > + if (base) > > + base_id = s->backing_file_id; > > + bdrv_change_backing_file(bs, base_id, NULL); > > This makes me want to move the bdrv_change_backing_file() call out to > blockdev.c where we would perform it on successful completion. That > way we don't need to pass base_id into stream.c at all. A streaming > block job would no longer automatically change the backing file on > completion. Well, it is safer to perform the backing file change under refcounting protection (i don't see the advantage of your suggestion other than the cosmetic aspect of saving a variable). > > @@ -166,6 +182,8 @@ int stream_start(BlockDriverState *bs, B > > > > s = block_job_create(&stream_job_type, bs, cb, opaque); > > s->base = base; > > + if (base_id) > > + strncpy(s->backing_file_id, base_id, sizeof(s->backing_file_id) - 1); > > cutils.c:pstrcpy() is nicer than strncpy(), it does not need the size - 1. Done.
Index: stefanha/block.c =================================================================== --- stefanha.orig/block.c +++ stefanha/block.c @@ -2229,6 +2229,70 @@ int bdrv_is_allocated(BlockDriverState * return data.ret; } +/* + * Given an image chain: [BASE] -> [INTER1] -> [INTER2] -> [TOP] + * + * Return true if the given sector is allocated in top or base. + * Return false if the given sector is allocated in intermediate images. + * + * 'pnum' is set to the number of sectors (including and immediately following + * the specified sector) that are known to be in the same + * allocated/unallocated state. + * + */ +int bdrv_is_allocated_base(BlockDriverState *top, + BlockDriverState *base, + int64_t sector_num, + int nb_sectors, int *pnum) +{ + BlockDriverState *intermediate; + int ret, n; + + ret = bdrv_co_is_allocated(top, sector_num, nb_sectors, &n); + if (ret) { + *pnum = n; + return ret; + } + + /* + * Is the unallocated chunk [sector_num, n] also + * unallocated between base and top? + */ + intermediate = top->backing_hd; + + while (intermediate) { + int pnum_inter; + + /* reached base */ + if (intermediate == base) { + *pnum = n; + return 1; + } + ret = bdrv_co_is_allocated(intermediate, sector_num, nb_sectors, + &pnum_inter); + if (ret < 0) { + return ret; + } else if (ret) { + *pnum = pnum_inter; + return 0; + } + + /* + * [sector_num, nb_sectors] is unallocated on top but intermediate + * might have + * + * [sector_num+x, nr_sectors] allocated. + */ + if (n > pnum_inter) { + n = pnum_inter; + } + + intermediate = intermediate->backing_hd; + } + + return 1; +} + void bdrv_mon_event(const BlockDriverState *bdrv, BlockMonEventAction action, int is_read) { Index: stefanha/block.h =================================================================== --- stefanha.orig/block.h +++ stefanha/block.h @@ -222,6 +222,8 @@ int bdrv_co_discard(BlockDriverState *bs int bdrv_has_zero_init(BlockDriverState *bs); int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum); +int bdrv_is_allocated_base(BlockDriverState *top, BlockDriverState *base, + int64_t sector_num, int nb_sectors, int *pnum); #define BIOS_ATA_TRANSLATION_AUTO 0 #define BIOS_ATA_TRANSLATION_NONE 1 Index: stefanha/block/stream.c =================================================================== --- stefanha.orig/block/stream.c +++ stefanha/block/stream.c @@ -57,6 +57,7 @@ typedef struct StreamBlockJob { BlockJob common; RateLimit limit; BlockDriverState *base; + char backing_file_id[1024]; } StreamBlockJob; static int coroutine_fn stream_populate(BlockDriverState *bs, @@ -79,6 +80,7 @@ static void coroutine_fn stream_run(void { StreamBlockJob *s = opaque; BlockDriverState *bs = s->common.bs; + BlockDriverState *base = s->base; int64_t sector_num, end; int ret = 0; int n; @@ -97,8 +99,17 @@ retry: break; } - ret = bdrv_co_is_allocated(bs, sector_num, - STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n); + + if (base) { + ret = bdrv_is_allocated_base(bs, base, sector_num, + STREAM_BUFFER_SIZE / + BDRV_SECTOR_SIZE, + &n); + } else { + ret = bdrv_co_is_allocated(bs, sector_num, + STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, + &n); + } trace_stream_one_iteration(s, sector_num, n, ret); if (ret == 0) { if (s->common.speed) { @@ -115,6 +126,7 @@ retry: if (ret < 0) { break; } + ret = 0; /* Publish progress */ s->common.offset += n * BDRV_SECTOR_SIZE; @@ -129,7 +141,10 @@ retry: bdrv_disable_zero_detection(bs); if (sector_num == end && ret == 0) { - bdrv_change_backing_file(bs, NULL, NULL); + const char *base_id = NULL; + if (base) + base_id = s->backing_file_id; + bdrv_change_backing_file(bs, base_id, NULL); } qemu_vfree(buf); @@ -155,7 +170,8 @@ static BlockJobType stream_job_type = { }; int stream_start(BlockDriverState *bs, BlockDriverState *base, - BlockDriverCompletionFunc *cb, void *opaque) + const char *base_id, BlockDriverCompletionFunc *cb, + void *opaque) { StreamBlockJob *s; Coroutine *co; @@ -166,6 +182,8 @@ int stream_start(BlockDriverState *bs, B s = block_job_create(&stream_job_type, bs, cb, opaque); s->base = base; + if (base_id) + strncpy(s->backing_file_id, base_id, sizeof(s->backing_file_id) - 1); co = qemu_coroutine_create(stream_run); trace_stream_start(bs, base, s, co, opaque); Index: stefanha/blockdev.c =================================================================== --- stefanha.orig/blockdev.c +++ stefanha/blockdev.c @@ -931,6 +931,7 @@ void qmp_block_stream(const char *device const char *base, Error **errp) { BlockDriverState *bs; + BlockDriverState *base_bs; int ret; bs = bdrv_find(device); @@ -939,13 +940,15 @@ void qmp_block_stream(const char *device return; } - /* Base device not supported */ if (base) { - error_set(errp, QERR_NOT_SUPPORTED); - return; + base_bs = bdrv_find_backing_image(bs, base); + if (base_bs == NULL) { + error_set(errp, QERR_BASE_ID_NOT_FOUND, base); + return; + } } - ret = stream_start(bs, NULL, block_stream_cb, bs); + ret = stream_start(bs, base_bs, base, block_stream_cb, bs); if (ret < 0) { switch (ret) { case -EBUSY: Index: stefanha/block_int.h =================================================================== --- stefanha.orig/block_int.h +++ stefanha/block_int.h @@ -380,6 +380,7 @@ static inline bool block_job_is_cancelle } int stream_start(BlockDriverState *bs, BlockDriverState *base, - BlockDriverCompletionFunc *cb, void *opaque); + const char *base_id, BlockDriverCompletionFunc *cb, + void *opaque); #endif /* BLOCK_INT_H */
Add support for streaming data from an intermediate section of the image chain (see patch and documentation for details). Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>