[Qemu-devel] No error report when using the qemu-img.exe to convert a disk to vmdk format which is saved on a disk that has no more space

[Qemu-devel] No error report when using the qemu-img.exe to convert a disk to vmdk format which is saved on a disk that has no more space

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Hi,


I used the qemu-img.exe to convert a disk to vmdk format and the output file size could be 300 MB. However the left space of the disk the output file located on was about 200 MB. After a while, the left space had been zero but the program didn't stop or report any error. It was just going on as normal.


I read the source code and found the error report was controlled by "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState", which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no option to change the default behavior of the error report.


So I think if there were some ways to change the default value of the error report, it might be better. Further more, I suggest we could just add some codes to the "img_convert" function:


      1827:    out_blk = img_open("target", out_filename, out_fmt, flags, true, quiet);
      1828:    if (!out_blk) {
      1829:        ret = -1;
      1830:        goto out;
      1831:    }
      1832:    out_bs = blk_bs(out_blk);

++ 1833:
++ 1834:    bdrv_set_on_error(out_bs, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT);


Thanks,
Guangmu Zhu

[-- Attachment #2: Type: text/html, Size: 2020 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exe to convert a disk to vmdk format which is saved on a disk that has no more space

[Kevin Wolf]

Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
> 
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
> 
> So I think if there were some ways to change the default value of the error
> report, it might be better. Further more, I suggest we could just add some
> codes to the "img_convert" function:
> 
>       1827:    out_blk = img_open("target", out_filename, out_fmt, flags, true,
> quiet);
>       1828:    if (!out_blk) {
>       1829:        ret = -1;
>       1830:        goto out;
>       1831:    }
>       1832:    out_bs = blk_bs(out_blk);
> ++ 1833:
> ++ 1834:    bdrv_set_on_error
> (out_bs, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT);

This shouldn't make any difference for qemu-img. The error handling mode
is only for emulated devices in qemu proper.

It looks more like VMDK is somehow failing to report an error at all whn
it's running out of free disk space (though I couldn't spot an error in
the code at first sight).

Kevin

Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 5139 bytes --]

Thanks for your reply.


I read the source code again and have some question:


1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which called the "drv->bdrv_co_writev" function. I haven't know how the qume-img found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file" for the file on Windows, is it?

2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The function "bdrv_register" registers the "BlockDriver" and checks whether the "bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL, the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function "bdrv_co_io_em". There are some codes of it:

static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *iov,
                                      bool is_write)
{
    CoroutineIOCompletion co = {
        .coroutine = qemu_coroutine_self(),
    };
    BlockAIOCB *acb;


    if (is_write) {
        acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
                                       bdrv_co_io_em_complete, &co);
    } else {
        acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
                                      bdrv_co_io_em_complete, &co);
    }


    trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
    if (!acb) {
        return -EIO;
    }
    qemu_coroutine_yield();


    return co.ret;
}



3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file "raw-win32.c" and the quemu-img uses synchronous IO always, so the function "paio_submit" in the same file will be called. This function submits the "aio" to "worker_thread" with the callback "aio_worker". There are some codes in "aio_worker":


    ssize_t ret = 0;
    ......
    case QEMU_AIO_WRITE:
        count = handle_aiocb_rw(aiocb);
        if (count == aiocb->aio_nbytes) {
            count = 0;
        } else {
            count = -EINVAL;
        }
        break;

    ......
    return ret;


So though the "count" would be zero if error occurred while writing some file, the return value will always be zero. Maybe I missed something?


4. The function "worker_thread" calls the callback:


        ret = req->func(req->arg);


        req->ret = ret;
        /* Write ret before state.  */
        smp_wmb();
        req->state = THREAD_DONE;



        qemu_mutex_lock(&pool->lock);


        qemu_bh_schedule(pool->completion_bh);



The "pool->completion_bh" is function "thread_pool_completion_bh", which calls "elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb" is function "bdrv_co_io_em_complete":


static void bdrv_co_io_em_complete(void *opaque, int ret)
{
    CoroutineIOCompletion *co = opaque;


    co->ret = ret;
    qemu_coroutine_enter(co->coroutine, NULL);
}



5. Finally the return value(zero) will be stored in "co.ret" in function "bdrv_co_io_em". However, what would happen if the req haven't done but the function "bdrv_co_io_em" returned? The write operation would return an uninitialized value(is it zero? I don't know.), is it?


Maybe these made the program report nothing with writer errors, I think.


Thanks again in advance.
Guangmu Zhu


