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

[Kevin Wolf <kwolf@redhat.com>]

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 <hch@lst.de>
> 
> 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