[Qemu-devel] RFC: raw device support for block device targets

[Qemu-devel] RFC: raw device support for block device targets

[Alex Bligh]

qemu-img convert appears to support block devices as input, but not
as output. That is irritating, as when using qemu-img convert to
convert qcow to raw on a block partition, an intermediate file has
to be used, which slows things down and pointlessly uses disk space.

The problem is that ftruncate() is being called on the output file
in order ensure it is sufficiently large, and this fails on
block devices.

I appreciate there may be other calls that fail depending on the
input file format, but these will presumably be error checked
at the time.

Is it therefore worth skipping the ftruncate() if the block device
is large enough, and at least attempting to proceed further? Something
like the following (not-even compile tested) patch?

-- 
Alex Bligh

Signed-Off-By: Alex Bligh <alex@alex.org.uk>

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2ee5d69..be9a371 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -573,9 +573,29 @@ static int raw_create(const char *filename, 
QEMUOptionParameter *options)
     if (fd < 0) {
         result = -errno;
     } else {
+
+        struct stat sb;
+
+        if (-1 == fstat(dest, &sb)) {
+            result = -errno;
+            goto close;
+        }
+
+        /* block devices do not support truncate. If the device is large
+           enough, then it will do */
+
+        if (S_ISBLK(sb.st_mode)) {
+            /* divide to prevent overflow */
+            if (sb.st_size / BDRV_SECTOR_SIZE < total_size)
+                result = -ENOSPC;
+            goto close;
+        }
+
         if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
             result = -errno;
         }
+
+    close:
         if (close(fd) != 0) {
             result = -errno;
         }

Re: [Qemu-devel] RFC: raw device support for block device targets

[Christoph Hellwig]

On Tue, Dec 06, 2011 at 04:20:56PM +0000, Alex Bligh wrote:
> qemu-img convert appears to support block devices as input, but not
> as output. That is irritating, as when using qemu-img convert to
> convert qcow to raw on a block partition, an intermediate file has
> to be used, which slows things down and pointlessly uses disk space.
>
> The problem is that ftruncate() is being called on the output file
> in order ensure it is sufficiently large, and this fails on
> block devices.
>
> I appreciate there may be other calls that fail depending on the
> input file format, but these will presumably be error checked
> at the time.
>
> Is it therefore worth skipping the ftruncate() if the block device
> is large enough, and at least attempting to proceed further? Something
> like the following (not-even compile tested) patch?

It probably should share the code with raw_truncate.  That is factor
a new raw_truncate_common that is equivalent to raw_truncate but
takes a file descriptor instead of a block driver state and then use
it for both raw_truncate and raw_create.

Re: [Qemu-devel] RFC: raw device support for block device targets

[Alex Bligh]

--On 6 December 2011 19:42:26 +0100 Christoph Hellwig <hch@lst.de> wrote:

>> Is it therefore worth skipping the ftruncate() if the block device
>> is large enough, and at least attempting to proceed further? Something
>> like the following (not-even compile tested) patch?
>
> It probably should share the code with raw_truncate.  That is factor
> a new raw_truncate_common that is equivalent to raw_truncate but
> takes a file descriptor instead of a block driver state and then use
> it for both raw_truncate and raw_create.

You are of course correct, not least because raw_getlength() illustrates
that not every OS supports the method I suggested for length checking.
However, this makes things much more intrusive. Essentially you need
a raw_ftruncate that takes an fd, and a raw_fgetlength that takes an
fd, and that means modifying each of the OS implementations for the
latter. I've done them (blind) save for the reference to cdrom_reopen(),
sometimes by deleting fd_open(). I'm not quite sure why that's needed
there; perhaps doing it in raw_truncate and raw_getlength would
be sufficient.

Again, this is compile tested only.

-- 
Alex Bligh

Signed-Off-By: Alex Bligh <alex@alex.org.uk>

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2ee5d69..e67efa9 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,6 +138,7 @@ typedef struct BDRVRawState {

 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
+static int64_t raw_fgetlength(int fd);

 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 static int cdrom_reopen(BlockDriverState *bs);
@@ -379,21 +380,20 @@ static void raw_close(BlockDriverState *bs)
     }
 }

