[Qemu-devel] [PATCH] Two trivial patches for iSCSI and blank DVDs

[Qemu-devel] [PATCH] Two trivial patches for iSCSI and blank DVDs

[Ronnie Sahlberg]

Kevin, List, Paolo

Please find two trivial patches to the iscsi block driver.

1, If the LUN contains a blank disk then the readcapacity10 data will
contain a ReturnedLogicalBlockAddress of 0. For this case this does not mean that the device contains one readable block and can be read from.
This is a special case for a blank disk that contains no readable blocks at all. This change makes bdrv_getlength()/iscsi_getlength() now rerurn a size of 0  for iSCSI LUNs that contain a blank disk.

2, If the iSCSI LUN contains a blank disk then we can not read or write to it using bdrv_*() functions since there is no bdrv api for writeable mmc devices.
For this case with a blank disk force the use of scsi-generic just like 
we do for tape and mediachanger devices.
This allows the guest to talk directly to the target LUN and write to/burn the disk.


I have confirmed that 

  dvdrecord -dao -ignsize -overburn dev=/dev/sg1 <some-file>.iso

works from a qemu guest to burn an iSCSI cdrom with these patches.

regards
ronnie sahlberg

[Qemu-devel] [PATCH 1/2] ISCSI: Set number of blocks to 0 for blank CDROM devices

[Ronnie Sahlberg]

The number of blocks of the device is used to compute the device size
in bdrv_getlength()/iscsi_getlength().
For MMC devices, the ReturnedLogicalBlockAddress in the READCAPACITY10
has a special meaning when it is 0.
In this case it does not mean that LBA 0 is the last accessible LBA,
and thus the device has 1 readable block, but instead it means that the
disc is blank and there are no readable blocks.

This change ensures that when the iSCSI LUN is loaded with a blank
DVD-R disk or similar that bdrv_getlength() will return the correct
size of the device as 0 bytes.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index bb9cf82..fb420ea 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -717,7 +717,12 @@ iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
     }
 
     itask->iscsilun->block_size = rc10->block_size;
-    itask->iscsilun->num_blocks = rc10->lba + 1;
+    if (rc10->lba == 0) {
+        /* blank disk loaded */
+        itask->iscsilun->num_blocks = 0;
+    } else {
+        itask->iscsilun->num_blocks = rc10->lba + 1;
+    }
     itask->bs->total_sectors    = itask->iscsilun->num_blocks *
                                itask->iscsilun->block_size / BDRV_SECTOR_SIZE ;
 
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 1/2] ISCSI: Set number of blocks to 0 for blank CDROM devices

[Paolo Bonzini]

Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
> The number of blocks of the device is used to compute the device size
> in bdrv_getlength()/iscsi_getlength().
> For MMC devices, the ReturnedLogicalBlockAddress in the READCAPACITY10
> has a special meaning when it is 0.
> In this case it does not mean that LBA 0 is the last accessible LBA,
> and thus the device has 1 readable block, but instead it means that the
> disc is blank and there are no readable blocks.
> 
> This change ensures that when the iSCSI LUN is loaded with a blank
> DVD-R disk or similar that bdrv_getlength() will return the correct
> size of the device as 0 bytes.
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index bb9cf82..fb420ea 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -717,7 +717,12 @@ iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      itask->iscsilun->block_size = rc10->block_size;
> -    itask->iscsilun->num_blocks = rc10->lba + 1;
> +    if (rc10->lba == 0) {
> +        /* blank disk loaded */
> +        itask->iscsilun->num_blocks = 0;
> +    } else {
> +        itask->iscsilun->num_blocks = rc10->lba + 1;
> +    }
>      itask->bs->total_sectors    = itask->iscsilun->num_blocks *
>                                 itask->iscsilun->block_size / BDRV_SECTOR_SIZE ;
>  
> 

Thanks, applied to SCSI branch.  We'll see whether this hits 1.2.

Paolo

Re: [Qemu-devel] [PATCH 1/2] ISCSI: Set number of blocks to 0 for blank CDROM devices

[ronnie sahlberg]

