Re: [Qemu-devel] [PATCH] qemu-io: add aio read/write/flush commands

[Kevin Wolf]

Christoph Hellwig schrieb:
> Add commands to exercise asynchronous reads/writes and to flush all
> outstanding aio commands.  Commands to exercise aio cancellations will
> follow in a separate patch.
> 
> 
> Signed-off-by: Christoph Hellwig <address@hidden>
> 
> Index: qemu/qemu-io.c
> ===================================================================
> --- qemu.orig/qemu-io.c       2009-06-17 16:27:12.786939902 +0200
> +++ qemu/qemu-io.c    2009-06-20 21:04:09.015810045 +0200
> @@ -752,6 +752,351 @@ static const cmdinfo_t writev_cmd = {
>       .help           = writev_help,
>  };
>  
> +struct aio_ctx {
> +     QEMUIOVector qiov;
> +     int64_t offset;
> +     char *buf;
> +     int qflag;
> +     int vflag;
> +     int Cflag;
> +     int Pflag;
> +     int pattern;
> +     struct timeval t1;
> +};
> +
> +static void
> +aio_write_done(void *opaque, int ret)
> +{
> +     struct aio_ctx *ctx = opaque;
> +     struct timeval t2;
> +     int total;
> +     int cnt = 1;

What is the reason for having the total and cnt variables here which are
never modified and used only once?

> +
> +     gettimeofday(&t2, NULL);
> +
> +     total = ctx->qiov.size;
> +
> +     if (ret < 0) {
> +             printf("aio_write failed: %s\n", strerror(-ret));
> +             return;
> +     }
> +
> +     if (ctx->qflag)
> +             return;

Coding style (missing braces)

> +
> +     /* Finally, report back -- -C gives a parsable format */
> +     t2 = tsub(t2, ctx->t1);
> +     print_report("wrote", &t2, ctx->offset, ctx->qiov.size, total, cnt,
> +                  ctx->Cflag);
> +
> +     qemu_io_free(ctx->buf);
> +     free(ctx);
> +}
> +
> +static const cmdinfo_t aio_read_cmd;
> +
> +static void
> +aio_read_done(void *opaque, int ret)
> +{
> +     struct aio_ctx *ctx = opaque;
> +     struct timeval t2;
> +     int total;
> +     int cnt = 1;
> +
> +     gettimeofday(&t2, NULL);
> +
> +     total = ctx->qiov.size;
> +
> +     if (ret < 0) {
> +             printf("readv failed: %s\n", strerror(-ret));
> +             return;
> +     }
> +
> +     if (ctx->Pflag) {
> +             void *cmp_buf = malloc(total);
> +
> +             memset(cmp_buf, ctx->pattern, total);
> +             if (memcmp(ctx->buf, cmp_buf, total)) {
> +                     printf("Pattern verification failed at offset %lld, "
> +                             "%d bytes\n",
> +                             (long long) ctx->offset, total);
> +             }
> +             free(cmp_buf);
> +     }
> +
> +     if (ctx->qflag)
> +             return;
> +
> +        if (ctx->vflag)
> +             dump_buffer(ctx->buf, ctx->offset, total);

Same as for aio_write_done plus additional missing braces and
inconsistent whitespace here.

> +
> +     /* Finally, report back -- -C gives a parsable format */
> +     t2 = tsub(t2, ctx->t1);
> +     print_report("read", &t2, ctx->offset, ctx->qiov.size, total, cnt,
> +                  ctx->Cflag);
> +
> +     qemu_io_free(ctx->buf);
> +     free(ctx);
> +
> +}
> +
> +static void
> +aio_read_help(void)
> +{
> +     printf(
> +"\n"
> +" asynchronously reads a range of bytes from the given offset\n"
> +"\n"
> +" Example:\n"
> +" 'aio_read -v 512 1k 1k ' - dumps 2 kilobytes read from 512 bytes into the 
> file\n"
> +"\n"
> +" Reads a segment of the currently open file, optionally dumping it to the\n"
> +" standard output stream (with -v option) for subsequent inspection.\n"
> +" The read is performed asynchronously and should the aio_flush command \n"
> +" should be used to ensure all outstanding aio requests have been 
> completed\n"
> +" -C, -- report statistics in a machine parsable format\n"
> +" -P, -- use a pattern to verify read data\n"
> +" -v, -- dump buffer to standard output\n"
> +" -q, -- quite mode, do not show I/O statistics\n"
> +"\n");
> +}
> +
> +static int
> +aio_read_f(int argc, char **argv)
> +{
> +     char *p;
> +     int count = 0;

Shouldn't this be size_t?

> +     int nr_iov, i, c;
> +     struct aio_ctx *ctx = calloc(1, sizeof(struct aio_ctx));
> +     BlockDriverAIOCB *acb;
> +
> +     ctx->pattern = 0xcd;

Has no effect, pattern is only used with Pflag also set

> +
> +     while ((c = getopt(argc, argv, "CP:qv")) != EOF) {
> +             switch (c) {
> +             case 'C':
> +                     ctx->Cflag = 1;
> +                     break;
> +             case 'P':
> +                     ctx->Pflag = 1;
> +                     ctx->pattern = atoi(optarg);
> +                     break;
> +             case 'q':
> +                     ctx->qflag = 1;
> +                     break;
> +             case 'v':
> +                     ctx->vflag = 1;
> +                     break;
> +             default:
> +                     return command_usage(&aio_read_cmd);
> +             }
> +     }
> +
> +     if (optind > argc - 2)
> +             return command_usage(&aio_read_cmd);
> +
> +
> +     ctx->offset = cvtnum(argv[optind]);
> +     if (ctx->offset < 0) {
> +             printf("non-numeric length argument -- %s\n", argv[optind]);
> +             return 0;
> +     }
> +     optind++;
> +
> +     if (ctx->offset & 0x1ff) {
> +             printf("offset %lld is not sector aligned\n",
> +                     (long long)ctx->offset);
> +             return 0;
> +     }
> +
> +     if (count & 0x1ff) {
> +             printf("count %d is not sector aligned\n",
> +                     count);
> +             return 0;
> +     }

count is always 0 here. I guess you rather want to check each len below.

> +
> +     for (i = optind; i < argc; i++) {
> +             size_t len;
> +
> +             len = cvtnum(argv[i]);
> +             if (len < 0) {

len is unsigned

The same points are valid for aio_write_f, otherwise looks ok.

Kevin