Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

On 2020-06-25 21:25, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 8:14 PM
> ...
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 387503e11b..9e3002ddaf 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
>>      }
>>  }
>>
>> +typedef struct GetLbaStatusCBData {
>> +    uint32_t num_blocks;
>> +    uint32_t is_deallocated;
>> +    SCSIDiskReq *r;
>> +} GetLbaStatusCBData;
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret);
>> +
>> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
>> +{
>> +    SCSIDiskReq *r = data->r;
>> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> +    assert(r->req.aiocb == NULL);
>> +
>> +    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
>> +                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
>> +
>> +    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
>> +                                          r->req.cmd.lba * s->qdev.blocksize,
>> +                                          s->qdev.blocksize,
>> +                                          scsi_get_lba_status_complete, data);
>> +}
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret)
>> +{
>> +    GetLbaStatusCBData *data = opaque;
>> +    SCSIDiskReq *r = data->r;
>> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> +    assert(r->req.aiocb != NULL);
>> +    r->req.aiocb = NULL;
>> +
>> +    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> +    if (scsi_disk_req_check_error(r, ret, true)) {
>> +        g_free(data);
>> +        goto done;
>> +    }
>> +
>> +    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
>> +    scsi_req_unref(&r->req);
>> +    g_free(data);
>> +
>> +done:
>> +    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> +}
>> +
>> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
>> +{
>> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> +    GetLbaStatusCBData *data;
>> +    uint32_t *num_blocks;
>> +    uint32_t *is_deallocated;
>> +
>> +    data = g_new0(GetLbaStatusCBData, 1);
>> +    data->r = r;
>> +    num_blocks = &(data->num_blocks);
>> +    is_deallocated = &(data->is_deallocated);
>> +
>> +    scsi_req_ref(&r->req);
>> +    scsi_get_lba_status_complete_noio(data, 0);
> 
> scsi_get_lba_status_complete_noio() looks asynchronous. If the
> BlockDriver yields in .bdrv_co_block_status() then the operation has not
> completed yet when scsi_get_lba_status_complete_noio() returns. It is
> not safe to access the GetLbaStatusCBData data until the async operation
> is complete.
> 
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
> 
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
> the completion function before g_free(data) is called.

Emm, I need to dig into the code for better understanding your suggestion.
I used to think that I already completely make 'scsi_get_lba_status_complete_noio'
asynchronous.

>> +
>> +    /*
>> +     * 8 + 16 is the length in bytes of response header and
>> +     * one LBA status descriptor
>> +     */
>> +    memset(outbuf, 0, 8 + 16);
>> +    outbuf[3] = 20;
>> +    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
>> +    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
>> +    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
>> +    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
>> +    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
>> +    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
>> +    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
>> +    outbuf[15] = req->cmd.lba & 0xff;
>> +    outbuf[16] = (*num_blocks >> 24) & 0xff;
>> +    outbuf[17] = (*num_blocks >> 16) & 0xff;
>> +    outbuf[18] = (*num_blocks >> 8) & 0xff;
>> +    outbuf[19] = *num_blocks & 0xff;
>> +    outbuf[20] = *is_deallocated ? 1 : 0;
> 
> SCSI defines 3 values and QEMU can represent all of them:
> 
> 0 - mapped or unknown
> 1 - deallocated
> 2 - anchored
> 
> See the BDRV_BLOCK_* constants in include/block/block.h. The
> is_deallocated boolean is not enough to represent this state, but the
> bdrv_block_status() return value can be used instead.

I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',

I'd like to use these combinations to analyze the bdrv_block_status() return value:
'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
'anchored':    BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
Am I right?


>> +}
>> +
>>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>>  {
>>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>>
>>              /* Protection, exponent and lowest lba field left blank. */
>>              break;
>> +        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
>> +            if (req->cmd.lba > s->qdev.max_lba) {
>> +                goto illegal_lba;
>> +            }
>> +            scsi_disk_emulate_get_lba_status(req, outbuf);
>> +            r->iov.iov_len = req->cmd.xfer;
>> +            return r->iov.iov_len;
> 
> Is there something tricky going on here with iov_len that prevents us
> from using break here and sharing the functions normal return code path?

Using 'break' here and following the normal return code path cause the assertion
'assert(!r->req.aiocb)'.

In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
I borrowed this idea to avoid the assertion.

>>          }
>>          trace_scsi_disk_emulate_command_SAI_unsupported();
>>          goto illegal_request;
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index 878b4c3581..645014fb0b 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -38,6 +38,7 @@ enum BlockAcctType {
>>      BLOCK_ACCT_WRITE,
>>      BLOCK_ACCT_FLUSH,
>>      BLOCK_ACCT_UNMAP,
>> +    BLOCK_ACCT_GET_LBA_STATUS,
>>      BLOCK_MAX_IOTYPE,
>>  };
>>
>> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
>> index 874176019e..b18377b214 100644
>> --- a/include/scsi/constants.h
>> +++ b/include/scsi/constants.h
>> @@ -154,6 +154,7 @@
>>   * SERVICE ACTION IN subcodes
>>   */
>>  #define SAI_READ_CAPACITY_16  0x10
>> +#define SAI_GET_LBA_STATUS    0x12
>>
>>  /*
>>   * READ POSITION service action codes
>> --
>> 2.26.0
>>

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

On Thu, Jun 25, 2020 at 01:31:56PM +0000, Lin Ma wrote:
> On 2020-06-25 21:25, Lin Ma wrote:
> >> +    /*
> >> +     * 8 + 16 is the length in bytes of response header and
> >> +     * one LBA status descriptor
> >> +     */
> >> +    memset(outbuf, 0, 8 + 16);
> >> +    outbuf[3] = 20;
> >> +    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> >> +    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> >> +    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> >> +    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> >> +    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> >> +    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> >> +    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> >> +    outbuf[15] = req->cmd.lba & 0xff;
> >> +    outbuf[16] = (*num_blocks >> 24) & 0xff;
> >> +    outbuf[17] = (*num_blocks >> 16) & 0xff;
> >> +    outbuf[18] = (*num_blocks >> 8) & 0xff;
> >> +    outbuf[19] = *num_blocks & 0xff;
> >> +    outbuf[20] = *is_deallocated ? 1 : 0;
> > 
> > SCSI defines 3 values and QEMU can represent all of them:
> > 
> > 0 - mapped or unknown
> > 1 - deallocated
> > 2 - anchored
> > 
> > See the BDRV_BLOCK_* constants in include/block/block.h. The
> > is_deallocated boolean is not enough to represent this state, but the
> > bdrv_block_status() return value can be used instead.
> 
> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
> 
> I'd like to use these combinations to analyze the bdrv_block_status() return value:
> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
> 'anchored':    BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
> Am I right?

