[Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Alexandre DERUMIER]

Hello,

I'm looking to use qemu-img convert to write to iscsi block device (iscsi://..)

As iscsi doesn't have  .bdrv_create, qemu-img convert hang on 


    /* Create the new image */
    ret = bdrv_create(drv, out_filename, param);
    if (ret < 0) {
        if (ret == -ENOTSUP) {
            error_report("Formatting not supported for file format '%s'",
                         out_fmt);
        } else if (ret == -EFBIG) {
            error_report("The image size is too large for file format '%s'",
                         out_fmt);
        } else {
            error_report("%s: error while converting %s: %s",
                         out_filename, out_fmt, strerror(-ret));
        }
        goto out;
    }




What is the best way to get it working ?


1)add a .bdrv_create in block/iscsi.c ? 

(like host_device block driver, only open/close the device and check if size if big enough)

block/raw-posix.c
.bdrv_create        = hdev_create,

static int hdev_create(const char *filename, QEMUOptionParameter *options)
{
    int fd;
    int ret = 0;
    struct stat stat_buf;
    int64_t total_size = 0;

    /* Read out options */
    while (options && options->name) {
        if (!strcmp(options->name, "size")) {
            total_size = options->value.n / BDRV_SECTOR_SIZE;
        }
        options++;
    }

    fd = qemu_open(filename, O_WRONLY | O_BINARY);
    if (fd < 0)
        return -errno;

    if (fstat(fd, &stat_buf) < 0)
        ret = -errno;
    else if (!S_ISBLK(stat_buf.st_mode) && !S_ISCHR(stat_buf.st_mode))
        ret = -ENODEV;
    else if (lseek(fd, 0, SEEK_END) < total_size * BDRV_SECTOR_SIZE)
        ret = -ENOSPC;

    qemu_close(fd);
    return ret;
}


2)or add a fallback in qemu-img, if bdrv_create doesn't exist, use bdrv_open to see if the backend device is pre-existing ?


Regards,

Alexandre Derumier

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Paolo Bonzini]

Il 25/10/2012 09:25, Alexandre DERUMIER ha scritto:
> What is the best way to get it working ?
> 
> 1)add a .bdrv_create in block/iscsi.c ? 
> 
> (like host_device block driver, only open/close the device and check if size if big enough)
> 
>     if (fstat(fd, &stat_buf) < 0)
>         ret = -errno;
>     else if (!S_ISBLK(stat_buf.st_mode) && !S_ISCHR(stat_buf.st_mode))
>         ret = -ENODEV;

I'm not even sure this S_ISBLK/S_ISCHR test is necessary.

>     else if (lseek(fd, 0, SEEK_END) < total_size * BDRV_SECTOR_SIZE)
>         ret = -ENOSPC;
> 
>     qemu_close(fd);
>     return ret;
> }
> 
> 
> 2)or add a fallback in qemu-img, if bdrv_create doesn't exist, use bdrv_open to see if the backend device is pre-existing ?

Or even add it in block.c, so that it also applies to live snapshots etc.

Paolo

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Kevin Wolf]

Am 25.10.2012 09:25, schrieb Alexandre DERUMIER:
> Hello,
> 
> I'm looking to use qemu-img convert to write to iscsi block device (iscsi://..)
> 
> As iscsi doesn't have  .bdrv_create, qemu-img convert hang on [...]
> 
> 
> What is the best way to get it working ?
> 
> 
> 1)add a .bdrv_create in block/iscsi.c ? 
> 
> (like host_device block driver, only open/close the device and check if size if big enough)

Yes, this is the right way.

Kevin

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Paolo Bonzini]

Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>> > 1)add a .bdrv_create in block/iscsi.c ? 
>> > 
>> > (like host_device block driver, only open/close the device and check if size if big enough)
> Yes, this is the right way.

Could it be a default implementation of .bdrv_create (i.e. something
you'd do in bdrv_create if the protocol doesn't have it)?

Paolo

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Kevin Wolf]

Am 25.10.2012 09:52, schrieb Paolo Bonzini:
> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>> 1)add a .bdrv_create in block/iscsi.c ? 
>>>>
>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>> Yes, this is the right way.
> 
> Could it be a default implementation of .bdrv_create (i.e. something
> you'd do in bdrv_create if the protocol doesn't have it)?

