Fam Zheng <f...@redhat.com> writes: > Signed-off-by: Fam Zheng <f...@redhat.com> > --- > block.c | 109 > +++++++++++++++++++++++++++++----------------- > block/mirror.c | 2 +- > include/block/block.h | 3 +- > include/block/block_int.h | 3 ++ > 4 files changed, 76 insertions(+), 41 deletions(-) > > diff --git a/block.c b/block.c > index c1a3576..41562fd 100644 > --- a/block.c > +++ b/block.c > @@ -961,63 +961,87 @@ fail: > /* > * Opens the backing file for a BlockDriverState if not yet open > * > + * If backing_bs is not NULL or empty, find the BDS by name and reference it > as > + * the backing_hd, in this case options is ignored. > + *
The name "backing_bs" suggests this is a BlockDriverState, which it's not. What about calling it "name", just like bdrv_find()'s parameter? Or "device_name", like the BlockDriverState member? > * options is a QDict of options to pass to the block drivers, or NULL for an > * empty set of options. The reference to the QDict is transferred to this > * function (even on failure), so if the caller intends to reuse the > dictionary, > * it needs to use QINCREF() before calling bdrv_file_open. > */ > -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error > **errp) > +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs, > + QDict *options, Error **errp) This function is now too overloaded for my taste. Possibly cleaner: * Have a new function to set backing_hd. Takes a device name argument, or maybe a BDS. I'm leaning towards the latter. * The new function and old bdrv_open_backing_file() have a common tail. Factor it out into a helper. > { > char backing_filename[PATH_MAX]; > int back_flags, ret; > BlockDriver *back_drv = NULL; > Error *local_err = NULL; > > - if (bs->backing_hd != NULL) { > - QDECREF(options); > - return 0; > - } > + if (backing_bs && *backing_bs != '\0') { > + bs->backing_hd = bdrv_find(backing_bs); What if bs->backing_hd is already set? > + if (!bs->backing_hd) { > + error_setg(errp, "Backing device not found: %s", backing_bs); > + return -ENOENT; > + } > + bdrv_ref(bs->backing_hd); > + } else { > + if (bs->backing_hd != NULL) { > + QDECREF(options); > + return 0; > + } > > - /* NULL means an empty set of options */ > - if (options == NULL) { > - options = qdict_new(); > - } > + /* NULL means an empty set of options */ > + if (options == NULL) { > + options = qdict_new(); > + } > > - bs->open_flags &= ~BDRV_O_NO_BACKING; > - if (qdict_haskey(options, "file.filename")) { > - backing_filename[0] = '\0'; > - } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { > - QDECREF(options); > - return 0; > - } else { > - bdrv_get_full_backing_filename(bs, backing_filename, > - sizeof(backing_filename)); > - } > + bs->open_flags &= ~BDRV_O_NO_BACKING; > + if (qdict_haskey(options, "file.filename")) { > + backing_filename[0] = '\0'; > + } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) { > + QDECREF(options); > + return 0; > + } else { > + bdrv_get_full_backing_filename(bs, backing_filename, > + sizeof(backing_filename)); > + } > > - bs->backing_hd = bdrv_new(""); > + bs->backing_hd = bdrv_new(""); > > - if (bs->backing_format[0] != '\0') { > - back_drv = bdrv_find_format(bs->backing_format); > - } > + if (bs->backing_format[0] != '\0') { > + back_drv = bdrv_find_format(bs->backing_format); > + } > > - /* backing files always opened read-only */ > - back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | > - BDRV_O_COPY_ON_READ); > + /* backing files always opened read-only */ > + back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | > + BDRV_O_COPY_ON_READ); > > - ret = bdrv_open(bs->backing_hd, > - *backing_filename ? backing_filename : NULL, options, > - back_flags, back_drv, &local_err); > - if (ret < 0) { > - bdrv_unref(bs->backing_hd); > - bs->backing_hd = NULL; > - bs->open_flags |= BDRV_O_NO_BACKING; > - error_setg(errp, "Could not open backing file: %s", > - error_get_pretty(local_err)); > - error_free(local_err); > - return ret; > + ret = bdrv_open(bs->backing_hd, > + *backing_filename ? backing_filename : NULL, options, > + back_flags, back_drv, &local_err); > + if (ret < 0) { > + bdrv_unref(bs->backing_hd); > + bs->backing_hd = NULL; > + bs->open_flags |= BDRV_O_NO_BACKING; > + error_setg(errp, "Could not open backing file: %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + return ret; > + } > } > + assert(bs->backing_hd); > + assert(!bs->backing_blocker); > + error_setg(&bs->backing_blocker, > + "device is used as backing hd of '%s'", > + bs->device_name); > + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); > + /* Otherwise we won't be able to commit due to check in bdrv_commit */ > + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, > + bs->backing_blocker); > pstrcpy(bs->backing_file, sizeof(bs->backing_file), > bs->backing_hd->file->filename); > + pstrcpy(bs->backing_format, sizeof(bs->backing_format), > + bs->backing_hd->drv->format_name); > return 0; > } > This patch hunk is hard to review, because you had to indent the old code. Same with whitespace differences ignored: @@ -961,18 +961,30 @@ /* * Opens the backing file for a BlockDriverState if not yet open * + * If backing_bs is not NULL or empty, find the BDS by name and reference it as + * the backing_hd, in this case options is ignored. + * * options is a QDict of options to pass to the block drivers, or NULL for an * empty set of options. The reference to the QDict is transferred to this * function (even on failure), so if the caller intends to reuse the dictionary, * it needs to use QINCREF() before calling bdrv_file_open. */ -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs, + QDict *options, Error **errp) { char backing_filename[PATH_MAX]; int back_flags, ret; BlockDriver *back_drv = NULL; Error *local_err = NULL; + if (backing_bs && *backing_bs != '\0') { + bs->backing_hd = bdrv_find(backing_bs); + if (!bs->backing_hd) { + error_setg(errp, "Backing device not found: %s", backing_bs); + return -ENOENT; + } + bdrv_ref(bs->backing_hd); + } else { if (bs->backing_hd != NULL) { QDECREF(options); return 0; @@ -1016,8 +1028,20 @@ error_free(local_err); return ret; } + } + assert(bs->backing_hd); + assert(!bs->backing_blocker); + error_setg(&bs->backing_blocker, + "device is used as backing hd of '%s'", + bs->device_name); + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); + /* Otherwise we won't be able to commit due to check in bdrv_commit */ + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, + bs->backing_blocker); pstrcpy(bs->backing_file, sizeof(bs->backing_file), bs->backing_hd->file->filename); + pstrcpy(bs->backing_format, sizeof(bs->backing_format), + bs->backing_hd->drv->format_name); return 0; } Looks like you do more than the stated subject in this patch: you also add a blocker! Separate patch, please. "Block all" followed by "unblock specific" is a bit awkward. A function taking a bit set would let you do the masking on bits rather than an array of lists. Suggestion, not a request, and it can be done in a followup patch. > @@ -1164,9 +1188,11 @@ int bdrv_open(BlockDriverState *bs, const char > *filename, QDict *options, > /* If there is a backing file, use it */ > if ((flags & BDRV_O_NO_BACKING) == 0) { > QDict *backing_options; > - > qdict_extract_subqdict(options, &backing_options, "backing."); > - ret = bdrv_open_backing_file(bs, backing_options, &local_err); > + ret = bdrv_open_backing_file(bs, qdict_get_try_str(options, > "backing"), > + backing_options, &local_err); > + > + qdict_del(options, "backing"); Why do you have to delete "backing"? > if (ret < 0) { > goto close_and_fail; > } > @@ -1459,9 +1485,14 @@ void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > if (bs->backing_hd) { > + assert(bs->backing_blocker); > + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > bdrv_unref(bs->backing_hd); > bs->backing_hd = NULL; > } > + if (bs->backing_blocker) { > + error_free(bs->backing_blocker); > + } Can bs->backing_blocker legitimately be non-null when bs->backing_hd is null? > bs->drv->bdrv_close(bs); > g_free(bs->opaque); > #ifdef _WIN32 > diff --git a/block/mirror.c b/block/mirror.c > index 6dc27ad..b1596d3 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -511,7 +511,7 @@ static void mirror_complete(BlockJob *job, Error **errp) > Error *local_err = NULL; > int ret; > > - ret = bdrv_open_backing_file(s->target, NULL, &local_err); > + ret = bdrv_open_backing_file(s->target, NULL, NULL, &local_err); > if (ret < 0) { > char backing_filename[PATH_MAX]; > bdrv_get_full_backing_filename(s->target, backing_filename, > diff --git a/include/block/block.h b/include/block/block.h > index 18397e0..24d06f9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -185,7 +185,8 @@ int bdrv_parse_cache_flags(const char *mode, int *flags); > int bdrv_parse_discard_flags(const char *mode, int *flags); > int bdrv_file_open(BlockDriverState **pbs, const char *filename, > QDict *options, int flags, Error **errp); > -int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error > **errp); > +int bdrv_open_backing_file(BlockDriverState *bs, const char *backing_bs, > + QDict *options, Error **errp); > int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > int flags, BlockDriver *drv, Error **errp); > BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 458acd6..83dcf7d 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -342,6 +342,9 @@ struct BlockDriverState { > BlockJob *job; > > QDict *options; > + > + /* For backing reference, block the operations of named backing device */ > + Error *backing_blocker; > }; > > int get_tmp_filename(char *filename, int size); I find the comment confusing :) What about: /* The error object in use for blocking operations on backing_hd */