On Sun, Aug 19, 2012 at 7:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
>> The number of blocks of the device is used to compute the device size
>> in bdrv_getlength()/iscsi_getlength().
>> For MMC devices, the ReturnedLogicalBlockAddress in the READCAPACITY10
>> has a special meaning when it is 0.
>> In this case it does not mean that LBA 0 is the last accessible LBA,
>> and thus the device has 1 readable block, but instead it means that the
>> disc is blank and there are no readable blocks.
>>
>> This change ensures that when the iSCSI LUN is loaded with a blank
>> DVD-R disk or similar that bdrv_getlength() will return the correct
>> size of the device as 0 bytes.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>> ---
>>  block/iscsi.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index bb9cf82..fb420ea 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -717,7 +717,12 @@ iscsi_readcapacity10_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      itask->iscsilun->block_size = rc10->block_size;
>> -    itask->iscsilun->num_blocks = rc10->lba + 1;
>> +    if (rc10->lba == 0) {
>> +        /* blank disk loaded */
>> +        itask->iscsilun->num_blocks = 0;
>> +    } else {
>> +        itask->iscsilun->num_blocks = rc10->lba + 1;
>> +    }
>>      itask->bs->total_sectors    = itask->iscsilun->num_blocks *
>>                                 itask->iscsilun->block_size / BDRV_SECTOR_SIZE ;
>>
>>
>
> Thanks, applied to SCSI branch.  We'll see whether this hits 1.2.

This patch only matters for the use case of having empty/blank media
loaded into an iscsi cdrom.
I doubt being able to "burn" data to an iscsi drive is very important or urgent,
so it is fine with me to delay until post 1.2

regards
ronnie sahlberg

[Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Ronnie Sahlberg]

There is no bdrv_* API for the commands for burning a blank MMC disk
so when iSCSI LUNs are specified and the LUN is a MMC device with
0 available blocks. This is a blank disk so force scsi generic.

This allows the guest to talk directly to the target to burn data on
the disk.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index fb420ea..ca53afa 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1017,10 +1017,19 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     /* Medium changer or tape. We dont have any emulation for this so this must
      * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
      * to read from the device to guess the image format.
+     * MMC device with no blocks contain a blank disk so force them to use sg
+     * too.
      */
-    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
-        iscsilun->type == TYPE_TAPE) {
+    switch (iscsilun->type) {
+    case TYPE_ROM:
+        if (iscsilun->num_blocks > 0) {
+            break;
+        }
+    case TYPE_MEDIUM_CHANGER:
+    case TYPE_TAPE:
         bs->sg = 1;
+    default:
+        break;
     }
 
     ret = 0;
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Blue Swirl]

On Fri, Aug 17, 2012 at 2:36 AM, Ronnie Sahlberg
<ronniesahlberg@gmail.com> wrote:
> There is no bdrv_* API for the commands for burning a blank MMC disk
> so when iSCSI LUNs are specified and the LUN is a MMC device with
> 0 available blocks. This is a blank disk so force scsi generic.
>
> This allows the guest to talk directly to the target to burn data on
> the disk.
>
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index fb420ea..ca53afa 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1017,10 +1017,19 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>      /* Medium changer or tape. We dont have any emulation for this so this must
>       * be sg ioctl compatible. We force it to be sg, otherwise qemu will try
>       * to read from the device to guess the image format.
> +     * MMC device with no blocks contain a blank disk so force them to use sg
> +     * too.
>       */
> -    if (iscsilun->type == TYPE_MEDIUM_CHANGER ||
> -        iscsilun->type == TYPE_TAPE) {
> +    switch (iscsilun->type) {
> +    case TYPE_ROM:
> +        if (iscsilun->num_blocks > 0) {
> +            break;
> +        }

Please don't fall through without a comment.

> +    case TYPE_MEDIUM_CHANGER:
> +    case TYPE_TAPE:
>          bs->sg = 1;

Also here, but I'd add 'break' instead.

> +    default:
> +        break;
>      }
>
>      ret = 0;
> --
> 1.7.3.1
>
>

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Paolo Bonzini]

Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
> There is no bdrv_* API for the commands for burning a blank MMC disk
> so when iSCSI LUNs are specified and the LUN is a MMC device with
> 0 available blocks. This is a blank disk so force scsi generic.
> 
> This allows the guest to talk directly to the target to burn data on
> the disk.
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>

