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>
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;
         }

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.

>>> 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

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.