diff mbox series

[v4,6/8] scsi: account unmap operations

Message ID 1534844779-118784-7-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series discard blockstats | expand

Commit Message

Anton Nefedov Aug. 21, 2018, 9:46 a.m. UTC 
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/scsi-disk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Kevin Wolf Oct. 4, 2018, 3:47 p.m. UTC | #1 
Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  hw/scsi/scsi-disk.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 9d10daf..0aac137 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1664,6 +1664,10 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>              goto done;
>          }
>  
> +        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> +                         r->sector_count * s->qdev.blocksize,
> +                         BLOCK_ACCT_UNMAP);

If the check just above this (check_lba_range) fails, we should account
for an invalid request.

>          r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>                                          r->sector * s->qdev.blocksize,
>                                          r->sector_count * s->qdev.blocksize,
> @@ -1690,10 +1694,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
>      r->req.aiocb = NULL;
>  
>      aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> -    if (scsi_disk_req_check_error(r, ret, false)) {
> +    if (scsi_disk_req_check_error(r, ret, true)) {
>          scsi_req_unref(&r->req);
>          g_free(data);
>      } else {
> +        block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>          scsi_unmap_complete_noio(data, ret);
>      }
>      aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> @@ -1740,10 +1745,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
>      return;
>  
>  invalid_param_len:
> +    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>      scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
>      return;
>  
>  invalid_field:
> +    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>      scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>  }

What about the blk_is_read_only() case which directly returns without
jumping to one of the error labels?

Kevin
Anton Nefedov Oct. 8, 2018, 2:43 p.m. UTC | #2 
On 4/10/2018 6:47 PM, Kevin Wolf wrote:
> Am 21.08.2018 um 11:46 hat Anton Nefedov geschrieben:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   hw/scsi/scsi-disk.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 9d10daf..0aac137 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1664,6 +1664,10 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>>               goto done;
>>           }
>>   
>> +        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>> +                         r->sector_count * s->qdev.blocksize,
>> +                         BLOCK_ACCT_UNMAP);
> 
> If the check just above this (check_lba_range) fails, we should account
> for an invalid request.
> 

Done.

>>           r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>>                                           r->sector * s->qdev.blocksize,
>>                                           r->sector_count * s->qdev.blocksize,
>> @@ -1690,10 +1694,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
>>       r->req.aiocb = NULL;
>>   
>>       aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> -    if (scsi_disk_req_check_error(r, ret, false)) {
>> +    if (scsi_disk_req_check_error(r, ret, true)) {
>>           scsi_req_unref(&r->req);
>>           g_free(data);
>>       } else {
>> +        block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>>           scsi_unmap_complete_noio(data, ret);
>>       }
>>       aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> @@ -1740,10 +1745,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
>>       return;
>>   
>>   invalid_param_len:
>> +    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>>       scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
>>       return;
>>   
>>   invalid_field:
>> +    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
>>       scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>>   }
> 
> What about the blk_is_read_only() case which directly returns without
> jumping to one of the error labels?
> 

So basically anything we don't bring to blk layer is invalid req.
Fixed.
diff mbox series

Patch

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 9d10daf..0aac137 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1664,6 +1664,10 @@  static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
             goto done;
         }
 
+        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+                         r->sector_count * s->qdev.blocksize,
+                         BLOCK_ACCT_UNMAP);
+
         r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
                                         r->sector * s->qdev.blocksize,
                                         r->sector_count * s->qdev.blocksize,
@@ -1690,10 +1694,11 @@  static void scsi_unmap_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
 
     aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
-    if (scsi_disk_req_check_error(r, ret, false)) {
+    if (scsi_disk_req_check_error(r, ret, true)) {
         scsi_req_unref(&r->req);
         g_free(data);
     } else {
+        block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
         scsi_unmap_complete_noio(data, ret);
     }
     aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
@@ -1740,10 +1745,12 @@  static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     return;
 
 invalid_param_len:
+    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
     scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
     return;
 
 invalid_field:
+    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
     scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }