| Message ID | 20170913181910.29688-18-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 09/13 20:19, Max Reitz wrote: > Add a new parameter -B to qemu-io's write command. When used, qemu-io > will not wait for the result of the operation and instead execute it in > the background. Cannot aio_write be used for this purpose? Fam
On 2017-09-18 08:46, Fam Zheng wrote: > On Wed, 09/13 20:19, Max Reitz wrote: >> Add a new parameter -B to qemu-io's write command. When used, qemu-io >> will not wait for the result of the operation and instead execute it in >> the background. > > Cannot aio_write be used for this purpose? Depends. I have been trained to dislike *_aio_*, so that's probably the initial reason why I didn't use it. Second, I'd have to fix aio_write before it can be used. Currently, this aborts: echo 'qemu-io drv0 "aio_write -P 0x11 0 64M"' \ | x86_64-softmmu/qemu-system-x86_64 -monitor stdio \ -blockdev node-name=drv0,driver=null-co because aio_write_done thinks it's a good idea to use qemu-io's BlockBackend -- but when qemu-io is executed through the HMP, the BlockBackend is only created for the duration of the qemu-io command (unless there already is a BB). So what I'd have to do is add a blk_ref()/blk_unref() there, but for some reason I really don't like that. So I'd probably have to give up on using -blockdev in the new iotest and would have to use -drive again. (Note: With if=none, it still aborts while doing the block accounting, and I have looked long enough into it to just decide I'd go with if=virtio instead.) So, yes, it appears I can use aio_write, together with -drive if=virtio instead of -blockdev. The remaining difference is the following: With aio_write, all writes come from the same BlockBackend, and they are really asynchronous. That's nice because it's like a guest behaves. With write -B, they come from different BBs and the BB is usually already gone when the write is completed -- or maybe destroying the BB means that everything is flushed and thus the writes are not necessarily asynchronous. That doesn't seem so nice, but this behavior made me write patch 13, so maybe it actually is a good idea to test this. So I'm a bit torn. On one hand it seems to be a good idea to use aio_write because that's already there and it's good enough to simulate a guest. But on the other hand, write -B gives a bit more funny behavior which in my opinion is always good for a test... Max
On Mon, 09/18 19:53, Max Reitz wrote: > On 2017-09-18 08:46, Fam Zheng wrote: > > On Wed, 09/13 20:19, Max Reitz wrote: > >> Add a new parameter -B to qemu-io's write command. When used, qemu-io > >> will not wait for the result of the operation and instead execute it in > >> the background. > > > > Cannot aio_write be used for this purpose? > > Depends. I have been trained to dislike *_aio_*, so that's probably the > initial reason why I didn't use it. > > Second, I'd have to fix aio_write before it can be used. Currently, > this aborts: > > echo 'qemu-io drv0 "aio_write -P 0x11 0 64M"' \ > | x86_64-softmmu/qemu-system-x86_64 -monitor stdio \ > -blockdev node-name=drv0,driver=null-co > > because aio_write_done thinks it's a good idea to use qemu-io's > BlockBackend -- but when qemu-io is executed through the HMP, the > BlockBackend is only created for the duration of the qemu-io command > (unless there already is a BB). So what I'd have to do is add a > blk_ref()/blk_unref() there, but for some reason I really don't like that. What is the reason? If it crashes it should be fixed anyway, I assume? Fam
On 2017-09-19 10:03, Fam Zheng wrote: > On Mon, 09/18 19:53, Max Reitz wrote: >> On 2017-09-18 08:46, Fam Zheng wrote: >>> On Wed, 09/13 20:19, Max Reitz wrote: >>>> Add a new parameter -B to qemu-io's write command. When used, qemu-io >>>> will not wait for the result of the operation and instead execute it in >>>> the background. >>> >>> Cannot aio_write be used for this purpose? >> >> Depends. I have been trained to dislike *_aio_*, so that's probably the >> initial reason why I didn't use it. >> >> Second, I'd have to fix aio_write before it can be used. Currently, >> this aborts: >> >> echo 'qemu-io drv0 "aio_write -P 0x11 0 64M"' \ >> | x86_64-softmmu/qemu-system-x86_64 -monitor stdio \ >> -blockdev node-name=drv0,driver=null-co >> >> because aio_write_done thinks it's a good idea to use qemu-io's >> BlockBackend -- but when qemu-io is executed through the HMP, the >> BlockBackend is only created for the duration of the qemu-io command >> (unless there already is a BB). So what I'd have to do is add a >> blk_ref()/blk_unref() there, but for some reason I really don't like that. > > What is the reason? If it crashes it should be fixed anyway, I assume? Because the AIO CB (aio_write_done()) continues to use qemu-io's BB -- but in case of HMP's qemu-io, that is pretty much already gone once the command is done. That could be fixed, as I said, by blk_ref()ing the BB before aio_write returns (and then blk_unref()ing it in aio_write_done()). However, I'm not even sure whether aio_write_done() is always executed in the main thread... Other than that, I just have a bad feeling about adding the pair, not sure why. Probably because it means having to carry a temporary BB around until the command is done, which is weird. Well, it's not an issue permission-wise, because the qemu-io BB simply doesn't take the proper permissions (no, I'm not going to question the fact how it's then possible to even write to it, considering we have assertions that check whether the correct permissions have been taken...), and I can't think of another way. In any case, you're right, it probably needs to be fixed anyway -- even if the fix is just not allowing aio_write with a temporary BB (i.e. from HMP). Max
On Thu, 09/21 16:40, Max Reitz wrote: > On 2017-09-19 10:03, Fam Zheng wrote: > > On Mon, 09/18 19:53, Max Reitz wrote: > >> On 2017-09-18 08:46, Fam Zheng wrote: > >>> On Wed, 09/13 20:19, Max Reitz wrote: > >>>> Add a new parameter -B to qemu-io's write command. When used, qemu-io > >>>> will not wait for the result of the operation and instead execute it in > >>>> the background. > >>> > >>> Cannot aio_write be used for this purpose? > >> > >> Depends. I have been trained to dislike *_aio_*, so that's probably the > >> initial reason why I didn't use it. > >> > >> Second, I'd have to fix aio_write before it can be used. Currently, > >> this aborts: > >> > >> echo 'qemu-io drv0 "aio_write -P 0x11 0 64M"' \ > >> | x86_64-softmmu/qemu-system-x86_64 -monitor stdio \ > >> -blockdev node-name=drv0,driver=null-co > >> > >> because aio_write_done thinks it's a good idea to use qemu-io's > >> BlockBackend -- but when qemu-io is executed through the HMP, the > >> BlockBackend is only created for the duration of the qemu-io command > >> (unless there already is a BB). So what I'd have to do is add a > >> blk_ref()/blk_unref() there, but for some reason I really don't like that. > > > > What is the reason? If it crashes it should be fixed anyway, I assume? > > Because the AIO CB (aio_write_done()) continues to use qemu-io's BB -- > but in case of HMP's qemu-io, that is pretty much already gone once the > command is done. I can see aio_{read,write}_done accesses BB for accounting, we can probably skip this part altogether if issued from HMP (because the BB is gone). This way you don't need the blk_ref/unref pair. Fam > > That could be fixed, as I said, by blk_ref()ing the BB before aio_write > returns (and then blk_unref()ing it in aio_write_done()). However, I'm > not even sure whether aio_write_done() is always executed in the main > thread... > > Other than that, I just have a bad feeling about adding the pair, not > sure why. Probably because it means having to carry a temporary BB > around until the command is done, which is weird. Well, it's not an > issue permission-wise, because the qemu-io BB simply doesn't take the > proper permissions (no, I'm not going to question the fact how it's then > possible to even write to it, considering we have assertions that check > whether the correct permissions have been taken...), and I can't think > of another way. > > In any case, you're right, it probably needs to be fixed anyway -- even > if the fix is just not allowing aio_write with a temporary BB (i.e. from > HMP). > > Max >
On 2017-09-21 16:59, Fam Zheng wrote: > On Thu, 09/21 16:40, Max Reitz wrote: >> On 2017-09-19 10:03, Fam Zheng wrote: >>> On Mon, 09/18 19:53, Max Reitz wrote: >>>> On 2017-09-18 08:46, Fam Zheng wrote: >>>>> On Wed, 09/13 20:19, Max Reitz wrote: >>>>>> Add a new parameter -B to qemu-io's write command. When used, qemu-io >>>>>> will not wait for the result of the operation and instead execute it in >>>>>> the background. >>>>> >>>>> Cannot aio_write be used for this purpose? >>>> >>>> Depends. I have been trained to dislike *_aio_*, so that's probably the >>>> initial reason why I didn't use it. >>>> >>>> Second, I'd have to fix aio_write before it can be used. Currently, >>>> this aborts: >>>> >>>> echo 'qemu-io drv0 "aio_write -P 0x11 0 64M"' \ >>>> | x86_64-softmmu/qemu-system-x86_64 -monitor stdio \ >>>> -blockdev node-name=drv0,driver=null-co >>>> >>>> because aio_write_done thinks it's a good idea to use qemu-io's >>>> BlockBackend -- but when qemu-io is executed through the HMP, the >>>> BlockBackend is only created for the duration of the qemu-io command >>>> (unless there already is a BB). So what I'd have to do is add a >>>> blk_ref()/blk_unref() there, but for some reason I really don't like that. >>> >>> What is the reason? If it crashes it should be fixed anyway, I assume? >> >> Because the AIO CB (aio_write_done()) continues to use qemu-io's BB -- >> but in case of HMP's qemu-io, that is pretty much already gone once the >> command is done. > > I can see aio_{read,write}_done accesses BB for accounting, we can probably skip > this part altogether if issued from HMP (because the BB is gone). This way you > don't need the blk_ref/unref pair. Yep, and then it'd be functionally the same as this write -B, so that sounds good. Well, I fear that someone will have to rewrite it with coroutines somewhere in the future anyway, but, er, well, not a problem for now!!1! :-) Max
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 2811a89099..c635a248f5 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -481,6 +481,62 @@ static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, typedef struct { BlockBackend *blk; int64_t offset; + int bytes; + char *buf; + int flags; +} CoBackgroundWrite; + +static void coroutine_fn co_background_pwrite_entry(void *opaque) +{ + CoBackgroundWrite *data = opaque; + QEMUIOVector qiov; + int ret; + + qemu_iovec_init(&qiov, 1); + qemu_iovec_add(&qiov, data->buf, data->bytes); + + ret = blk_co_pwritev(data->blk, data->offset, data->bytes, &qiov, + data->flags); + + qemu_iovec_destroy(&qiov); + g_free(data->buf); + + if (ret < 0) { + Error *err; + error_setg_errno(&err, -ret, "Background write failed"); + error_report_err(err); + } +} + +/* Takes ownership of @buf */ +static int do_background_pwrite(BlockBackend *blk, char *buf, int64_t offset, + int64_t bytes, int flags) +{ + Coroutine *co; + CoBackgroundWrite *data; + + if (bytes > INT_MAX) { + return -ERANGE; + } + + data = g_new(CoBackgroundWrite, 1); + *data = (CoBackgroundWrite){ + .blk = blk, + .offset = offset, + .bytes = bytes, + .buf = buf, + .flags = flags, + }; + + co = qemu_coroutine_create(co_background_pwrite_entry, data); + bdrv_coroutine_enter(blk_bs(blk), co); + + return bytes; +} + +typedef struct { + BlockBackend *blk; + int64_t offset; int64_t bytes; int64_t *total; int flags; @@ -931,6 +987,7 @@ static void write_help(void) " Writes into a segment of the currently open file, using a buffer\n" " filled with a set pattern (0xcdcdcdcd).\n" " -b, -- write to the VM state rather than the virtual disk\n" +" -B, -- just start a background write, do not wait for the result\n" " -c, -- write compressed data with blk_write_compressed\n" " -f, -- use Force Unit Access semantics\n" " -p, -- ignored for backwards compatibility\n" @@ -951,7 +1008,7 @@ static const cmdinfo_t write_cmd = { .perm = BLK_PERM_WRITE, .argmin = 2, .argmax = -1, - .args = "[-bcCfquz] [-P pattern] off len", + .args = "[-bBcCfquz] [-P pattern] off len", .oneline = "writes a number of bytes at a specified offset", .help = write_help, }; @@ -961,6 +1018,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv) struct timeval t1, t2; bool Cflag = false, qflag = false, bflag = false; bool Pflag = false, zflag = false, cflag = false; + bool background = false; int flags = 0; int c, cnt; char *buf = NULL; @@ -970,11 +1028,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv) int64_t total = 0; int pattern = 0xcd; - while ((c = getopt(argc, argv, "bcCfpP:quz")) != -1) { + while ((c = getopt(argc, argv, "bBcCfpP:quz")) != -1) { switch (c) { case 'b': bflag = true; break; + case 'B': + background = true; + break; case 'c': cflag = true; break; @@ -1032,6 +1093,11 @@ static int write_f(BlockBackend *blk, int argc, char **argv) return 0; } + if (background && (bflag || cflag || zflag)) { + printf("-B cannot be specified together with -b, -c, or -z\n"); + return 0; + } + offset = cvtnum(argv[optind]); if (offset < 0) { print_cvtnum_err(offset, argv[optind]); @@ -1074,6 +1140,8 @@ static int write_f(BlockBackend *blk, int argc, char **argv) cnt = do_co_pwrite_zeroes(blk, offset, count, flags, &total); } else if (cflag) { cnt = do_write_compressed(blk, buf, offset, count, &total); + } else if (background) { + cnt = do_background_pwrite(blk, buf, offset, count, flags); } else { cnt = do_pwrite(blk, buf, offset, count, flags, &total); } @@ -1088,12 +1156,15 @@ static int write_f(BlockBackend *blk, int argc, char **argv) goto out; } - /* Finally, report back -- -C gives a parsable format */ - t2 = tsub(t2, t1); - print_report("wrote", &t2, offset, count, total, cnt, Cflag); + if (!background) { + /* Finally, report back -- -C gives a parsable format */ + t2 = tsub(t2, t1); + print_report("wrote", &t2, offset, count, total, cnt, Cflag); + } out: - if (!zflag) { + /* do_background_pwrite() takes ownership of the buffer */ + if (!zflag && !background) { qemu_io_free(buf); }
Add a new parameter -B to qemu-io's write command. When used, qemu-io will not wait for the result of the operation and instead execute it in the background. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-io-cmds.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 6 deletions(-)