-------------------------------------------------
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
> 
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
> 
> So I think if there were some ways to change the default value of the error
> report, it might be better. Further more, I suggest we could just add some
> codes to the "img_convert" function:
> 
>       1827:    out_blk = img_open("target", out_filename, out_fmt, flags, true,
> quiet);
>       1828:    if (!out_blk) {
>       1829:        ret = -1;
>       1830:        goto out;
>       1831:    }
>       1832:    out_bs = blk_bs(out_blk);
> ++ 1833:
> ++ 1834:    bdrv_set_on_error
> (out_bs, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT);

This shouldn't make any difference for qemu-img. The error handling mode
is only for emulated devices in qemu proper.

It looks more like VMDK is somehow failing to report an error at all whn
it's running out of free disk space (though I couldn't spot an error in
the code at first sight).

Kevin

[-- Attachment #2: Type: text/html, Size: 8784 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 5388 bytes --]

Correct a mistake:
So though the "count" would be "-EINVAL" if error occurred while writing some file, the return value will always be zero. Maybe I missed something?


Sorry.
Guangmu Zhu
-------------------------------------------------


Thanks for your reply.


I read the source code again and have some question:


1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which called the "drv->bdrv_co_writev" function. I haven't know how the qume-img found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file" for the file on Windows, is it?

2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The function "bdrv_register" registers the "BlockDriver" and checks whether the "bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL, the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function "bdrv_co_io_em". There are some codes of it:

static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *iov,
                                      bool is_write)
{
    CoroutineIOCompletion co = {
        .coroutine = qemu_coroutine_self(),
    };
    BlockAIOCB *acb;


    if (is_write) {
        acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
                                       bdrv_co_io_em_complete, &co);
    } else {
        acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
                                      bdrv_co_io_em_complete, &co);
    }


    trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
    if (!acb) {
        return -EIO;
    }
    qemu_coroutine_yield();


    return co.ret;
}



3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file "raw-win32.c" and the quemu-img uses synchronous IO always, so the function "paio_submit" in the same file will be called. This function submits the "aio" to "worker_thread" with the callback "aio_worker". There are some codes in "aio_worker":


    ssize_t ret = 0;
    ......
    case QEMU_AIO_WRITE:
        count = handle_aiocb_rw(aiocb);
        if (count == aiocb->aio_nbytes) {
            count = 0;
        } else {
            count = -EINVAL;
        }
        break;

    ......
    return ret;


So though the "count" would be zero if error occurred while writing some file, the return value will always be zero. Maybe I missed something?


4. The function "worker_thread" calls the callback:


        ret = req->func(req->arg);


        req->ret = ret;
        /* Write ret before state.  */
        smp_wmb();
        req->state = THREAD_DONE;



        qemu_mutex_lock(&pool->lock);


        qemu_bh_schedule(pool->completion_bh);



The "pool->completion_bh" is function "thread_pool_completion_bh", which calls "elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb" is function "bdrv_co_io_em_complete":


static void bdrv_co_io_em_complete(void *opaque, int ret)
{
    CoroutineIOCompletion *co = opaque;


    co->ret = ret;
    qemu_coroutine_enter(co->coroutine, NULL);
}



5. Finally the return value(zero) will be stored in "co.ret" in function "bdrv_co_io_em". However, what would happen if the req haven't done but the function "bdrv_co_io_em" returned? The write operation would return an uninitialized value(is it zero? I don't know.), is it?


Maybe these made the program report nothing with writer errors, I think.


Thanks again in advance.
Guangmu Zhu


-------------------------------------------------
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
> 
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
> 
> So I think if there were some ways to change the default value of the error
> report, it might be better. Further more, I suggest we could just add some
> codes to the "img_convert" function:
> 
>       1827:    out_blk = img_open("target", out_filename, out_fmt, flags, true,
> quiet);
>       1828:    if (!out_blk) {
>       1829:        ret = -1;
>       1830:        goto out;
>       1831:    }
>       1832:    out_bs = blk_bs(out_blk);
> ++ 1833:
> ++ 1834:    bdrv_set_on_error
> (out_bs, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT);

This shouldn't make any difference for qemu-img. The error handling mode
is only for emulated devices in qemu proper.

It looks more like VMDK is somehow failing to report an error at all whn
it's running out of free disk space (though I couldn't spot an error in
the code at first sight).

Kevin