-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_ftruncate(int fd, int64_t offset)
 {
-    BDRVRawState *s = bs->opaque;
     struct stat st;

-    if (fstat(s->fd, &st)) {
+    if (fstat(fd, &st)) {
         return -errno;
     }

     if (S_ISREG(st.st_mode)) {
-        if (ftruncate(s->fd, offset) < 0) {
+        if (ftruncate(fd, offset) < 0) {
             return -errno;
         }
     } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
-       if (offset > raw_getlength(bs)) {
+       if (offset > raw_fgetlength(fd)) {
            return -EINVAL;
        }
     } else {
@@ -403,11 +403,16 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }

-#ifdef __OpenBSD__
-static int64_t raw_getlength(BlockDriverState *bs)
+
+static int raw_truncate(BlockDriverState *bs, int64_t offset)
 {
     BDRVRawState *s = bs->opaque;
-    int fd = s->fd;
+    return raw_ftruncate(s->fd, offset);
+}
+
+#ifdef __OpenBSD__
+static int64_t raw_fgetlength(int fd)
+{
     struct stat st;

     if (fstat(fd, &st))
@@ -423,10 +428,8 @@ static int64_t raw_getlength(BlockDriverState *bs)
         return st.st_size;
 }
 #elif defined(__NetBSD__)
-static int64_t raw_getlength(BlockDriverState *bs)
+static int64_t raw_fgetlength(int fd)
 {
-    BDRVRawState *s = bs->opaque;
-    int fd = s->fd;
     struct stat st;

     if (fstat(fd, &st))
@@ -448,21 +451,15 @@ static int64_t raw_getlength(BlockDriverState *bs)
         return st.st_size;
 }
 #elif defined(__sun__)
-static int64_t raw_getlength(BlockDriverState *bs)
+static int64_t raw_fgetlength(int fd)
 {
-    BDRVRawState *s = bs->opaque;
     struct dk_minfo minfo;
     int ret;

-    ret = fd_open(bs);
-    if (ret < 0) {
-        return ret;
-    }
-
     /*
      * Use the DKIOCGMEDIAINFO ioctl to read the size.
      */
-    ret = ioctl(s->fd, DKIOCGMEDIAINFO, &minfo);
+    ret = ioctl(fd, DKIOCGMEDIAINFO, &minfo);
     if (ret != -1) {
         return minfo.dki_lbsize * minfo.dki_capacity;
     }
@@ -471,13 +468,11 @@ static int64_t raw_getlength(BlockDriverState *bs)
      * There are reports that lseek on some devices fails, but
      * irc discussion said that contingency on contingency was overkill.
      */
-    return lseek(s->fd, 0, SEEK_END);
+    return lseek(fd, 0, SEEK_END);
 }
 #elif defined(CONFIG_BSD)
-static int64_t raw_getlength(BlockDriverState *bs)
+static int64_t raw_fgetlength(int fd)
 {
-    BDRVRawState *s = bs->opaque;
-    int fd = s->fd;
     int64_t size;
     struct stat sb;
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
@@ -485,10 +480,6 @@ static int64_t raw_getlength(BlockDriverState *bs)
 #endif
     int ret;

-    ret = fd_open(bs);
-    if (ret < 0)
-        return ret;
-
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
 again:
 #endif
@@ -529,19 +520,17 @@ again:
     return size;
 }
 #else
+static int64_t raw_fgetlength(int fd)
+{
+    return lseek(fd, 0, SEEK_END);
+}
+#endif
+
 static int64_t raw_getlength(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    int ret;
-
-    ret = fd_open(bs);
-    if (ret < 0) {
-        return ret;
-    }
-
-    return lseek(s->fd, 0, SEEK_END);
+    return raw_fgetlength(s->fd);
 }
-#endif

 static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
 {
@@ -573,7 +562,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     if (fd < 0) {
         result = -errno;
     } else {
-        if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
+        if (raw_ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
             result = -errno;
         }
         if (close(fd) != 0) {

Re: [Qemu-devel] RFC: raw device support for block device targets

[Kevin Wolf]

Am 06.12.2011 17:20, schrieb Alex Bligh:
> qemu-img convert appears to support block devices as input, but not
> as output. That is irritating, as when using qemu-img convert to
> convert qcow to raw on a block partition, an intermediate file has
> to be used, which slows things down and pointlessly uses disk space.
> 
> The problem is that ftruncate() is being called on the output file
> in order ensure it is sufficiently large, and this fails on
> block devices.
> 
> I appreciate there may be other calls that fail depending on the
> input file format, but these will presumably be error checked
> at the time.
> 
> Is it therefore worth skipping the ftruncate() if the block device
> is large enough, and at least attempting to proceed further? Something
> like the following (not-even compile tested) patch?

Creating an image on a block device shouldn't even call raw_create(),
but only hdev_create(), which doesn't try to truncate the device, but
just uses lseek to make sure that it's large enough.

Which qemu version are you using and what's your command line?

Kevin

Re: [Qemu-devel] RFC: raw device support for block device targets

[Alex Bligh]

--On 8 December 2011 13:40:57 +0100 Kevin Wolf <kwolf@redhat.com> wrote:

>> qemu-img convert appears to support block devices as input, but not
>> as output. That is irritating, as when using qemu-img convert to
>> convert qcow to raw on a block partition, an intermediate file has
>> to be used, which slows things down and pointlessly uses disk space.
>>
>> The problem is that ftruncate() is being called on the output file
>> in order ensure it is sufficiently large, and this fails on
>> block devices.
...
>
> Creating an image on a block device shouldn't even call raw_create(),
> but only hdev_create(), which doesn't try to truncate the device, but
> just uses lseek to make sure that it's large enough.
>
> Which qemu version are you using and what's your command line?

I was testing on:

amb@alex-test:~$ qemu-img --version
qemu-img version 0.12.3, Copyright (c) 2004-2008 Fabrice Bellard

though the patch was against current trunk.

Command line simply:

qemu-img convert -O raw test.qcow /dev/xyz

Fails on ftruncate() as verified with strace.

I must admit I only 'tested' on trunk by reading the source.

-- 
Alex Bligh

Re: [Qemu-devel] RFC: raw device support for block device targets

[Kevin Wolf]

Am 11.12.2011 11:45, schrieb Alex Bligh:
> 
> 
> --On 8 December 2011 13:40:57 +0100 Kevin Wolf <kwolf@redhat.com> wrote:
> 
>>> qemu-img convert appears to support block devices as input, but not
>>> as output. That is irritating, as when using qemu-img convert to
>>> convert qcow to raw on a block partition, an intermediate file has
>>> to be used, which slows things down and pointlessly uses disk space.
>>>
>>> The problem is that ftruncate() is being called on the output file
>>> in order ensure it is sufficiently large, and this fails on
>>> block devices.
> ...
>>
>> Creating an image on a block device shouldn't even call raw_create(),
>> but only hdev_create(), which doesn't try to truncate the device, but
>> just uses lseek to make sure that it's large enough.
>>
>> Which qemu version are you using and what's your command line?
> 
> I was testing on:
> 
> amb@alex-test:~$ qemu-img --version
> qemu-img version 0.12.3, Copyright (c) 2004-2008 Fabrice Bellard

That's the problem. It should work since 0.13.

Kevin

Re: [Qemu-devel] RFC: raw device support for block device targets

[Alex Bligh]

--On 12 December 2011 10:48:43 +0100 Kevin Wolf <kwolf@redhat.com> wrote:

>> I was testing on:
>>
>> amb@alex-test:~$ qemu-img --version
>> qemu-img version 0.12.3, Copyright (c) 2004-2008 Fabrice Bellard
>
> That's the problem. It should work since 0.13.

Thanks

-- 
Alex Bligh