What happens without the patch?  It's ok that scsi-{hd,cd} does not
work, but do scsi-{block,generic} work without the patch?

Paolo

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[ronnie sahlberg]

On Sun, Aug 19, 2012 at 7:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
>> There is no bdrv_* API for the commands for burning a blank MMC disk
>> so when iSCSI LUNs are specified and the LUN is a MMC device with
>> 0 available blocks. This is a blank disk so force scsi generic.
>>
>> This allows the guest to talk directly to the target to burn data on
>> the disk.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>
> What happens without the patch?  It's ok that scsi-{hd,cd} does not
> work, but do scsi-{block,generic} work without the patch?
>

Neither of them work, basically because in
block.c:find_image_format()

if bs->sg is not set in

 if (bs->sg || !bdrv_is_inserted(bs)) {

then we continue to

  ret = bdrv_pread(bs, 0, buf, sizeof(buf));

which fails with an error.
So this patch is basically to prevent find_image_format() from trying
to read from a blank disk.

Maybe there is a better way to do this?



regards
ronnie sahlberg

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[ronnie sahlberg]

Ah,

This patch only affects the case when there is a blank / empty disk loaded.
It has no effect on when real *.iso images are loaded and the disk
contains data.

The use case to be able to "burn" to an iscsi cdrom is probably not
very urgent, so maybe it is best to delay this until post 1.2


regards
ronnie sahlberg


On Sun, Aug 19, 2012 at 8:02 AM, ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
> On Sun, Aug 19, 2012 at 7:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
>>> There is no bdrv_* API for the commands for burning a blank MMC disk
>>> so when iSCSI LUNs are specified and the LUN is a MMC device with
>>> 0 available blocks. This is a blank disk so force scsi generic.
>>>
>>> This allows the guest to talk directly to the target to burn data on
>>> the disk.
>>>
>>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>
>> What happens without the patch?  It's ok that scsi-{hd,cd} does not
>> work, but do scsi-{block,generic} work without the patch?
>>
>
> Neither of them work, basically because in
> block.c:find_image_format()
>
> if bs->sg is not set in
>
>  if (bs->sg || !bdrv_is_inserted(bs)) {
>
> then we continue to
>
>   ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>
> which fails with an error.
> So this patch is basically to prevent find_image_format() from trying
> to read from a blank disk.
>
> Maybe there is a better way to do this?
>
>
>
> regards
> ronnie sahlberg

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Paolo Bonzini]

Il 19/08/2012 00:02, ronnie sahlberg ha scritto:
> Neither of them work, basically because in
> block.c:find_image_format()
> 
> if bs->sg is not set in
> 
>  if (bs->sg || !bdrv_is_inserted(bs)) {
> 
> then we continue to
> 
>   ret = bdrv_pread(bs, 0, buf, sizeof(buf));
> 
> which fails with an error.
> So this patch is basically to prevent find_image_format() from trying
> to read from a blank disk.
> 
> Maybe there is a better way to do this?

Yeah, I think in this case find_image_format should just use raw.

Paolo

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[ronnie sahlberg]

On Sun, Aug 19, 2012 at 8:16 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 19/08/2012 00:02, ronnie sahlberg ha scritto:
>> Neither of them work, basically because in
>> block.c:find_image_format()
>>
>> if bs->sg is not set in
>>
>>  if (bs->sg || !bdrv_is_inserted(bs)) {
>>
>> then we continue to
>>
>>   ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>>
>> which fails with an error.
>> So this patch is basically to prevent find_image_format() from trying
>> to read from a blank disk.
>>
>> Maybe there is a better way to do this?
>
> Yeah, I think in this case find_image_format should just use raw.

Ok, so that is basically what the patch does. It forces bs->sg==true
so that we pick "raw" right there instead of trying to read from the
device.


So you are happy with the patch ?


regards
ronnie sahlberg

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Paolo Bonzini]

Il 19/08/2012 00:19, ronnie sahlberg ha scritto:
>> > Yeah, I think in this case find_image_format should just use raw.
> Ok, so that is basically what the patch does. It forces bs->sg==true
> so that we pick "raw" right there instead of trying to read from the
> device.
> 
> So you are happy with the patch ?

No, the solution should be the same that allows "touch ff + qemu-kvm
-hda ff" to work.  This is implemented here:

            if (ret >= 0 && ret < aiocb->aio_nbytes && aiocb->common.bs->growable) {
                /* A short read means that we have reached EOF. Pad the buffer
                 * with zeros for bytes after EOF. */
                iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret,
                           0, aiocb->aio_nbytes - ret);

                ret = aiocb->aio_nbytes;
            }

and block/iscsi.c should do the same.

Paolo

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Paolo Bonzini]