No, there are block drivers that really can't create images. They should
keep failing.

Kevin

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[ronnie sahlberg]

On Thu, Oct 25, 2012 at 1:02 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 25.10.2012 09:52, schrieb Paolo Bonzini:
>> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>>> 1)add a .bdrv_create in block/iscsi.c ?
>>>>>
>>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>>> Yes, this is the right way.
>>
>> Could it be a default implementation of .bdrv_create (i.e. something
>> you'd do in bdrv_create if the protocol doesn't have it)?
>
> No, there are block drivers that really can't create images. They should
> keep failing.

Technically, you can not create new LUNs via the iscsi protocol
itself, you can only access pre-existing luns
that have been created by some out-of-band method.

So basically, with iscsi you can only ever access already preexisting luns.

In that case I think requiring a .bdrv_create feels wrong. We are not
creating a new LUN here but just opening an already existing LUN.


What about changing bdrv_create() so that IF there is no .bdrv_create
method, then assume it might be a pre-existing file in which case we
fallback to use .bdrv_open instead.

regards
ronnie sahlberg

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Kevin Wolf]

Am 25.10.2012 15:41, schrieb ronnie sahlberg:
> On Thu, Oct 25, 2012 at 1:02 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 25.10.2012 09:52, schrieb Paolo Bonzini:
>>> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>>>> 1)add a .bdrv_create in block/iscsi.c ?
>>>>>>
>>>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>>>> Yes, this is the right way.
>>>
>>> Could it be a default implementation of .bdrv_create (i.e. something
>>> you'd do in bdrv_create if the protocol doesn't have it)?
>>
>> No, there are block drivers that really can't create images. They should
>> keep failing.
> 
> Technically, you can not create new LUNs via the iscsi protocol
> itself, you can only access pre-existing luns
> that have been created by some out-of-band method.
> 
> So basically, with iscsi you can only ever access already preexisting luns.
> 
> In that case I think requiring a .bdrv_create feels wrong. We are not
> creating a new LUN here but just opening an already existing LUN.

This is the very same as with host_device, which has always worked like
this. Obviously it can't create new block devices, but it can check that
the preexisting block device is suitable for the requested size and options.

> What about changing bdrv_create() so that IF there is no .bdrv_create
> method, then assume it might be a pre-existing file in which case we
> fallback to use .bdrv_open instead.

I don't think this is the correct behaviour for all block drivers. It's
wrong for any driver that would actually require a specific content of
the image file. For example let's overwrite an existing image:

$ touch /tmp/test.dmg # Or this could be a real DMG image as well
$ ./qemu-img create -f dmg /tmp/test.dmg 64M
Formatting '/tmp/test.dmg', fmt=dmg size=67108864
qemu-img: Formatting or formatting option not supported for file format
'dmg'

After your change it would appear to have succeeded, whereas of course
it still wouldn't have created the image.

Kevin

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Paolo Bonzini]

Il 25/10/2012 15:41, ronnie sahlberg ha scritto:
> On Thu, Oct 25, 2012 at 1:02 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 25.10.2012 09:52, schrieb Paolo Bonzini:
>>> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>>>> 1)add a .bdrv_create in block/iscsi.c ?
>>>>>>
>>>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>>>> Yes, this is the right way.
>>>
>>> Could it be a default implementation of .bdrv_create (i.e. something
>>> you'd do in bdrv_create if the protocol doesn't have it)?
>>
>> No, there are block drivers that really can't create images. They should
>> keep failing.
> 
> Technically, you can not create new LUNs via the iscsi protocol
> itself, you can only access pre-existing luns
> that have been created by some out-of-band method.
> 
> So basically, with iscsi you can only ever access already preexisting luns.
> 
> In that case I think requiring a .bdrv_create feels wrong. We are not
> creating a new LUN here but just opening an already existing LUN.

The problem is that bdrv_create is overloaded to mean both "create the
backing storage" and "format the image".  Only the latter applies to
iSCSI and, in general, as far as protocols are concerned bdrv_create is
usually a no-op.

