* [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release()
@ 2012-05-10 15:37 Jan Beulich
2012-05-11 16:01 ` Stefano Stabellini
2012-05-11 16:01 ` Stefano Stabellini
0 siblings, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2012-05-10 15:37 UTC (permalink / raw)
To: xen-devel, qemu-devel; +Cc: Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]
While for the "normal" case (called from blk_send_response_all())
decrementing requests_finished is correct, doing so in the parse error
case is wrong; requests_inflight needs to be decremented instead.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
blkdev->requests_finished++;
}
-static void ioreq_release(struct ioreq *ioreq)
+static void ioreq_release(struct ioreq *ioreq, bool finish)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
@@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
memset(ioreq, 0, sizeof(*ioreq));
ioreq->blkdev = blkdev;
QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
- blkdev->requests_finished--;
+ if (finish)
+ blkdev->requests_finished--;
+ else
+ blkdev->requests_inflight--;
}
/*
@@ -449,7 +452,7 @@ static void blk_send_response_all(struct
while (!QLIST_EMPTY(&blkdev->finished)) {
ioreq = QLIST_FIRST(&blkdev->finished);
send_notify += blk_send_response_one(ioreq);
- ioreq_release(ioreq);
+ ioreq_release(ioreq, true);
}
if (send_notify) {
xen_be_send_notify(&blkdev->xendev);
@@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
if (blk_send_response_one(ioreq)) {
xen_be_send_notify(&blkdev->xendev);
}
- ioreq_release(ioreq);
+ ioreq_release(ioreq, false);
continue;
}
[-- Attachment #2: qemu-xendisk-accounting.patch --]
[-- Type: text/plain, Size: 1638 bytes --]
qemu/xendisk: properly update stats in ioreq_release()
While for the "normal" case (called from blk_send_response_all())
decrementing requests_finished is correct, doing so in the parse error
case is wrong; requests_inflight needs to be decremented instead.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
blkdev->requests_finished++;
}
-static void ioreq_release(struct ioreq *ioreq)
+static void ioreq_release(struct ioreq *ioreq, bool finish)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
@@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
memset(ioreq, 0, sizeof(*ioreq));
ioreq->blkdev = blkdev;
QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
- blkdev->requests_finished--;
+ if (finish)
+ blkdev->requests_finished--;
+ else
+ blkdev->requests_inflight--;
}
/*
@@ -449,7 +452,7 @@ static void blk_send_response_all(struct
while (!QLIST_EMPTY(&blkdev->finished)) {
ioreq = QLIST_FIRST(&blkdev->finished);
send_notify += blk_send_response_one(ioreq);
- ioreq_release(ioreq);
+ ioreq_release(ioreq, true);
}
if (send_notify) {
xen_be_send_notify(&blkdev->xendev);
@@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
if (blk_send_response_one(ioreq)) {
xen_be_send_notify(&blkdev->xendev);
}
- ioreq_release(ioreq);
+ ioreq_release(ioreq, false);
continue;
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release()
2012-05-10 15:37 [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release() Jan Beulich
@ 2012-05-11 16:01 ` Stefano Stabellini
2012-05-14 7:39 ` Jan Beulich
2012-05-14 7:39 ` [Qemu-devel] " Jan Beulich
2012-05-11 16:01 ` Stefano Stabellini
1 sibling, 2 replies; 7+ messages in thread
From: Stefano Stabellini @ 2012-05-11 16:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: Stefano Stabellini, qemu-devel, xen-devel
On Thu, 10 May 2012, Jan Beulich wrote:
> While for the "normal" case (called from blk_send_response_all())
> decrementing requests_finished is correct, doing so in the parse error
> case is wrong; requests_inflight needs to be decremented instead.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
> blkdev->requests_finished++;
> }
>
> -static void ioreq_release(struct ioreq *ioreq)
> +static void ioreq_release(struct ioreq *ioreq, bool finish)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
> memset(ioreq, 0, sizeof(*ioreq));
> ioreq->blkdev = blkdev;
> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
> - blkdev->requests_finished--;
> + if (finish)
> + blkdev->requests_finished--;
> + else
> + blkdev->requests_inflight--;
> }
>
> /*
> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
> while (!QLIST_EMPTY(&blkdev->finished)) {
> ioreq = QLIST_FIRST(&blkdev->finished);
> send_notify += blk_send_response_one(ioreq);
> - ioreq_release(ioreq);
> + ioreq_release(ioreq, true);
> }
> if (send_notify) {
> xen_be_send_notify(&blkdev->xendev);
> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
> if (blk_send_response_one(ioreq)) {
> xen_be_send_notify(&blkdev->xendev);
> }
> - ioreq_release(ioreq);
> + ioreq_release(ioreq, false);
> continue;
> }
In case of an error returned by ioreq_parse requests_finished should
also be decremented.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qemu/xendisk: properly update stats in ioreq_release()
2012-05-10 15:37 [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release() Jan Beulich
2012-05-11 16:01 ` Stefano Stabellini
@ 2012-05-11 16:01 ` Stefano Stabellini
1 sibling, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2012-05-11 16:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: Stefano Stabellini, qemu-devel, xen-devel
On Thu, 10 May 2012, Jan Beulich wrote:
> While for the "normal" case (called from blk_send_response_all())
> decrementing requests_finished is correct, doing so in the parse error
> case is wrong; requests_inflight needs to be decremented instead.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
> blkdev->requests_finished++;
> }
>
> -static void ioreq_release(struct ioreq *ioreq)
> +static void ioreq_release(struct ioreq *ioreq, bool finish)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
> memset(ioreq, 0, sizeof(*ioreq));
> ioreq->blkdev = blkdev;
> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
> - blkdev->requests_finished--;
> + if (finish)
> + blkdev->requests_finished--;
> + else
> + blkdev->requests_inflight--;
> }
>
> /*
> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
> while (!QLIST_EMPTY(&blkdev->finished)) {
> ioreq = QLIST_FIRST(&blkdev->finished);
> send_notify += blk_send_response_one(ioreq);
> - ioreq_release(ioreq);
> + ioreq_release(ioreq, true);
> }
> if (send_notify) {
> xen_be_send_notify(&blkdev->xendev);
> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
> if (blk_send_response_one(ioreq)) {
> xen_be_send_notify(&blkdev->xendev);
> }
> - ioreq_release(ioreq);
> + ioreq_release(ioreq, false);
> continue;
> }
In case of an error returned by ioreq_parse requests_finished should
also be decremented.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release()
2012-05-11 16:01 ` Stefano Stabellini
2012-05-14 7:39 ` Jan Beulich
@ 2012-05-14 7:39 ` Jan Beulich
2012-05-14 9:39 ` Stefano Stabellini
2012-05-14 9:39 ` Stefano Stabellini
1 sibling, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2012-05-14 7:39 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel, xen-devel
>>> On 11.05.12 at 18:01, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Thu, 10 May 2012, Jan Beulich wrote:
>> While for the "normal" case (called from blk_send_response_all())
>> decrementing requests_finished is correct, doing so in the parse error
>> case is wrong; requests_inflight needs to be decremented instead.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/hw/xen_disk.c
>> +++ b/hw/xen_disk.c
>> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
>> blkdev->requests_finished++;
>> }
>>
>> -static void ioreq_release(struct ioreq *ioreq)
>> +static void ioreq_release(struct ioreq *ioreq, bool finish)
>> {
>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>
>> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
>> memset(ioreq, 0, sizeof(*ioreq));
>> ioreq->blkdev = blkdev;
>> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
>> - blkdev->requests_finished--;
>> + if (finish)
>> + blkdev->requests_finished--;
>> + else
>> + blkdev->requests_inflight--;
>> }
>>
>> /*
>> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
>> while (!QLIST_EMPTY(&blkdev->finished)) {
>> ioreq = QLIST_FIRST(&blkdev->finished);
>> send_notify += blk_send_response_one(ioreq);
>> - ioreq_release(ioreq);
>> + ioreq_release(ioreq, true);
>> }
>> if (send_notify) {
>> xen_be_send_notify(&blkdev->xendev);
>> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
>> if (blk_send_response_one(ioreq)) {
>> xen_be_send_notify(&blkdev->xendev);
>> }
>> - ioreq_release(ioreq);
>> + ioreq_release(ioreq, false);
>> continue;
>> }
>
> In case of an error returned by ioreq_parse requests_finished should
> also be decremented.
I don't think so - it gets incremented in ioreq_finish(), and that gets
called only when I/O was at least attempted (which isn't the case
when parse failed).
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qemu/xendisk: properly update stats in ioreq_release()
2012-05-11 16:01 ` Stefano Stabellini
@ 2012-05-14 7:39 ` Jan Beulich
2012-05-14 7:39 ` [Qemu-devel] " Jan Beulich
1 sibling, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2012-05-14 7:39 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel, xen-devel
>>> On 11.05.12 at 18:01, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Thu, 10 May 2012, Jan Beulich wrote:
>> While for the "normal" case (called from blk_send_response_all())
>> decrementing requests_finished is correct, doing so in the parse error
>> case is wrong; requests_inflight needs to be decremented instead.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/hw/xen_disk.c
>> +++ b/hw/xen_disk.c
>> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
>> blkdev->requests_finished++;
>> }
>>
>> -static void ioreq_release(struct ioreq *ioreq)
>> +static void ioreq_release(struct ioreq *ioreq, bool finish)
>> {
>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>
>> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
>> memset(ioreq, 0, sizeof(*ioreq));
>> ioreq->blkdev = blkdev;
>> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
>> - blkdev->requests_finished--;
>> + if (finish)
>> + blkdev->requests_finished--;
>> + else
>> + blkdev->requests_inflight--;
>> }
>>
>> /*
>> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
>> while (!QLIST_EMPTY(&blkdev->finished)) {
>> ioreq = QLIST_FIRST(&blkdev->finished);
>> send_notify += blk_send_response_one(ioreq);
>> - ioreq_release(ioreq);
>> + ioreq_release(ioreq, true);
>> }
>> if (send_notify) {
>> xen_be_send_notify(&blkdev->xendev);
>> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
>> if (blk_send_response_one(ioreq)) {
>> xen_be_send_notify(&blkdev->xendev);
>> }
>> - ioreq_release(ioreq);
>> + ioreq_release(ioreq, false);
>> continue;
>> }
>
> In case of an error returned by ioreq_parse requests_finished should
> also be decremented.
I don't think so - it gets incremented in ioreq_finish(), and that gets
called only when I/O was at least attempted (which isn't the case
when parse failed).
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release()
2012-05-14 7:39 ` [Qemu-devel] " Jan Beulich
@ 2012-05-14 9:39 ` Stefano Stabellini
2012-05-14 9:39 ` Stefano Stabellini
1 sibling, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2012-05-14 9:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Mon, 14 May 2012, Jan Beulich wrote:
> >>> On 11.05.12 at 18:01, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> wrote:
> > On Thu, 10 May 2012, Jan Beulich wrote:
> >> While for the "normal" case (called from blk_send_response_all())
> >> decrementing requests_finished is correct, doing so in the parse error
> >> case is wrong; requests_inflight needs to be decremented instead.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/hw/xen_disk.c
> >> +++ b/hw/xen_disk.c
> >> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
> >> blkdev->requests_finished++;
> >> }
> >>
> >> -static void ioreq_release(struct ioreq *ioreq)
> >> +static void ioreq_release(struct ioreq *ioreq, bool finish)
> >> {
> >> struct XenBlkDev *blkdev = ioreq->blkdev;
> >>
> >> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
> >> memset(ioreq, 0, sizeof(*ioreq));
> >> ioreq->blkdev = blkdev;
> >> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
> >> - blkdev->requests_finished--;
> >> + if (finish)
> >> + blkdev->requests_finished--;
> >> + else
> >> + blkdev->requests_inflight--;
> >> }
> >>
> >> /*
> >> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
> >> while (!QLIST_EMPTY(&blkdev->finished)) {
> >> ioreq = QLIST_FIRST(&blkdev->finished);
> >> send_notify += blk_send_response_one(ioreq);
> >> - ioreq_release(ioreq);
> >> + ioreq_release(ioreq, true);
> >> }
> >> if (send_notify) {
> >> xen_be_send_notify(&blkdev->xendev);
> >> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
> >> if (blk_send_response_one(ioreq)) {
> >> xen_be_send_notify(&blkdev->xendev);
> >> }
> >> - ioreq_release(ioreq);
> >> + ioreq_release(ioreq, false);
> >> continue;
> >> }
> >
> > In case of an error returned by ioreq_parse requests_finished should
> > also be decremented.
>
> I don't think so - it gets incremented in ioreq_finish(), and that gets
> called only when I/O was at least attempted (which isn't the case
> when parse failed).
You are right, the patch is correct as it is.
However it is not following QEMU's CODING_STYLE (braces around single
line statements): you can use scripts/checkpatch.pl to check your
patches for code style errors.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qemu/xendisk: properly update stats in ioreq_release()
2012-05-14 7:39 ` [Qemu-devel] " Jan Beulich
2012-05-14 9:39 ` Stefano Stabellini
@ 2012-05-14 9:39 ` Stefano Stabellini
1 sibling, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2012-05-14 9:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Mon, 14 May 2012, Jan Beulich wrote:
> >>> On 11.05.12 at 18:01, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> wrote:
> > On Thu, 10 May 2012, Jan Beulich wrote:
> >> While for the "normal" case (called from blk_send_response_all())
> >> decrementing requests_finished is correct, doing so in the parse error
> >> case is wrong; requests_inflight needs to be decremented instead.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/hw/xen_disk.c
> >> +++ b/hw/xen_disk.c
> >> @@ -154,7 +154,7 @@ static void ioreq_finish(struct ioreq *i
> >> blkdev->requests_finished++;
> >> }
> >>
> >> -static void ioreq_release(struct ioreq *ioreq)
> >> +static void ioreq_release(struct ioreq *ioreq, bool finish)
> >> {
> >> struct XenBlkDev *blkdev = ioreq->blkdev;
> >>
> >> @@ -162,7 +162,10 @@ static void ioreq_release(struct ioreq *
> >> memset(ioreq, 0, sizeof(*ioreq));
> >> ioreq->blkdev = blkdev;
> >> QLIST_INSERT_HEAD(&blkdev->freelist, ioreq, list);
> >> - blkdev->requests_finished--;
> >> + if (finish)
> >> + blkdev->requests_finished--;
> >> + else
> >> + blkdev->requests_inflight--;
> >> }
> >>
> >> /*
> >> @@ -449,7 +452,7 @@ static void blk_send_response_all(struct
> >> while (!QLIST_EMPTY(&blkdev->finished)) {
> >> ioreq = QLIST_FIRST(&blkdev->finished);
> >> send_notify += blk_send_response_one(ioreq);
> >> - ioreq_release(ioreq);
> >> + ioreq_release(ioreq, true);
> >> }
> >> if (send_notify) {
> >> xen_be_send_notify(&blkdev->xendev);
> >> @@ -505,7 +508,7 @@ static void blk_handle_requests(struct X
> >> if (blk_send_response_one(ioreq)) {
> >> xen_be_send_notify(&blkdev->xendev);
> >> }
> >> - ioreq_release(ioreq);
> >> + ioreq_release(ioreq, false);
> >> continue;
> >> }
> >
> > In case of an error returned by ioreq_parse requests_finished should
> > also be decremented.
>
> I don't think so - it gets incremented in ioreq_finish(), and that gets
> called only when I/O was at least attempted (which isn't the case
> when parse failed).
You are right, the patch is correct as it is.
However it is not following QEMU's CODING_STYLE (braces around single
line statements): you can use scripts/checkpatch.pl to check your
patches for code style errors.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-14 9:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-10 15:37 [Qemu-devel] qemu/xendisk: properly update stats in ioreq_release() Jan Beulich
2012-05-11 16:01 ` Stefano Stabellini
2012-05-14 7:39 ` Jan Beulich
2012-05-14 7:39 ` [Qemu-devel] " Jan Beulich
2012-05-14 9:39 ` Stefano Stabellini
2012-05-14 9:39 ` Stefano Stabellini
2012-05-11 16:01 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.