[-- Attachment #2: Type: text/html, Size: 9371 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 5719 bytes --]

If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite(extent->file, write_offset, write_buf, write_len);". So the "extend->file" is "bdrv_file", is it?


Thanks.
Guangmu Zhu


-------------------------------------------------


Correct a mistake:
So though the "count" would be "-EINVAL" if error occurred while writing some file, the return value will always be zero. Maybe I missed something?


Sorry.
Guangmu Zhu
-------------------------------------------------


Thanks for your reply.


I read the source code again and have some question:


1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which called the "drv->bdrv_co_writev" function. I haven't know how the qume-img found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file" for the file on Windows, is it?

2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The function "bdrv_register" registers the "BlockDriver" and checks whether the "bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL, the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function "bdrv_co_io_em". There are some codes of it:

static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *iov,
                                      bool is_write)
{
    CoroutineIOCompletion co = {
        .coroutine = qemu_coroutine_self(),
    };
    BlockAIOCB *acb;


    if (is_write) {
        acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
                                       bdrv_co_io_em_complete, &co);
    } else {
        acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
                                      bdrv_co_io_em_complete, &co);
    }


    trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
    if (!acb) {
        return -EIO;
    }
    qemu_coroutine_yield();


    return co.ret;
}



3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file "raw-win32.c" and the quemu-img uses synchronous IO always, so the function "paio_submit" in the same file will be called. This function submits the "aio" to "worker_thread" with the callback "aio_worker". There are some codes in "aio_worker":


    ssize_t ret = 0;
    ......
    case QEMU_AIO_WRITE:
        count = handle_aiocb_rw(aiocb);
        if (count == aiocb->aio_nbytes) {
            count = 0;
        } else {
            count = -EINVAL;
        }
        break;

    ......
    return ret;


So though the "count" would be zero if error occurred while writing some file, the return value will always be zero. Maybe I missed something?


4. The function "worker_thread" calls the callback:


        ret = req->func(req->arg);


        req->ret = ret;
        /* Write ret before state.  */
        smp_wmb();
        req->state = THREAD_DONE;



        qemu_mutex_lock(&pool->lock);


        qemu_bh_schedule(pool->completion_bh);



The "pool->completion_bh" is function "thread_pool_completion_bh", which calls "elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb" is function "bdrv_co_io_em_complete":


static void bdrv_co_io_em_complete(void *opaque, int ret)
{
    CoroutineIOCompletion *co = opaque;


    co->ret = ret;
    qemu_coroutine_enter(co->coroutine, NULL);
}



5. Finally the return value(zero) will be stored in "co.ret" in function "bdrv_co_io_em". However, what would happen if the req haven't done but the function "bdrv_co_io_em" returned? The write operation would return an uninitialized value(is it zero? I don't know.), is it?


Maybe these made the program report nothing with writer errors, I think.


Thanks again in advance.
Guangmu Zhu


-------------------------------------------------
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
> 
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
> 
> So I think if there were some ways to change the default value of the error
> report, it might be better. Further more, I suggest we could just add some
> codes to the "img_convert" function:
> 
>       1827:    out_blk = img_open("target", out_filename, out_fmt, flags, true,
> quiet);
>       1828:    if (!out_blk) {
>       1829:        ret = -1;
>       1830:        goto out;
>       1831:    }
>       1832:    out_bs = blk_bs(out_blk);
> ++ 1833:
> ++ 1834:    bdrv_set_on_error
> (out_bs, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT);

This shouldn't make any difference for qemu-img. The error handling mode
is only for emulated devices in qemu proper.

It looks more like VMDK is somehow failing to report an error at all whn
it's running out of free disk space (though I couldn't spot an error in
the code at first sight).

Kevin