Il 19/08/2012 00:02, ronnie sahlberg ha scritto:
> On Sun, Aug 19, 2012 at 7:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
>>> There is no bdrv_* API for the commands for burning a blank MMC disk
>>> so when iSCSI LUNs are specified and the LUN is a MMC device with
>>> 0 available blocks. This is a blank disk so force scsi generic.
>>>
>>> This allows the guest to talk directly to the target to burn data on
>>> the disk.
>>>
>>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>
>> What happens without the patch?  It's ok that scsi-{hd,cd} does not
>> work, but do scsi-{block,generic} work without the patch?
>>
> 
> Neither of them work, basically because in
> block.c:find_image_format()
> 
> if bs->sg is not set in
> 
>  if (bs->sg || !bdrv_is_inserted(bs)) {
> 
> then we continue to
> 
>   ret = bdrv_pread(bs, 0, buf, sizeof(buf));
> 
> which fails with an error.
> So this patch is basically to prevent find_image_format() from trying
> to read from a blank disk.
> 
> Maybe there is a better way to do this?

Ah, BTW does using -drive ...,format=raw work, or does it hiccup later
again on something else?

Paolo

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[ronnie sahlberg]

On Sun, Aug 19, 2012 at 8:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 19/08/2012 00:02, ronnie sahlberg ha scritto:
>> On Sun, Aug 19, 2012 at 7:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 17/08/2012 04:36, Ronnie Sahlberg ha scritto:
>>>> There is no bdrv_* API for the commands for burning a blank MMC disk
>>>> so when iSCSI LUNs are specified and the LUN is a MMC device with
>>>> 0 available blocks. This is a blank disk so force scsi generic.
>>>>
>>>> This allows the guest to talk directly to the target to burn data on
>>>> the disk.
>>>>
>>>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>>
>>> What happens without the patch?  It's ok that scsi-{hd,cd} does not
>>> work, but do scsi-{block,generic} work without the patch?
>>>
>>
>> Neither of them work, basically because in
>> block.c:find_image_format()
>>
>> if bs->sg is not set in
>>
>>  if (bs->sg || !bdrv_is_inserted(bs)) {
>>
>> then we continue to
>>
>>   ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>>
>> which fails with an error.
>> So this patch is basically to prevent find_image_format() from trying
>> to read from a blank disk.
>>
>> Maybe there is a better way to do this?
>
> Ah, BTW does using -drive ...,format=raw work, or does it hiccup later
> again on something else?
>

format=raw works !

That then begs the question if would it be possible to force
format=raw always for iscsi devices?

A iscsi device as far as I can see would always just be a raw block
device and there would never be a "header" on such devices
so maybe that would be a solution?


regards
ronnie sahlberg

Re: [Qemu-devel] [PATCH 2/2] ISCSI: Force scsi-generic for MMC with blank disks

[Paolo Bonzini]

Il 19/08/2012 01:44, ronnie sahlberg ha scritto:
> format=raw works !
> 
> That then begs the question if would it be possible to force
> format=raw always for iscsi devices?
> 
> A iscsi device as far as I can see would always just be a raw block
> device and there would never be a "header" on such devices
> so maybe that would be a solution?

While it sounds weird, it is perfectly possible to host a qcow2 image on
an iSCSI LUN.  So the solution is to make iscsi behave the same as
raw-posix and raw-win32 (since they all sit at the same level).

Paolo