| Message ID | 1385097894-1380-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 22, 2013 at 01:24:49PM +0800, Fam Zheng wrote: > +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp) > +{ > + BdrvOpBlocker *blocker; > + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); > + if (!QLIST_EMPTY(&bs->op_blockers[op])) { > + blocker = QLIST_FIRST(&bs->op_blockers[op]); > + *errp = error_copy(blocker->reason); > + return true; > + } > + return false; > +} It's worth following the convention that Error **errp may be NULL: if (errp) { *errp = error_copy(blocker->reason); } The bool return value might be enough for some callers who don't need the full Error.
Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben: > BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX s/list/lists/ > elements. Each list is a list of blockers of an operation type > (BlockOpType), that marks this BDS is currently blocked for certain type s/is/as/ s/for/for a/ > of operation with reason errors stored in the list. The rule of usage > is: > > * BDS user who wants to take an operation should check if there's any > blocker of the type with bdrv_op_is_blocked(). > > * BDS user who wants to block certain types of operation, should call > bdrv_op_block (or bdrv_op_block_all to block all types of operations, > which is similar to bdrv_set_in_use of now). > > * A blocker is only referenced by op_blockers, so the lifecycle is > managed by caller, and shouldn't be lost until unblock, so typically > a caller does these: > > - Allocate a blocker with error_setg or similar, call bdrv_op_block() > to block some operations. > - Hold the blocker, do his job. > - Unblock operations that it blocked, with the same reason pointer > passed to bdrv_op_unblock(). > - Release the blocker with error_free(). > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 6 ++++++ > include/block/block_int.h | 5 +++++ > 3 files changed, 66 insertions(+) > > diff --git a/block.c b/block.c > index 382ea71..2b18a43 100644 > --- a/block.c > +++ b/block.c > @@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs) > } > } > > +struct BdrvOpBlocker { > + Error *reason; > + QLIST_ENTRY(BdrvOpBlocker) list; > +}; > + > +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp) > +{ > + BdrvOpBlocker *blocker; > + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); Missing space after >= (same in all other functions) The logic looks fine. Kevin
Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben: > BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX > elements. Each list is a list of blockers of an operation type > (BlockOpType), that marks this BDS is currently blocked for certain type > of operation with reason errors stored in the list. The rule of usage > is: > > * BDS user who wants to take an operation should check if there's any > blocker of the type with bdrv_op_is_blocked(). > > * BDS user who wants to block certain types of operation, should call > bdrv_op_block (or bdrv_op_block_all to block all types of operations, > which is similar to bdrv_set_in_use of now). > > * A blocker is only referenced by op_blockers, so the lifecycle is > managed by caller, and shouldn't be lost until unblock, so typically > a caller does these: > > - Allocate a blocker with error_setg or similar, call bdrv_op_block() > to block some operations. > - Hold the blocker, do his job. > - Unblock operations that it blocked, with the same reason pointer > passed to bdrv_op_unblock(). > - Release the blocker with error_free(). > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 6 ++++++ > include/block/block_int.h | 5 +++++ > 3 files changed, 66 insertions(+) > > diff --git a/block.c b/block.c > index 382ea71..2b18a43 100644 > --- a/block.c > +++ b/block.c > @@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs) > } > } > > +struct BdrvOpBlocker { > + Error *reason; > + QLIST_ENTRY(BdrvOpBlocker) list; > +}; > + > +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp) > +{ > + BdrvOpBlocker *blocker; > + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); Oh, and my compiler doesn't like that check in the first place: block.c: In function 'bdrv_op_is_blocked': block.c:4473:5: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] block.c: In function 'bdrv_op_block': block.c:4485:5: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] block.c: In function 'bdrv_op_unblock': block.c:4495:5: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] Perhaps cast op to int for the first condition: assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX); Kevin
Il 22/11/2013 06:24, Fam Zheng ha scritto: > +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason) > +{ > + BdrvOpBlocker *blocker; > + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); > + > + blocker = g_malloc0(sizeof(BdrvOpBlocker)); > + blocker->reason = reason; > + QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); > +} > + > +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason) What about making BlockOpType a bitmask, and having bdrv_op_{,un}block take multiple ORed BlockOpTypes? bdrv_op_{,un}block_all then are not necessary, you only need a BLOCK_OPERATION_ALL value. Paolo
On 2013年11月26日 01:13, Paolo Bonzini wrote: > Il 22/11/2013 06:24, Fam Zheng ha scritto: >> +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason) >> +{ >> + BdrvOpBlocker *blocker; >> + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); >> + >> + blocker = g_malloc0(sizeof(BdrvOpBlocker)); >> + blocker->reason = reason; >> + QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); >> +} >> + >> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason) > > What about making BlockOpType a bitmask, and having bdrv_op_{,un}block > take multiple ORed BlockOpTypes? > > bdrv_op_{,un}block_all then are not necessary, you only need a > BLOCK_OPERATION_ALL value. > Bitmap is not enough, at least it should be an array. For example when we enable multiple block jobs, there're two stoppers for drive_del, right? And a bool or int is not as friendly as an error message, because when an operation is blocked we can't print a specific reason when we don't have this information at all. So let's be consistent with migration blockers. Fam
Il 26/11/2013 03:07, Fam Zheng ha scritto: >>> >>> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error >>> *reason) >> >> What about making BlockOpType a bitmask, and having bdrv_op_{,un}block >> take multiple ORed BlockOpTypes? >> >> bdrv_op_{,un}block_all then are not necessary, you only need a >> BLOCK_OPERATION_ALL value. >> > > Bitmap is not enough, at least it should be an array. For example when > we enable multiple block jobs, there're two stoppers for drive_del, right? I said bitmask, not bitmap. :) So that you can add the same Error to multiple items of the array with a single bdrv_op_block call (by ORing them into the second parameter). Paolo
On Tue, Nov 26, 2013 at 5:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/11/2013 03:07, Fam Zheng ha scritto: >>>> >>>> +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error >>>> *reason) >>> >>> What about making BlockOpType a bitmask, and having bdrv_op_{,un}block >>> take multiple ORed BlockOpTypes? >>> >>> bdrv_op_{,un}block_all then are not necessary, you only need a >>> BLOCK_OPERATION_ALL value. >>> >> >> Bitmap is not enough, at least it should be an array. For example when >> we enable multiple block jobs, there're two stoppers for drive_del, right? > > I said bitmask, not bitmap. :) > OK. Sorry.. > So that you can add the same Error to multiple items of the array with a > single bdrv_op_block call (by ORing them into the second parameter). > What data type to contain this? I'm not sure 64 is enough in long term... Fam
Il 26/11/2013 11:19, Fam Zheng ha scritto: >> So that you can add the same Error to multiple items of the array with a >> single bdrv_op_block call (by ORing them into the second parameter). > > What data type to contain this? I'm not sure 64 is enough in long term... I would just use an uint64_t, I think you have ~20 operations now. There is still quite some breathing room. Paolo
On Tue, Nov 26, 2013 at 6:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/11/2013 11:19, Fam Zheng ha scritto: >>> So that you can add the same Error to multiple items of the array with a >>> single bdrv_op_block call (by ORing them into the second parameter). >> >> What data type to contain this? I'm not sure 64 is enough in long term... > > I would just use an uint64_t, I think you have ~20 operations now. > There is still quite some breathing room. > OK. Then, can we still use QAPI enum? Because I think being possible to query blockers makes sense, that way the user doesn't have to trial and error to know what operation is blocked. Fam
Il 26/11/2013 11:31, Fam Zheng ha scritto: >> > I would just use an uint64_t, I think you have ~20 operations now. >> > There is still quite some breathing room. >> > > OK. Then, can we still use QAPI enum? Because I think being possible > to query blockers makes sense, that way the user doesn't have to trial > and error to know what operation is blocked. Yes, we can. You would still have to add a bunch of #define BLOCK_OPERATION_MASK_RESIZE (1ULL << BLOCK_OPERATION_TYPE_RESIZE) in block.h. Paolo
On Tue, Nov 26, 2013 at 6:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/11/2013 11:31, Fam Zheng ha scritto: >>> > I would just use an uint64_t, I think you have ~20 operations now. >>> > There is still quite some breathing room. >>> > >> OK. Then, can we still use QAPI enum? Because I think being possible >> to query blockers makes sense, that way the user doesn't have to trial >> and error to know what operation is blocked. > > Yes, we can. You would still have to add a bunch of > > #define BLOCK_OPERATION_MASK_RESIZE (1ULL << BLOCK_OPERATION_TYPE_RESIZE) > > in block.h. > Well that works, but then we have a repeated list of operations in two places, I'm not sure that would be good to have. Kevin, Stefan, any opinion? Fam
diff --git a/block.c b/block.c index 382ea71..2b18a43 100644 --- a/block.c +++ b/block.c @@ -4425,6 +4425,61 @@ void bdrv_unref(BlockDriverState *bs) } } +struct BdrvOpBlocker { + Error *reason; + QLIST_ENTRY(BdrvOpBlocker) list; +}; + +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp) +{ + BdrvOpBlocker *blocker; + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); + if (!QLIST_EMPTY(&bs->op_blockers[op])) { + blocker = QLIST_FIRST(&bs->op_blockers[op]); + *errp = error_copy(blocker->reason); + return true; + } + return false; +} + +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason) +{ + BdrvOpBlocker *blocker; + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); + + blocker = g_malloc0(sizeof(BdrvOpBlocker)); + blocker->reason = reason; + QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list); +} + +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason) +{ + BdrvOpBlocker *blocker, *next; + assert(op >=0 && op < BLOCK_OP_TYPE_MAX); + QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) { + if (blocker->reason == reason) { + QLIST_REMOVE(blocker, list); + g_free(blocker); + } + } +} + +void bdrv_op_block_all(BlockDriverState *bs, Error *reason) +{ + int i; + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + bdrv_op_block(bs, i, reason); + } +} + +void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason) +{ + int i; + for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { + bdrv_op_unblock(bs, i, reason); + } +} + void bdrv_set_in_use(BlockDriverState *bs, int in_use) { assert(bs->in_use != in_use); diff --git a/include/block/block.h b/include/block/block.h index 3560deb..693d305 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -403,6 +403,12 @@ void bdrv_unref(BlockDriverState *bs); void bdrv_set_in_use(BlockDriverState *bs, int in_use); int bdrv_in_use(BlockDriverState *bs); +bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); +void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); +void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason); +void bdrv_op_block_all(BlockDriverState *bs, Error *reason); +void bdrv_op_unblock_all(BlockDriverState *bs, Error *reason); + #ifdef CONFIG_LINUX_AIO int raw_get_aio_fd(BlockDriverState *bs); #else diff --git a/include/block/block_int.h b/include/block/block_int.h index 1666066..d8cef85 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -230,6 +230,8 @@ struct BlockDriver { QLIST_ENTRY(BlockDriver) list; }; +typedef struct BdrvOpBlocker BdrvOpBlocker; + /* * Note: the function bdrv_append() copies and swaps contents of * BlockDriverStates, so if you add new fields to this struct, please @@ -308,6 +310,9 @@ struct BlockDriverState { QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; + /* operation blockers */ + QLIST_HEAD(, BdrvOpBlocker) op_blockers[BLOCK_OP_TYPE_MAX]; + /* long-running background operation */ BlockJob *job;
BlockDriverState.op_blockers is an array of list with BLOCK_OP_TYPE_MAX elements. Each list is a list of blockers of an operation type (BlockOpType), that marks this BDS is currently blocked for certain type of operation with reason errors stored in the list. The rule of usage is: * BDS user who wants to take an operation should check if there's any blocker of the type with bdrv_op_is_blocked(). * BDS user who wants to block certain types of operation, should call bdrv_op_block (or bdrv_op_block_all to block all types of operations, which is similar to bdrv_set_in_use of now). * A blocker is only referenced by op_blockers, so the lifecycle is managed by caller, and shouldn't be lost until unblock, so typically a caller does these: - Allocate a blocker with error_setg or similar, call bdrv_op_block() to block some operations. - Hold the blocker, do his job. - Unblock operations that it blocked, with the same reason pointer passed to bdrv_op_unblock(). - Release the blocker with error_free(). Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 6 ++++++ include/block/block_int.h | 5 +++++ 3 files changed, 66 insertions(+)