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

[Max Reitz]

On 16.05.19 16:33, Anton Nefedov wrote:
> it allows to report it in the error handler
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
>  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;
>

signature.asc
Description: OpenPGP digital signature