My understanding is that the SCSI status are mapped to QEMU block status
as follows:

allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
deallocated: !BDRV_BLOCK_DATA

I have CCed Eric Blake, who is familiar with block status.

> >> +}
> >> +
> >>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >>  {
> >>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> >> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >>
> >>              /* Protection, exponent and lowest lba field left blank. */
> >>              break;
> >> +        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> >> +            if (req->cmd.lba > s->qdev.max_lba) {
> >> +                goto illegal_lba;
> >> +            }
> >> +            scsi_disk_emulate_get_lba_status(req, outbuf);
> >> +            r->iov.iov_len = req->cmd.xfer;
> >> +            return r->iov.iov_len;
> > 
> > Is there something tricky going on here with iov_len that prevents us
> > from using break here and sharing the functions normal return code path?
> 
> Using 'break' here and following the normal return code path cause the assertion
> 'assert(!r->req.aiocb)'.
> 
> In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in function
> scsi_disk_emulate_command, It causes the same assertion if no this 'return' statement.
> I borrowed this idea to avoid the assertion.

The assertion shows that this patch isn't asynchronous. I think the
assertion will pass once you convert GET_LBA_STATUS to async and then
a break statement will work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Paolo Bonzini]

On 29/06/20 12:39, Stefan Hajnoczi wrote:
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored':    BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
> 
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
> deallocated: !BDRV_BLOCK_DATA

I agree except that I wouldn't test BDRV_BLOCK_OFFSET_VALID.  For
example a compressed cluster would still be reported as allocated even
if BDRV_BLOCK_OFFSET_VALID is cleared.

Paolo

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Eric Blake]

On 6/29/20 5:39 AM, Stefan Hajnoczi wrote:

>>> SCSI defines 3 values and QEMU can represent all of them:
>>>
>>> 0 - mapped or unknown
>>> 1 - deallocated
>>> 2 - anchored
>>>
>>> See the BDRV_BLOCK_* constants in include/block/block.h. The
>>> is_deallocated boolean is not enough to represent this state, but the
>>> bdrv_block_status() return value can be used instead.
>>
>> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
>> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
>>
>> I'd like to use these combinations to analyze the bdrv_block_status() return value:
>> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ZERO
>> 'anchored':    BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
>> Am I right?
> 
> My understanding is that the SCSI status are mapped to QEMU block status
> as follows:
> 
> allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID

