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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature