[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
On Sun, 27 Mar 2011, Feiran Zheng wrote:
Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.
Signed-off-by: Feiran Zheng famc...@gmail.com
---
hw/xen_disk.c | 22 +-
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..7940fab 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
int i, rc, len = 0;
off_t pos;
-if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+}
if (ioreq-presync)
bdrv_flush(blkdev-bs);
this change is just to make the code clearer and easier to read, right?
@@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
return 0;
err:
+ioreq_unmap(ioreq);
+err_no_map:
+ioreq_finish(ioreq);
ioreq-status = BLKIF_RSP_ERROR;
return -1;
}
@@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq-blkdev;
-if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+}
ioreq-aio_inflight++;
if (ioreq-presync)
@@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
qemu_aio_complete(ioreq, 0);
return 0;
+
+err_no_map:
+ioreq_finish(ioreq);
+ioreq-status = BLKIF_RSP_ERROR;
+return -1;
why aren't you calling ioreq_unmap before ioreq_finish, like in the
previous case?
[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
On Sun, 27 Mar 2011, Feiran Zheng wrote:
Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.
Signed-off-by: Feiran Zheng famc...@gmail.com
---
hw/xen_disk.c | 22 +-
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..7940fab 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
int i, rc, len = 0;
off_t pos;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
if (ioreq-presync)
bdrv_flush(blkdev-bs);
this change is just to make the code clearer and easier to read, right?
I think so.
@@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
return 0;
err:
+ ioreq_unmap(ioreq);
+err_no_map:
+ ioreq_finish(ioreq);
ioreq-status = BLKIF_RSP_ERROR;
return -1;
}
@@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq-blkdev;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
ioreq-aio_inflight++;
if (ioreq-presync)
@@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
qemu_aio_complete(ioreq, 0);
return 0;
+
+err_no_map:
+ ioreq_finish(ioreq);
+ ioreq-status = BLKIF_RSP_ERROR;
+ return -1;
why aren't you calling ioreq_unmap before ioreq_finish, like in the
previous case?
It seems not a must but if call qemu_aio_complete, the error info can
be printed, I thought it may be helpful.
--
Best regards!
Fam Zheng
[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
On Mon, 28 Mar 2011, Feiran Zheng wrote:
On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
On Sun, 27 Mar 2011, Feiran Zheng wrote:
Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.
Signed-off-by: Feiran Zheng famc...@gmail.com
---
hw/xen_disk.c | 22 +-
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..7940fab 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
int i, rc, len = 0;
off_t pos;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
if (ioreq-presync)
bdrv_flush(blkdev-bs);
this change is just to make the code clearer and easier to read, right?
I think so.
@@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
return 0;
err:
+ ioreq_unmap(ioreq);
+err_no_map:
+ ioreq_finish(ioreq);
ioreq-status = BLKIF_RSP_ERROR;
return -1;
}
@@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq-blkdev;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
ioreq-aio_inflight++;
if (ioreq-presync)
@@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
qemu_aio_complete(ioreq, 0);
return 0;
+
+err_no_map:
+ ioreq_finish(ioreq);
+ ioreq-status = BLKIF_RSP_ERROR;
+ return -1;
why aren't you calling ioreq_unmap before ioreq_finish, like in the
previous case?
It seems not a must but if call qemu_aio_complete, the error info can
be printed, I thought it may be helpful.
I am not sure I understand what you mean here because in case of error
we don't call qemu_aio_complete at all in the err_no_map code path.
[Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
Sorry for the confusing, I'll clean up it.
On Tue, Mar 29, 2011 at 2:16 AM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
On Mon, 28 Mar 2011, Feiran Zheng wrote:
On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
stefano.stabell...@eu.citrix.com wrote:
On Sun, 27 Mar 2011, Feiran Zheng wrote:
Bug fix: routines 'ioreq_runio_qemu_sync' and 'ioreq_runio_qemu_aio'
won't call 'ioreq_unmap' or 'ioreq_finish' on errors, leaving ioreq in
the blkdev-inflight list and a leak.
Signed-off-by: Feiran Zheng famc...@gmail.com
---
hw/xen_disk.c | 22 +-
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..7940fab 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -309,8 +309,10 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
int i, rc, len = 0;
off_t pos;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
if (ioreq-presync)
bdrv_flush(blkdev-bs);
this change is just to make the code clearer and easier to read, right?
I think so.
@@ -364,6 +366,9 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
return 0;
err:
+ ioreq_unmap(ioreq);
+err_no_map:
+ ioreq_finish(ioreq);
ioreq-status = BLKIF_RSP_ERROR;
return -1;
}
@@ -392,8 +397,10 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq-blkdev;
- if (ioreq-req.nr_segments ioreq_map(ioreq) == -1)
- goto err;
+ if (ioreq-req.nr_segments) {
+ if (ioreq_map(ioreq) == -1)
+ goto err_no_map;
+ }
ioreq-aio_inflight++;
if (ioreq-presync)
@@ -425,9 +432,14 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
qemu_aio_complete(ioreq, 0);
return 0;
+
+err_no_map:
+ ioreq_finish(ioreq);
+ ioreq-status = BLKIF_RSP_ERROR;
+ return -1;
why aren't you calling ioreq_unmap before ioreq_finish, like in the
previous case?
It seems not a must but if call qemu_aio_complete, the error info can
be printed, I thought it may be helpful.
I am not sure I understand what you mean here because in case of error
we don't call qemu_aio_complete at all in the err_no_map code path.
--
Best regards!
Fam Zheng