I haven't read the scsi code, but as written, I would assume that any 
portion of the block status with this result has the following properties:
- reading sees guest-visible data; we cannot guarantee whether it is all 
zero, and the data reserves actual space

> anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID

- reading sees all zeros, but the disk has reserved actual space such 
that future writes will not run out of space

> deallocated: !BDRV_BLOCK_DATA

- reading does not see data; future writes will have to reserve space. 
I would expect this option might also have '| BDRV_BLOCK_ZERO' depending 
on whether this particular scsi device guarantees that reads of an 
unallocated portion see all zeros instead of unspecified garbage.

> 
> I have CCed Eric Blake, who is familiar with block status.

Adding Vladimir in CC as well, as he has pending patches that try to 
clear up the use of BDRV_BLOCK_* constants with regards to two different 
queries:
- allocation implies that data comes from this layer of a backing chain, 
rather than deferring to a backing image
- allocation implies that storage is reserved (that is, not sparse)

It sounds like we are trying to represent the second question for scsi 
(namely, the same question that gets answered by lseek(SEEK_HOLE) for 
POSIX files), and not the first (namely, the question answered for qcow2 
images).


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Paolo Bonzini]

On 29/06/20 18:02, Eric Blake wrote:
> - allocation implies that data comes from this layer of a backing chain, rather than deferring to a backing image 
> 
> - allocation implies that storage is reserved (that is, not sparse)
> 
> It sounds like we are trying to represent the second question for scsi
> (namely, the same question that gets answered by lseek(SEEK_HOLE) for
> POSIX files), and not the first (namely, the question answered for qcow2
> images).
> 

Yes, SCSI does not know about layers.

Paolo

[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.


In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0000000000000400  blocks: 32  deallocated

Lin Ma (3):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c          | 38 ++++++++++++++
 block/io.c                     | 43 ++++++++++++++++
 hw/scsi/scsi-disk.c            | 90 ++++++++++++++++++++++++++++++++++
 include/block/accounting.h     |  1 +
 include/block/block_int.h      |  5 ++
 include/scsi/constants.h       |  1 +
 include/sysemu/block-backend.h |  2 +
 7 files changed, 180 insertions(+)

-- 
2.26.0

[PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma <lma@suse.com>
---
 hw/scsi/scsi-disk.c        | 90 ++++++++++++++++++++++++++++++++++++++
 include/block/accounting.h |  1 +
 include/scsi/constants.h   |  1 +
 3 files changed, 92 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
     }
 }
 
+typedef struct GetLbaStatusCBData {
+    uint32_t num_blocks;
+    uint32_t is_deallocated;
+    SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
+{
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb == NULL);
+
+    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+                                          r->req.cmd.lba * s->qdev.blocksize,
+                                          s->qdev.blocksize,
+                                          scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+    GetLbaStatusCBData *data = opaque;
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+
+    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+    if (scsi_disk_req_check_error(r, ret, true)) {
+        g_free(data);
+        goto done;
+    }
+
+    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+    scsi_req_unref(&r->req);
+    g_free(data);
+
+done:
+    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    GetLbaStatusCBData *data;
+    uint32_t *num_blocks;
+    uint32_t *is_deallocated;
+
+    data = g_new0(GetLbaStatusCBData, 1);
+    data->r = r;
+    num_blocks = &(data->num_blocks);
+    is_deallocated = &(data->is_deallocated);
+
+    scsi_req_ref(&r->req);
+    scsi_get_lba_status_complete_noio(data, 0);
+
+    /*
+     * 8 + 16 is the length in bytes of response header and
+     * one LBA status descriptor
+     */
+    memset(outbuf, 0, 8 + 16);
+    outbuf[3] = 20;
+    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+    outbuf[15] = req->cmd.lba & 0xff;
+    outbuf[16] = (*num_blocks >> 24) & 0xff;
+    outbuf[17] = (*num_blocks >> 16) & 0xff;
+    outbuf[18] = (*num_blocks >> 8) & 0xff;
+    outbuf[19] = *num_blocks & 0xff;
+    outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 
             /* Protection, exponent and lowest lba field left blank. */
             break;
+        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+            if (req->cmd.lba > s->qdev.max_lba) {
+                goto illegal_lba;
+            }
+            scsi_disk_emulate_get_lba_status(req, outbuf);
+            r->iov.iov_len = req->cmd.xfer;
+            return r->iov.iov_len;
         }
         trace_scsi_disk_emulate_command_SAI_unsupported();
         goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
     BLOCK_ACCT_WRITE,
     BLOCK_ACCT_FLUSH,
     BLOCK_ACCT_UNMAP,
