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;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Stefano Stabellini
2012-May-11 16:01 UTC
Re: qemu/xendisk: properly update stats in ioreq_release()
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.
Jan Beulich
2012-May-14 07:39 UTC
Re: qemu/xendisk: properly update stats in ioreq_release()
>>> 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
Stefano Stabellini
2012-May-14 09:39 UTC
Re: qemu/xendisk: properly update stats in ioreq_release()
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.