* [Qemu-devel] [PATCH]hw/xen_disk: ioreq not finished on error
@ 2011-03-27 10:25 Feiran Zheng
2011-03-28 13:42 ` [Qemu-devel] " Stefano Stabellini
0 siblings, 1 reply; 8+ messages in thread
From: Feiran Zheng @ 2011-03-27 10:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Anthony PERARD, Kevin Wolf, Gerd Hoffmann, Stefano Stabellini
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 <famcool@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);
@@ -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;
err:
- ioreq->status = BLKIF_RSP_ERROR;
+ qemu_aio_complete(ioreq, -1);
return -1;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
2011-03-27 10:25 [Qemu-devel] [PATCH]hw/xen_disk: ioreq not finished on error Feiran Zheng
@ 2011-03-28 13:42 ` Stefano Stabellini
2011-03-28 14:02 ` Feiran Zheng
0 siblings, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2011-03-28 13:42 UTC (permalink / raw)
To: Feiran Zheng
Cc: Anthony Perard, Kevin Wolf, Stefano Stabellini,
qemu-devel@nongnu.org, Gerd Hoffmann
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 <famcool@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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
2011-03-28 13:42 ` [Qemu-devel] " Stefano Stabellini
@ 2011-03-28 14:02 ` Feiran Zheng
2011-03-28 18:16 ` Stefano Stabellini
0 siblings, 1 reply; 8+ messages in thread
From: Feiran Zheng @ 2011-03-28 14:02 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Anthony Perard, Kevin Wolf, qemu-devel@nongnu.org, Gerd Hoffmann
On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
<stefano.stabellini@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 <famcool@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
2011-03-28 14:02 ` Feiran Zheng
@ 2011-03-28 18:16 ` Stefano Stabellini
2011-03-29 0:34 ` Feiran Zheng
0 siblings, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2011-03-28 18:16 UTC (permalink / raw)
To: Feiran Zheng
Cc: Anthony Perard, Kevin Wolf, Gerd Hoffmann, qemu-devel@nongnu.org,
Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
On Mon, 28 Mar 2011, Feiran Zheng wrote:
> On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
> <stefano.stabellini@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 <famcool@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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH]hw/xen_disk: ioreq not finished on error
2011-03-28 18:16 ` Stefano Stabellini
@ 2011-03-29 0:34 ` Feiran Zheng
2011-03-29 1:00 ` [Qemu-devel] [PATCH V2] hw/xen_disk: " Feiran Zheng
0 siblings, 1 reply; 8+ messages in thread
From: Feiran Zheng @ 2011-03-29 0:34 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Anthony Perard, Kevin Wolf, qemu-devel@nongnu.org, Gerd Hoffmann
Sorry for the confusing, I'll clean up it.
On Tue, Mar 29, 2011 at 2:16 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 28 Mar 2011, Feiran Zheng wrote:
>> On Mon, Mar 28, 2011 at 9:42 PM, Stefano Stabellini
>> <stefano.stabellini@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 <famcool@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH V2] hw/xen_disk: ioreq not finished on error
2011-03-29 0:34 ` Feiran Zheng
@ 2011-03-29 1:00 ` Feiran Zheng
2011-03-29 10:48 ` [Qemu-devel] " Stefano Stabellini
0 siblings, 1 reply; 8+ messages in thread
From: Feiran Zheng @ 2011-03-29 1:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Anthony PERARD, Kevin Wolf, Gerd Hoffmann, Stefano Stabellini
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 <famcool@gmail.com>
---
hw/xen_disk.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 445bf03..558bf8a 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -310,7 +310,7 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
off_t pos;
if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1)
- goto err;
+ goto err_no_map;
if (ioreq->presync)
bdrv_flush(blkdev->bs);
@@ -364,6 +364,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;
}
@@ -393,7 +396,7 @@ 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;
+ goto err_no_map;
ioreq->aio_inflight++;
if (ioreq->presync)
@@ -427,6 +430,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
return 0;
err:
+ ioreq_unmap(ioreq);
+err_no_map:
+ ioreq_finish(ioreq);
ioreq->status = BLKIF_RSP_ERROR;
return -1;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH V2] hw/xen_disk: ioreq not finished on error
2011-03-29 1:00 ` [Qemu-devel] [PATCH V2] hw/xen_disk: " Feiran Zheng
@ 2011-03-29 10:48 ` Stefano Stabellini
2011-03-29 11:50 ` Kevin Wolf
0 siblings, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2011-03-29 10:48 UTC (permalink / raw)
To: Feiran Zheng
Cc: Anthony Perard, Kevin Wolf, Stefano Stabellini,
qemu-devel@nongnu.org, Gerd Hoffmann
On Tue, 29 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.
>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [PATCH V2] hw/xen_disk: ioreq not finished on error
2011-03-29 10:48 ` [Qemu-devel] " Stefano Stabellini
@ 2011-03-29 11:50 ` Kevin Wolf
0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2011-03-29 11:50 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Anthony Perard, Feiran Zheng, qemu-devel@nongnu.org,
Gerd Hoffmann
Am 29.03.2011 12:48, schrieb Stefano Stabellini:
> On Tue, 29 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.
>>
>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-03-29 11:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-27 10:25 [Qemu-devel] [PATCH]hw/xen_disk: ioreq not finished on error Feiran Zheng
2011-03-28 13:42 ` [Qemu-devel] " Stefano Stabellini
2011-03-28 14:02 ` Feiran Zheng
2011-03-28 18:16 ` Stefano Stabellini
2011-03-29 0:34 ` Feiran Zheng
2011-03-29 1:00 ` [Qemu-devel] [PATCH V2] hw/xen_disk: " Feiran Zheng
2011-03-29 10:48 ` [Qemu-devel] " Stefano Stabellini
2011-03-29 11:50 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).