Patchwork [2/2] ISCSI: Force scsi-generic for MMC with blank disks

login
register
mail settings
Submitter ronniesahlberg@gmail.com
Date Aug. 17, 2012, 2:36 a.m.
Message ID <1345170981-7738-3-git-send-email-ronniesahlberg@gmail.com>
Download mbox | patch
Permalink /patch/178135/
State New
Headers show

Comments

ronniesahlberg@gmail.com - Aug. 17, 2012, 2:36 a.m.
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(-)
Blue Swirl - Aug. 18, 2012, 2:23 p.m.
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
>
>
Paolo Bonzini - Aug. 18, 2012, 9:58 p.m.
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
ronniesahlberg@gmail.com - Aug. 18, 2012, 10:02 p.m.
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
ronniesahlberg@gmail.com - Aug. 18, 2012, 10:04 p.m.
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
Paolo Bonzini - Aug. 18, 2012, 10:16 p.m.
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
ronniesahlberg@gmail.com - Aug. 18, 2012, 10:19 p.m.
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
Paolo Bonzini - Aug. 18, 2012, 10:20 p.m.
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
Paolo Bonzini - Aug. 18, 2012, 10:41 p.m.
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
ronniesahlberg@gmail.com - Aug. 18, 2012, 11:44 p.m.
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
Paolo Bonzini - Aug. 19, 2012, 10:16 a.m.
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

Patch

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;