[-- Attachment #2: Type: text/html, Size: 10138 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace

[Kevin Wolf]

Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> (extent->file, write_offset, write_buf, write_len);". So the "extend->file" is
> "bdrv_file", is it?

Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
call goes to bdrv_file.

> -------------------------------------------------
> 
> Correct a mistake:
> So though the "count" would be "-EINVAL" if error occurred while writing some
> file, the return value will always be zero. Maybe I missed something?

I think you're right. Instead of setting count = 0/-EINVAL in
aio_worker, we should be setting ret.

Can you try the patch below and report back?

> 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> "paio_submit" in the same file will be called. This function submits the "aio"
> to "worker_thread" with the callback "aio_worker". There are some codes in
> "aio_worker":
> 
>     ssize_t ret = 0;
>     ......
>     case QEMU_AIO_WRITE:
>         count = handle_aiocb_rw(aiocb);
>         if (count == aiocb->aio_nbytes) {
>             count = 0;
>         } else {
>             count = -EINVAL;
>         }
>         break;
>     ......
>     return ret;

Independently of your problem, the code in aio_worker() looks a bit
fishy, because handle_aiocb_rw() can't distinguish between an error
and 0 bytes transferred.

For writes, that probably doesn't matter, but for reads, I think we
return a successful read of zeroes instead of signalling an error. This
might need another patch.

Generally, the Windows backend is not getting a lot of attention and
could use someone who checks it, cleans it up and fixes bugs.

Kevin


diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
     case QEMU_AIO_WRITE:
         count = handle_aiocb_rw(aiocb);
         if (count == aiocb->aio_nbytes) {
-            count = 0;
+            ret = 0;
         } else {
-            count = -EINVAL;
+            ret = -EINVAL;
         }
         break;
     case QEMU_AIO_FLUSH:

Re: [Qemu-devel] No error report when using the qemu-img.exetoconvert a disk to vmdk format which is saved on a disk that has nomorespace

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

I'll try the patch and report in a week for I'm too busy these days. And if I could, I would like to help to maintain the Windows backend.


Sincerely.
Guangmu Zhu


-------------------------------------------------


Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> (extent->file, write_offset, write_buf, write_len);". So the "extend->file" is
> "bdrv_file", is it?

Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
call goes to bdrv_file.

> -------------------------------------------------
> 
> Correct a mistake:
> So though the "count" would be "-EINVAL" if error occurred while writing some
> file, the return value will always be zero. Maybe I missed something?

I think you're right. Instead of setting count = 0/-EINVAL in
aio_worker, we should be setting ret.

Can you try the patch below and report back?

> 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> "paio_submit" in the same file will be called. This function submits the "aio"
> to "worker_thread" with the callback "aio_worker". There are some codes in
> "aio_worker":
> 
>     ssize_t ret = 0;
>     ......
>     case QEMU_AIO_WRITE:
>         count = handle_aiocb_rw(aiocb);
>         if (count == aiocb->aio_nbytes) {
>             count = 0;
>         } else {
>             count = -EINVAL;
>         }
>         break;
>     ......
>     return ret;

Independently of your problem, the code in aio_worker() looks a bit
fishy, because handle_aiocb_rw() can't distinguish between an error
and 0 bytes transferred.

For writes, that probably doesn't matter, but for reads, I think we
return a successful read of zeroes instead of signalling an error. This
might need another patch.

Generally, the Windows backend is not getting a lot of attention and
could use someone who checks it, cleans it up and fixes bugs.

Kevin


diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
     case QEMU_AIO_WRITE:
         count = handle_aiocb_rw(aiocb);
         if (count == aiocb->aio_nbytes) {
-            count = 0;
+            ret = 0;
         } else {
-            count = -EINVAL;
+            ret = -EINVAL;
         }
         break;
     case QEMU_AIO_FLUSH:

[-- Attachment #2: Type: text/html, Size: 3891 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exetoconvert a disk to vmdk format which is saved on a disk that has nomorespace

[Guangmu Zhu]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

Hi Kevin,


I tried the patch you provide, and I haven't seen that problem yet. If the disk space is full, an error will be reported with the message "Invalid argument" and the program will stop.


Will you merge the patch to the master?


Thanks.
Guangmu Zhu


diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
     case QEMU_AIO_WRITE:
         count = handle_aiocb_rw(aiocb);
         if (count == aiocb->aio_nbytes) {
-            count = 0;
+            ret = 0;
         } else {
-            count = -EINVAL;
+            ret = -EINVAL;
         }
         break;
     case QEMU_AIO_FLUSH:


-------------------------------------------------


I'll try the patch and report in a week for I'm too busy these days. And if I could, I would like to help to maintain the Windows backend.


Sincerely.
Guangmu Zhu


-------------------------------------------------


Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> (extent->file, write_offset, write_buf, write_len);". So the "extend->file" is
> "bdrv_file", is it?

Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
call goes to bdrv_file.

> -------------------------------------------------
> 
> Correct a mistake:
> So though the "count" would be "-EINVAL" if error occurred while writing some
> file, the return value will always be zero. Maybe I missed something?

I think you're right. Instead of setting count = 0/-EINVAL in
aio_worker, we should be setting ret.

Can you try the patch below and report back?

> 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> "paio_submit" in the same file will be called. This function submits the "aio"
> to "worker_thread" with the callback "aio_worker". There are some codes in
> "aio_worker":
> 
>     ssize_t ret = 0;
>     ......
>     case QEMU_AIO_WRITE:
>         count = handle_aiocb_rw(aiocb);
>         if (count == aiocb->aio_nbytes) {
>             count = 0;
>         } else {
>             count = -EINVAL;
>         }
>         break;
>     ......
>     return ret;

Independently of your problem, the code in aio_worker() looks a bit
fishy, because handle_aiocb_rw() can't distinguish between an error
and 0 bytes transferred.

For writes, that probably doesn't matter, but for reads, I think we
return a successful read of zeroes instead of signalling an error. This
might need another patch.

Generally, the Windows backend is not getting a lot of attention and
could use someone who checks it, cleans it up and fixes bugs.

Kevin


diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
     case QEMU_AIO_WRITE:
         count = handle_aiocb_rw(aiocb);
         if (count == aiocb->aio_nbytes) {
-            count = 0;
+            ret = 0;
         } else {
-            count = -EINVAL;
+            ret = -EINVAL;
         }
         break;
     case QEMU_AIO_FLUSH:

[-- Attachment #2: Type: text/html, Size: 5454 bytes --]

Re: [Qemu-devel] No error report when using the qemu-img.exetoconvert a disk to vmdk format which is saved on a disk that has nomorespace

[Kevin Wolf]

Am 24.09.2015 um 10:01 hat Guangmu Zhu geschrieben:
> Hi Kevin,
> 
> I tried the patch you provide, and I haven't seen that problem yet. If the disk
> space is full, an error will be reported with the message "Invalid argument"
> and the program will stop.
> 
> Will you merge the patch to the master?

I'll post it in a separate email thread as a proper patch now in order
to give people a chance to review it. After that, I'll include it in a
pull request for master, but it might still be a few days until then.

Kevin

> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 68f2338..b562c94 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -119,9 +119,9 @@ static int aio_worker(void *arg)
>      case QEMU_AIO_WRITE:
>          count = handle_aiocb_rw(aiocb);
>          if (count == aiocb->aio_nbytes) {
> -            count = 0;
> +            ret = 0;
>          } else {
> -            count = -EINVAL;
> +            ret = -EINVAL;
>          }
>          break;
>      case QEMU_AIO_FLUSH:
> 
> -------------------------------------------------
> 
> I'll try the patch and report in a week for I'm too busy these days. And if I
> could, I would like to help to maintain the Windows backend.
> 
> Sincerely.
> Guangmu Zhu
> 
> -------------------------------------------------
> 
> Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> > If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> > called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> > (extent->file, write_offset, write_buf, write_len);". So the "extend->file"
> is
> > "bdrv_file", is it?
> 
> Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
> call goes to bdrv_file.
> 
> > -------------------------------------------------
> >
> > Correct a mistake:
> > So though the "count" would be "-EINVAL" if error occurred while writing some
> > file, the return value will always be zero. Maybe I missed something?
> 
> I think you're right. Instead of setting count = 0/-EINVAL in
> aio_worker, we should be setting ret.
> 
> Can you try the patch below and report back?
> 
> > 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> > "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> > "paio_submit" in the same file will be called. This function submits the
> "aio"
> > to "worker_thread" with the callback "aio_worker". There are some codes in
> > "aio_worker":
> >
> >     ssize_t ret = 0;
> >     ......
> >     case QEMU_AIO_WRITE:
> >         count = handle_aiocb_rw(aiocb);
> >         if (count == aiocb->aio_nbytes) {
> >             count = 0;
> >         } else {
> >             count = -EINVAL;
> >         }
> >         break;
> >     ......
> >     return ret;
> 
> Independently of your problem, the code in aio_worker() looks a bit
> fishy, because handle_aiocb_rw() can't distinguish between an error
> and 0 bytes transferred.
> 
> For writes, that probably doesn't matter, but for reads, I think we
> return a successful read of zeroes instead of signalling an error. This
> might need another patch.
> 
> Generally, the Windows backend is not getting a lot of attention and
> could use someone who checks it, cleans it up and fixes bugs.
> 
> Kevin
> 
> 
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 68f2338..b562c94 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -119,9 +119,9 @@ static int aio_worker(void *arg)
>      case QEMU_AIO_WRITE:
>          count = handle_aiocb_rw(aiocb);
>          if (count == aiocb->aio_nbytes) {
> -            count = 0;
> +            ret = 0;
>          } else {
> -            count = -EINVAL;
> +            ret = -EINVAL;
>          }
>          break;
>      case QEMU_AIO_FLUSH: