Re: [Qemu-devel] [RFC 1/6] block: add request tracking
On Mon, Oct 17, 2011 at 11:47 PM, Stefan Hajnoczi
stefa...@linux.vnet.ibm.com wrote:
The block layer does not know about pending requests. This information
is necessary for copy-on-read since overlapping requests must be
serialized to prevent races that corrupt the image.
Add a simple mechanism to enable/disable request tracking. By default
request tracking is disabled.
The BlockDriverState gets a new tracked_request list field which
contains all pending requests. Each request is a BdrvTrackedRequest
record with sector_num, nb_sectors, and is_write fields.
Signed-off-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
block.c | 83
++-
block_int.h | 8 +
2 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/block.c b/block.c
index 9873b57..2d2c62a 100644
--- a/block.c
+++ b/block.c
@@ -978,6 +978,77 @@ void bdrv_commit_all(void)
}
}
+struct BdrvTrackedRequest {
+ BlockDriverState *bs;
+ int64_t sector_num;
+ int nb_sectors;
+ bool is_write;
+ QLIST_ENTRY(BdrvTrackedRequest) list;
+};
+
+/**
+ * Remove an active request from the tracked requests list
+ *
+ * If req is NULL, no operation is performed.
+ *
+ * This function should be called when a tracked request is completing.
+ */
+static void tracked_request_remove(BdrvTrackedRequest *req)
+{
+ if (req) {
+ QLIST_REMOVE(req, list);
+ g_free(req);
+ }
+}
+
+/**
+ * Add an active request to the tracked requests list
+ *
+ * Return NULL if request tracking is disabled, non-NULL otherwise.
+ */
+static BdrvTrackedRequest *tracked_request_add(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors, bool is_write)
+{
+ BdrvTrackedRequest *req;
+
+ if (!bs-request_tracking) {
+ return NULL;
+ }
+
+ req = g_malloc(sizeof(*req));
+ req-bs = bs;
+ req-sector_num = sector_num;
+ req-nb_sectors = nb_sectors;
+ req-is_write = is_write;
+
+ QLIST_INSERT_HEAD(bs-tracked_requests, req, list);
+
+ return req;
+}
+
+/**
+ * Enable tracking of incoming requests
+ *
+ * Request tracking can be safely used by multiple users at the same time,
+ * there is an internal reference count to match start and stop calls.
+ */
+void bdrv_start_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking++;
+}
+
+/**
+ * Disable tracking of incoming requests
+ *
+ * Note that in-flight requests are still tracked, this function only stops
+ * tracking incoming requests.
+ */
+void bdrv_stop_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking--;
+}
I don't understand what the real intention for the above functions is.
IMHO, why can we not drop them?
+
/*
* Return values:
* 0 - success
@@ -1262,6 +1333,8 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
{
BlockDriver *drv = bs-drv;
+ BdrvTrackedRequest *req;
+ int ret;
if (!drv) {
return -ENOMEDIUM;
@@ -1270,7 +1343,10 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
return -EIO;
}
- return drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+ req = tracked_request_add(bs, sector_num, nb_sectors, false);
+ ret = drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+ tracked_request_remove(req);
+ return ret;
}
int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
@@ -1288,6 +1364,7 @@ static int coroutine_fn
bdrv_co_do_writev(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
{
BlockDriver *drv = bs-drv;
+ BdrvTrackedRequest *req;
int ret;
if (!bs-drv) {
@@ -1300,6 +1377,8 @@ static int coroutine_fn
bdrv_co_do_writev(BlockDriverState *bs,
return -EIO;
}
+ req = tracked_request_add(bs, sector_num, nb_sectors, true);
+
ret = drv-bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
if (bs-dirty_bitmap) {
@@ -1310,6 +1389,8 @@ static int coroutine_fn
bdrv_co_do_writev(BlockDriverState *bs,
bs-wr_highest_sector = sector_num + nb_sectors - 1;
}
+ tracked_request_remove(req);
+
return ret;
}
diff --git a/block_int.h b/block_int.h
index f2f4f2d..87ce8b4 100644
--- a/block_int.h
+++ b/block_int.h
@@ -43,6 +43,8 @@
#define BLOCK_OPT_PREALLOC preallocation
#define BLOCK_OPT_SUBFMT subformat
+typedef struct BdrvTrackedRequest BdrvTrackedRequest;
+
typedef struct AIOPool {
void (*cancel)(BlockDriverAIOCB *acb);
int aiocb_size;
@@ -206,6 +208,9 @@ struct BlockDriverState {
int in_use; /* users other than guest access, eg. block migration */
QTAILQ_ENTRY(BlockDriverState) list;
void *private;
+
+ int
Re: [Qemu-devel] [RFC 1/6] block: add request tracking
On Mon, Nov 7, 2011 at 11:00 AM, Zhi Yong Wu zwu.ker...@gmail.com wrote:
On Mon, Oct 17, 2011 at 11:47 PM, Stefan Hajnoczi
stefa...@linux.vnet.ibm.com wrote:
+/**
+ * Enable tracking of incoming requests
+ *
+ * Request tracking can be safely used by multiple users at the same time,
+ * there is an internal reference count to match start and stop calls.
+ */
+void bdrv_start_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking++;
+}
+
+/**
+ * Disable tracking of incoming requests
+ *
+ * Note that in-flight requests are still tracked, this function only stops
+ * tracking incoming requests.
+ */
+void bdrv_stop_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking--;
+}
I don't understand what the real intention for the above functions is.
IMHO, why can we not drop them?
I have dropped them after removing the g_malloc() as Kevin suggested.
The idea was to avoid the overhead of request tracking when no feature
is using request tracking.
Stefan
Re: [Qemu-devel] [RFC 1/6] block: add request tracking
On Mon, Nov 7, 2011 at 7:41 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Nov 7, 2011 at 11:00 AM, Zhi Yong Wu zwu.ker...@gmail.com wrote:
On Mon, Oct 17, 2011 at 11:47 PM, Stefan Hajnoczi
stefa...@linux.vnet.ibm.com wrote:
+/**
+ * Enable tracking of incoming requests
+ *
+ * Request tracking can be safely used by multiple users at the same time,
+ * there is an internal reference count to match start and stop calls.
+ */
+void bdrv_start_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking++;
+}
+
+/**
+ * Disable tracking of incoming requests
+ *
+ * Note that in-flight requests are still tracked, this function only stops
+ * tracking incoming requests.
+ */
+void bdrv_stop_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking--;
+}
I don't understand what the real intention for the above functions is.
IMHO, why can we not drop them?
I have dropped them after removing the g_malloc() as Kevin suggested.
The idea was to avoid the overhead of request tracking when no feature
is using request tracking.
Great
Stefan
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [RFC 1/6] block: add request tracking
On Wed, Nov 2, 2011 at 4:30 PM, Kevin Wolf kw...@redhat.com wrote:
Am 17.10.2011 17:47, schrieb Stefan Hajnoczi:
The block layer does not know about pending requests. This information
is necessary for copy-on-read since overlapping requests must be
serialized to prevent races that corrupt the image.
Add a simple mechanism to enable/disable request tracking. By default
request tracking is disabled.
The BlockDriverState gets a new tracked_request list field which
contains all pending requests. Each request is a BdrvTrackedRequest
record with sector_num, nb_sectors, and is_write fields.
Signed-off-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
block.c | 83
++-
block_int.h | 8 +
2 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/block.c b/block.c
index 9873b57..2d2c62a 100644
--- a/block.c
+++ b/block.c
@@ -978,6 +978,77 @@ void bdrv_commit_all(void)
}
}
+struct BdrvTrackedRequest {
+ BlockDriverState *bs;
+ int64_t sector_num;
+ int nb_sectors;
+ bool is_write;
+ QLIST_ENTRY(BdrvTrackedRequest) list;
+};
+
+/**
+ * Remove an active request from the tracked requests list
+ *
+ * If req is NULL, no operation is performed.
+ *
+ * This function should be called when a tracked request is completing.
+ */
+static void tracked_request_remove(BdrvTrackedRequest *req)
+{
+ if (req) {
+ QLIST_REMOVE(req, list);
+ g_free(req);
+ }
+}
+
+/**
+ * Add an active request to the tracked requests list
+ *
+ * Return NULL if request tracking is disabled, non-NULL otherwise.
+ */
+static BdrvTrackedRequest *tracked_request_add(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors, bool
is_write)
+{
+ BdrvTrackedRequest *req;
+
+ if (!bs-request_tracking) {
+ return NULL;
+ }
+
+ req = g_malloc(sizeof(*req));
+ req-bs = bs;
+ req-sector_num = sector_num;
+ req-nb_sectors = nb_sectors;
+ req-is_write = is_write;
How about using g_malloc0 or a compound literal assignment for
initialisation, so that we won't get surprises when the struct is extended?
Sure.
+
+ QLIST_INSERT_HEAD(bs-tracked_requests, req, list);
+
+ return req;
+}
+
+/**
+ * Enable tracking of incoming requests
+ *
+ * Request tracking can be safely used by multiple users at the same time,
+ * there is an internal reference count to match start and stop calls.
+ */
+void bdrv_start_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking++;
+}
+
+/**
+ * Disable tracking of incoming requests
+ *
+ * Note that in-flight requests are still tracked, this function only stops
+ * tracking incoming requests.
+ */
+void bdrv_stop_request_tracking(BlockDriverState *bs)
+{
+ bs-request_tracking--;
+}
+
/*
* Return values:
* 0 - success
@@ -1262,6 +1333,8 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
{
BlockDriver *drv = bs-drv;
+ BdrvTrackedRequest *req;
+ int ret;
if (!drv) {
return -ENOMEDIUM;
@@ -1270,7 +1343,10 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
return -EIO;
}
- return drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+ req = tracked_request_add(bs, sector_num, nb_sectors, false);
+ ret = drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+ tracked_request_remove(req);
Hm... Do we actually need to malloc the BdrvTrackedRequest? If all users
are like this (and at least those in this patch are), we can just keep
it on the coroutine stack.
Okay.
Stefan
Re: [Qemu-devel] [RFC 1/6] block: add request tracking
Am 17.10.2011 17:47, schrieb Stefan Hajnoczi:
The block layer does not know about pending requests. This information
is necessary for copy-on-read since overlapping requests must be
serialized to prevent races that corrupt the image.
Add a simple mechanism to enable/disable request tracking. By default
request tracking is disabled.
The BlockDriverState gets a new tracked_request list field which
contains all pending requests. Each request is a BdrvTrackedRequest
record with sector_num, nb_sectors, and is_write fields.
Signed-off-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
block.c | 83
++-
block_int.h |8 +
2 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/block.c b/block.c
index 9873b57..2d2c62a 100644
--- a/block.c
+++ b/block.c
@@ -978,6 +978,77 @@ void bdrv_commit_all(void)
}
}
+struct BdrvTrackedRequest {
+BlockDriverState *bs;
+int64_t sector_num;
+int nb_sectors;
+bool is_write;
+QLIST_ENTRY(BdrvTrackedRequest) list;
+};
+
+/**
+ * Remove an active request from the tracked requests list
+ *
+ * If req is NULL, no operation is performed.
+ *
+ * This function should be called when a tracked request is completing.
+ */
+static void tracked_request_remove(BdrvTrackedRequest *req)
+{
+if (req) {
+QLIST_REMOVE(req, list);
+g_free(req);
+}
+}
+
+/**
+ * Add an active request to the tracked requests list
+ *
+ * Return NULL if request tracking is disabled, non-NULL otherwise.
+ */
+static BdrvTrackedRequest *tracked_request_add(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors, bool is_write)
+{
+BdrvTrackedRequest *req;
+
+if (!bs-request_tracking) {
+return NULL;
+}
+
+req = g_malloc(sizeof(*req));
+req-bs = bs;
+req-sector_num = sector_num;
+req-nb_sectors = nb_sectors;
+req-is_write = is_write;
How about using g_malloc0 or a compound literal assignment for
initialisation, so that we won't get surprises when the struct is extended?
+
+QLIST_INSERT_HEAD(bs-tracked_requests, req, list);
+
+return req;
+}
+
+/**
+ * Enable tracking of incoming requests
+ *
+ * Request tracking can be safely used by multiple users at the same time,
+ * there is an internal reference count to match start and stop calls.
+ */
+void bdrv_start_request_tracking(BlockDriverState *bs)
+{
+bs-request_tracking++;
+}
+
+/**
+ * Disable tracking of incoming requests
+ *
+ * Note that in-flight requests are still tracked, this function only stops
+ * tracking incoming requests.
+ */
+void bdrv_stop_request_tracking(BlockDriverState *bs)
+{
+bs-request_tracking--;
+}
+
/*
* Return values:
* 0- success
@@ -1262,6 +1333,8 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
{
BlockDriver *drv = bs-drv;
+BdrvTrackedRequest *req;
+int ret;
if (!drv) {
return -ENOMEDIUM;
@@ -1270,7 +1343,10 @@ static int coroutine_fn
bdrv_co_do_readv(BlockDriverState *bs,
return -EIO;
}
-return drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+req = tracked_request_add(bs, sector_num, nb_sectors, false);
+ret = drv-bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
+tracked_request_remove(req);
Hm... Do we actually need to malloc the BdrvTrackedRequest? If all users
are like this (and at least those in this patch are), we can just keep
it on the coroutine stack.
Kevin