+    BLOCK_ACCT_GET_LBA_STATUS,
     BLOCK_MAX_IOTYPE,
 };
 
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS    0x12
 
 /*
  * READ POSITION service action codes
-- 
2.26.0

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 6163 bytes --]

On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  hw/scsi/scsi-disk.c        | 90 ++++++++++++++++++++++++++++++++++++++
>  include/block/accounting.h |  1 +
>  include/scsi/constants.h   |  1 +
>  3 files changed, 92 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 387503e11b..9e3002ddaf 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
>      }
>  }
>  
> +typedef struct GetLbaStatusCBData {
> +    uint32_t num_blocks;
> +    uint32_t is_deallocated;
> +    SCSIDiskReq *r;
> +} GetLbaStatusCBData;
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret);
> +
> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
> +{
> +    SCSIDiskReq *r = data->r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +    assert(r->req.aiocb == NULL);
> +
> +    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> +                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
> +
> +    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> +                                          r->req.cmd.lba * s->qdev.blocksize,
> +                                          s->qdev.blocksize,
> +                                          scsi_get_lba_status_complete, data);
> +}
> +
> +static void scsi_get_lba_status_complete(void *opaque, int ret)
> +{
> +    GetLbaStatusCBData *data = opaque;
> +    SCSIDiskReq *r = data->r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +    assert(r->req.aiocb != NULL);
> +    r->req.aiocb = NULL;
> +
> +    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> +    if (scsi_disk_req_check_error(r, ret, true)) {
> +        g_free(data);
> +        goto done;
> +    }
> +
> +    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
> +    scsi_req_unref(&r->req);
> +    g_free(data);
> +
> +done:
> +    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> +}
> +
> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
> +{
> +    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +    GetLbaStatusCBData *data;
> +    uint32_t *num_blocks;
> +    uint32_t *is_deallocated;
> +
> +    data = g_new0(GetLbaStatusCBData, 1);
> +    data->r = r;
> +    num_blocks = &(data->num_blocks);
> +    is_deallocated = &(data->is_deallocated);
> +
> +    scsi_req_ref(&r->req);
> +    scsi_get_lba_status_complete_noio(data, 0);

scsi_get_lba_status_complete_noio() looks asynchronous. If the
BlockDriver yields in .bdrv_co_block_status() then the operation has not
completed yet when scsi_get_lba_status_complete_noio() returns. It is
not safe to access the GetLbaStatusCBData data until the async operation
is complete.

Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
use-after-free when *num_blocks and *is_deallocated are accessed.

These issues can be solved by making this code asynchronous (similar to
read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
the completion function before g_free(data) is called.

> +
> +    /*
> +     * 8 + 16 is the length in bytes of response header and
> +     * one LBA status descriptor
> +     */
> +    memset(outbuf, 0, 8 + 16);
> +    outbuf[3] = 20;
> +    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> +    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> +    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> +    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> +    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> +    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> +    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> +    outbuf[15] = req->cmd.lba & 0xff;
> +    outbuf[16] = (*num_blocks >> 24) & 0xff;
> +    outbuf[17] = (*num_blocks >> 16) & 0xff;
> +    outbuf[18] = (*num_blocks >> 8) & 0xff;
> +    outbuf[19] = *num_blocks & 0xff;
> +    outbuf[20] = *is_deallocated ? 1 : 0;

SCSI defines 3 values and QEMU can represent all of them:

0 - mapped or unknown
1 - deallocated
2 - anchored

See the BDRV_BLOCK_* constants in include/block/block.h. The
is_deallocated boolean is not enough to represent this state, but the
bdrv_block_status() return value can be used instead.

