1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v8 0/9] discard blockstats
  4. View patch

[Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct

Anton Nefedov posted 9 patches 6 years, 8 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, John Snow <jsnow@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Max Reitz <mreitz@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
Posted by Anton Nefedov 6 years, 8 months ago 
it allows to report it in the error handler

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 | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e7e865ab3b..b43254103c 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
 {
     SCSIDiskReq *r = data->r;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    uint64_t sector_num;
-    uint32_t nb_sectors;
 
     assert(r->req.aiocb == NULL);
     if (scsi_disk_req_check_error(r, ret, false)) {
@@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
     }
 
     if (data->count > 0) {
-        sector_num = ldq_be_p(&data->inbuf[0]);
-        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
-        if (!check_lba_range(s, sector_num, nb_sectors)) {
+        r->sector = ldq_be_p(&data->inbuf[0]);
+        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
+        if (!check_lba_range(s, r->sector, r->sector_count)) {
             scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
             goto done;
         }
 
         r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
-                                        sector_num * s->qdev.blocksize,
-                                        nb_sectors * s->qdev.blocksize,
+                                        r->sector * s->qdev.blocksize,
+                                        r->sector_count * s->qdev.blocksize,
                                         scsi_unmap_complete, data);
         data->count--;
         data->inbuf += 16;
-- 
2.17.1
Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
Posted by Max Reitz 6 years, 6 months ago 
On 16.05.19 16:33, Anton Nefedov wrote:
> it allows to report it in the error handler
> 
> 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 | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

(Sorry for the late reply :-/)

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e7e865ab3b..b43254103c 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>  {
>      SCSIDiskReq *r = data->r;
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    uint64_t sector_num;
> -    uint32_t nb_sectors;
>  
>      assert(r->req.aiocb == NULL);
>      if (scsi_disk_req_check_error(r, ret, false)) {
> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>      }
>  
>      if (data->count > 0) {
> -        sector_num = ldq_be_p(&data->inbuf[0]);
> -        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> -        if (!check_lba_range(s, sector_num, nb_sectors)) {
> +        r->sector = ldq_be_p(&data->inbuf[0]);
> +        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> +        if (!check_lba_range(s, r->sector, r->sector_count)) {
>              scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>              goto done;
>          }
>  
>          r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
> -                                        sector_num * s->qdev.blocksize,
> -                                        nb_sectors * s->qdev.blocksize,
> +                                        r->sector * s->qdev.blocksize,
> +                                        r->sector_count * s->qdev.blocksize,

This looks to me like these are not necessarily in terms of 512-byte
sectors.  It doesn’t seem to make anything technically wrong, because
patch 7 takes that into account.

But it’s still weird if everything else in this file treats these fields
as being in terms of 512 byte sectors (and they are actually defined
this way in SCSIDiskReq).

Max

>                                          scsi_unmap_complete, data);
>          data->count--;
>          data->inbuf += 16;
>
Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
Posted by Anton Nefedov 6 years, 5 months ago 
On 12/8/2019 8:58 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> it allows to report it in the error handler
>>
>> 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 | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> (Sorry for the late reply :-/)
> 
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index e7e865ab3b..b43254103c 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>>   {
>>       SCSIDiskReq *r = data->r;
>>       SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> -    uint64_t sector_num;
>> -    uint32_t nb_sectors;
>>   
>>       assert(r->req.aiocb == NULL);
>>       if (scsi_disk_req_check_error(r, ret, false)) {
>> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>>       }
>>   
>>       if (data->count > 0) {
>> -        sector_num = ldq_be_p(&data->inbuf[0]);
>> -        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> -        if (!check_lba_range(s, sector_num, nb_sectors)) {
>> +        r->sector = ldq_be_p(&data->inbuf[0]);
>> +        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> +        if (!check_lba_range(s, r->sector, r->sector_count)) {
>>               scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>>               goto done;
>>           }
>>   
>>           r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>> -                                        sector_num * s->qdev.blocksize,
>> -                                        nb_sectors * s->qdev.blocksize,
>> +                                        r->sector * s->qdev.blocksize,
>> +                                        r->sector_count * s->qdev.blocksize,
> 
> This looks to me like these are not necessarily in terms of 512-byte
> sectors.  It doesn’t seem to make anything technically wrong, because
> patch 7 takes that into account.
> 
> But it’s still weird if everything else in this file treats these fields
> as being in terms of 512 byte sectors (and they are actually defined
> this way in SCSIDiskReq).
> 

Nice that you caught this, thanks! I guess variable names misled me

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.