From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MJlvE-0006xL-RC for qemu-devel@nongnu.org; Thu, 25 Jun 2009 06:11:12 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MJlvA-0006wz-Qj for qemu-devel@nongnu.org; Thu, 25 Jun 2009 06:11:12 -0400 Received: from [199.232.76.173] (port=58076 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MJlvA-0006ww-IW for qemu-devel@nongnu.org; Thu, 25 Jun 2009 06:11:08 -0400 Received: from mx2.redhat.com ([66.187.237.31]:38647) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MJlvA-00040V-3v for qemu-devel@nongnu.org; Thu, 25 Jun 2009 06:11:08 -0400 Message-ID: <4A434CF7.7000503@redhat.com> Date: Thu, 25 Jun 2009 12:09:59 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] qemu-io: add aio read/write/flush commands References: <20090620191044.GA25835@lst.de> In-Reply-To: <20090620191044.GA25835@lst.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: qemu-devel@nongnu.org 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 > > 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