> +}
> +
>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>  {
>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> @@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
>  
>              /* Protection, exponent and lowest lba field left blank. */
>              break;
> +        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> +            if (req->cmd.lba > s->qdev.max_lba) {
> +                goto illegal_lba;
> +            }
> +            scsi_disk_emulate_get_lba_status(req, outbuf);
> +            r->iov.iov_len = req->cmd.xfer;
> +            return r->iov.iov_len;

Is there something tricky going on here with iov_len that prevents us
from using break here and sharing the functions normal return code path?

>          }
>          trace_scsi_disk_emulate_command_SAI_unsupported();
>          goto illegal_request;
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 878b4c3581..645014fb0b 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -38,6 +38,7 @@ enum BlockAcctType {
>      BLOCK_ACCT_WRITE,
>      BLOCK_ACCT_FLUSH,
>      BLOCK_ACCT_UNMAP,
> +    BLOCK_ACCT_GET_LBA_STATUS,
>      BLOCK_MAX_IOTYPE,
>  };
>  
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 874176019e..b18377b214 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -154,6 +154,7 @@
>   * SERVICE ACTION IN subcodes
>   */
>  #define SAI_READ_CAPACITY_16  0x10
> +#define SAI_GET_LBA_STATUS    0x12
>  
>  /*
>   * READ POSITION service action codes
> -- 
> 2.26.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.


In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0000000000000400  blocks: 32  deallocated

Lin Ma (3):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c          | 38 ++++++++++++++
 block/io.c                     | 43 ++++++++++++++++
 hw/scsi/scsi-disk.c            | 90 ++++++++++++++++++++++++++++++++++
 include/block/accounting.h     |  1 +
 include/block/block_int.h      |  5 ++
 include/scsi/constants.h       |  1 +
 include/sysemu/block-backend.h |  2 +
 7 files changed, 180 insertions(+)

-- 
2.26.0

[PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma <lma@suse.com>
---
 hw/scsi/scsi-disk.c        | 90 ++++++++++++++++++++++++++++++++++++++
 include/block/accounting.h |  1 +
 include/scsi/constants.h   |  1 +
 3 files changed, 92 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
     }
 }
 
+typedef struct GetLbaStatusCBData {
+    uint32_t num_blocks;
+    uint32_t is_deallocated;
+    SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int ret)
+{
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb == NULL);
+
+    block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+                     s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+    r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+                                          r->req.cmd.lba * s->qdev.blocksize,
+                                          s->qdev.blocksize,
+                                          scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+    GetLbaStatusCBData *data = opaque;
+    SCSIDiskReq *r = data->r;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+    assert(r->req.aiocb != NULL);
+    r->req.aiocb = NULL;
+
+    aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+    if (scsi_disk_req_check_error(r, ret, true)) {
+        g_free(data);
+        goto done;
+    }
+
+    block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
+    scsi_req_unref(&r->req);
+    g_free(data);
+
+done:
+    aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+    SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+    GetLbaStatusCBData *data;
+    uint32_t *num_blocks;
+    uint32_t *is_deallocated;
+
+    data = g_new0(GetLbaStatusCBData, 1);
+    data->r = r;
+    num_blocks = &(data->num_blocks);
+    is_deallocated = &(data->is_deallocated);
+
+    scsi_req_ref(&r->req);
+    scsi_get_lba_status_complete_noio(data, 0);
+
+    /*
+     * 8 + 16 is the length in bytes of response header and
+     * one LBA status descriptor
+     */
+    memset(outbuf, 0, 8 + 16);
+    outbuf[3] = 20;
+    outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+    outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+    outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+    outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+    outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+    outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+    outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+    outbuf[15] = req->cmd.lba & 0xff;
+    outbuf[16] = (*num_blocks >> 24) & 0xff;
+    outbuf[17] = (*num_blocks >> 16) & 0xff;
+    outbuf[18] = (*num_blocks >> 8) & 0xff;
+    outbuf[19] = *num_blocks & 0xff;
+    outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 
             /* Protection, exponent and lowest lba field left blank. */
             break;
+        } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+            if (req->cmd.lba > s->qdev.max_lba) {
+                goto illegal_lba;
+            }
+            scsi_disk_emulate_get_lba_status(req, outbuf);
+            r->iov.iov_len = req->cmd.xfer;
+            return r->iov.iov_len;
         }
         trace_scsi_disk_emulate_command_SAI_unsupported();
         goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
     BLOCK_ACCT_WRITE,
     BLOCK_ACCT_FLUSH,
     BLOCK_ACCT_UNMAP,
+    BLOCK_ACCT_GET_LBA_STATUS,
     BLOCK_MAX_IOTYPE,
 };
 
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS    0x12
 
 /*
  * READ POSITION service action codes
-- 
2.26.0