[Qemu-devel] [RFC PATCH 0/3] move global bdrv_states out of block.c

[Qemu-devel] [RFC PATCH 0/3] move global bdrv_states out of block.c

[Wenchao Xia]

In order to support multiple caller from different thread, global
inside block layer should be carefully treated. bdrv_states represent
a group of bds* which is now used by qemu, so it is a user concept which
should be managed by user. This series tries to move it out, so later
different thread can feed the API with its own bds* group.

This is a RFC series which does not completely convert the API, the missing
part is adding parameter *l in all API which uses bdrv_states, then move
bdrv_states to caller. About 10 functions need to be converted, so hope to
get comments before that, to see if this is the right direction.

Wenchao Xia (3):
  1 block: add typedef for BlockDriverState queue
  2 block: add function qemu_get_bds_queue
  3 block: add parameter bds queue in bdrv_invalidate_cache_all()

 block.c               |   11 ++++++++---
 include/block/block.h |    7 ++++++-
 migration.c           |    2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

[Qemu-devel] [RFC PATCH 1/3] block: add typedef for BlockDriverState queue

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    2 +-
 include/block/block.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index a387c1a..9db3c32 100644
--- a/block.c
+++ b/block.c
@@ -93,7 +93,7 @@ static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
 static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
         bool is_write, int64_t *wait);
 
-static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
+static BlockDrvierStateQueue bdrv_states =
     QTAILQ_HEAD_INITIALIZER(bdrv_states);
 
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
diff --git a/include/block/block.h b/include/block/block.h
index 742fce5..b0d4f2b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -86,6 +86,8 @@ typedef enum {
 } BlockErrorAction;
 
 typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockDrvierStateQueue, BlockDriverState) \
+        BlockDrvierStateQueue;
 
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
-- 
1.7.1

[Qemu-devel] [RFC PATCH 2/3] block: add function qemu_get_bds_queue

[Wenchao Xia]

It should be caller's responsibility to tell which bds* needs take
action in multithread case, but since now only one thread exist
and only one bds* group exist, add this function to manage the
bds* queue temporarily. Once the block layer user code manage
the bds* queue by itself and global bdrv_states is moved out,
this function can be removed.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    5 +++++
 include/block/block.h |    3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 9db3c32..bf35b90 100644
--- a/block.c
+++ b/block.c
@@ -102,6 +102,11 @@ static QLIST_HEAD(, BlockDriver) bdrv_drivers =
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
+BlockDrvierStateQueue *qemu_get_bds_queue(void)
+{
+    return &bdrv_states;
+}
+
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
 {
diff --git a/include/block/block.h b/include/block/block.h
index b0d4f2b..323a732 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -95,6 +95,9 @@ typedef struct BDRVReopenState {
     void *opaque;
 } BDRVReopenState;
 
+/* This function could be removed when block layer is thread safe. Then let
+   caller manage the bds it used, instead of block layer. */
+BlockDrvierStateQueue *qemu_get_bds_queue(void);
 
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
-- 
1.7.1

[Qemu-devel] [RFC PATCH 3/3] block: add parameter bds queue in bdrv_invalidate_cache_all()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    4 ++--
 include/block/block.h |    2 +-
 migration.c           |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index bf35b90..e4dd84e 100644
--- a/block.c
+++ b/block.c
@@ -4175,11 +4175,11 @@ void bdrv_invalidate_cache(BlockDriverState *bs)
     }
 }
 
-void bdrv_invalidate_cache_all(void)
+void bdrv_invalidate_cache_all(BlockDrvierStateQueue *q)
 {
     BlockDriverState *bs;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+    QTAILQ_FOREACH(bs, q, list) {
         bdrv_invalidate_cache(bs);
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 323a732..3762fe1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -267,7 +267,7 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
 
 /* Invalidate any cached metadata used by image formats */
 void bdrv_invalidate_cache(BlockDriverState *bs);
-void bdrv_invalidate_cache_all(void);
+void bdrv_invalidate_cache_all(BlockDrvierStateQueue *q);
 
 void bdrv_clear_incoming_migration_all(void);
 
diff --git a/migration.c b/migration.c
index 200d404..3153f20 100644
--- a/migration.c
+++ b/migration.c
@@ -113,7 +113,7 @@ static void process_incoming_migration_co(void *opaque)
 
     bdrv_clear_incoming_migration_all();
     /* Make sure all file formats flush their mutable metadata */
-    bdrv_invalidate_cache_all();
+    bdrv_invalidate_cache_all(qemu_get_bds_queue());
 
     if (autostart) {
         vm_start();
-- 
1.7.1

Re: [Qemu-devel] [RFC PATCH 0/3] move global bdrv_states out of block.c

[Stefan Hajnoczi]

On Tue, Aug 27, 2013 at 03:49:22PM +0800, Wenchao Xia wrote:
> In order to support multiple caller from different thread, global
> inside block layer should be carefully treated. bdrv_states represent
> a group of bds* which is now used by qemu, so it is a user concept which
> should be managed by user. This series tries to move it out, so later
> different thread can feed the API with its own bds* group.
> 
> This is a RFC series which does not completely convert the API, the missing
> part is adding parameter *l in all API which uses bdrv_states, then move
> bdrv_states to caller. About 10 functions need to be converted, so hope to
> get comments before that, to see if this is the right direction.

The global bdrv_states list needs to stay, I don't think we should try
to remove it.

The list is used to drain and flush all devices so that we can safely
shut down or live migrate.  If we give each thread their own list then
there's no way to globally drain and flush.

The invalidate function that you modified is another example: we *must*
invalidate all BDS instances across all threads.  Failure to invalidate
all instances could mean inconsistencies or data loss.

So in the end I think we need to keep bdrv_states.

Stefan

Re: [Qemu-devel] [RFC PATCH 0/3] move global bdrv_states out of block.c

[Wenchao Xia]

On 09/18/2013 10:03 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 27, 2013 at 03:49:22PM +0800, Wenchao Xia wrote:
>> In order to support multiple caller from different thread, global
>> inside block layer should be carefully treated. bdrv_states represent
>> a group of bds* which is now used by qemu, so it is a user concept which
>> should be managed by user. This series tries to move it out, so later
>> different thread can feed the API with its own bds* group.
>>
>> This is a RFC series which does not completely convert the API, the missing
>> part is adding parameter *l in all API which uses bdrv_states, then move
>> bdrv_states to caller. About 10 functions need to be converted, so hope to
>> get comments before that, to see if this is the right direction.
> The global bdrv_states list needs to stay, I don't think we should try
> to remove it.
>
> The list is used to drain and flush all devices so that we can safely
> shut down or live migrate.  If we give each thread their own list then
> there's no way to globally drain and flush.
>
> The invalidate function that you modified is another example: we *must*
> invalidate all BDS instances across all threads.  Failure to invalidate
> all instances could mean inconsistencies or data loss.
>
> So in the end I think we need to keep bdrv_states.
   I am OK to keep bdrv_states now, after the talk about AioContext
in IRC. I think it is related with thread model, and, If we find drain a 
subset
of BDS is needed, such as per AioContext drain, it is no late to change
at that time, after the programming model is built. Looking forward to your
next series for AioContext.

>
> Stefan
>