However, Kevin said he prefers to have an explicit override of
bdrv_create for iSCSI.  Can you implement that?  (I'll then do the same
for NBD).

Paolo

> 
> What about changing bdrv_create() so that IF there is no .bdrv_create
> method, then assume it might be a pre-existing file in which case we
> fallback to use .bdrv_open instead.
> 
> regards
> ronnie sahlberg
> 

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[ronnie sahlberg]

On Thu, Oct 25, 2012 at 6:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/10/2012 15:41, ronnie sahlberg ha scritto:
>> On Thu, Oct 25, 2012 at 1:02 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 25.10.2012 09:52, schrieb Paolo Bonzini:
>>>> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>>>>> 1)add a .bdrv_create in block/iscsi.c ?
>>>>>>>
>>>>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>>>>> Yes, this is the right way.
>>>>
>>>> Could it be a default implementation of .bdrv_create (i.e. something
>>>> you'd do in bdrv_create if the protocol doesn't have it)?
>>>
>>> No, there are block drivers that really can't create images. They should
>>> keep failing.
>>
>> Technically, you can not create new LUNs via the iscsi protocol
>> itself, you can only access pre-existing luns
>> that have been created by some out-of-band method.
>>
>> So basically, with iscsi you can only ever access already preexisting luns.
>>
>> In that case I think requiring a .bdrv_create feels wrong. We are not
>> creating a new LUN here but just opening an already existing LUN.
>
> The problem is that bdrv_create is overloaded to mean both "create the
> backing storage" and "format the image".  Only the latter applies to
> iSCSI and, in general, as far as protocols are concerned bdrv_create is
> usually a no-op.
>
> However, Kevin said he prefers to have an explicit override of
> bdrv_create for iSCSI.  Can you implement that?  (I'll then do the same
> for NBD).
>

Yepp. Since there is consensus.
We can do a .bdrv_create for iscsi.c

regards
ronnie sahlberg

Re: [Qemu-devel] qemu-img convert with block driver without .bdrv_create (like iscsi)

[Alexandre DERUMIER]

Thanks guys !


I'll try to send a patch next week.


Regards,

Alexandre

----- Mail original -----

De: "ronnie sahlberg" <ronniesahlberg@gmail.com>
À: "Paolo Bonzini" <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Alexandre DERUMIER" <aderumier@odiso.com>, "qemu-devel" <qemu-devel@nongnu.org>
Envoyé: Jeudi 25 Octobre 2012 16:00:41
Objet: Re: qemu-img convert with block driver without .bdrv_create (like iscsi)

On Thu, Oct 25, 2012 at 6:58 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/10/2012 15:41, ronnie sahlberg ha scritto:
>> On Thu, Oct 25, 2012 at 1:02 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 25.10.2012 09:52, schrieb Paolo Bonzini:
>>>> Il 25/10/2012 09:46, Kevin Wolf ha scritto:
>>>>>>> 1)add a .bdrv_create in block/iscsi.c ?
>>>>>>>
>>>>>>> (like host_device block driver, only open/close the device and check if size if big enough)
>>>>> Yes, this is the right way.
>>>>
>>>> Could it be a default implementation of .bdrv_create (i.e. something
>>>> you'd do in bdrv_create if the protocol doesn't have it)?
>>>
>>> No, there are block drivers that really can't create images. They should
>>> keep failing.
>>
>> Technically, you can not create new LUNs via the iscsi protocol
>> itself, you can only access pre-existing luns
>> that have been created by some out-of-band method.
>>
>> So basically, with iscsi you can only ever access already preexisting luns.
>>
>> In that case I think requiring a .bdrv_create feels wrong. We are not
>> creating a new LUN here but just opening an already existing LUN.
>
> The problem is that bdrv_create is overloaded to mean both "create the
> backing storage" and "format the image". Only the latter applies to
> iSCSI and, in general, as far as protocols are concerned bdrv_create is
> usually a no-op.
>
> However, Kevin said he prefers to have an explicit override of
> bdrv_create for iSCSI. Can you implement that? (I'll then do the same
> for NBD).
>

Yepp. Since there is consensus.
We can do a .bdrv_create for iscsi.c

regards
ronnie sahlberg