Re: [Qemu-devel] [V2 3/3] Command "block_set" for dynamic block params change

[Kevin Wolf <kwolf@redhat.com>]

Am 17.06.2011 18:38, schrieb Supriya Kannery:
> New command "block_set" added for dynamically changing any of the block 
> device parameters. For now, dynamic setting of hostcache params using this 
> command is implemented. Other block device parameters, can be integrated 
> in similar lines.
> 
> Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

Coding style is off in this one as well.

> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -797,3 +797,35 @@ int do_block_resize(Monitor *mon, const 
>  
>      return 0;
>  }
> +
> +
> +/*
> + * Handle changes to block device settings, like hostcache,
> + * while guest is running.
> +*/
> +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +	const char *device = qdict_get_str(qdict, "device");
> +	const char *name = qdict_get_str(qdict, "name");
> +	int enable = qdict_get_bool(qdict, "enable");
> +	BlockDriverState *bs;
> +
> +	bs = bdrv_find(device);
> +	if (!bs) {
> +		qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +		return -1;
> +	}
> +
> +	if (!(strcmp(name, "hostcache"))) {

The bracket after ! isn't necessary.

> +		if (bdrv_is_inserted(bs)) {
> +			/* cache change applicable only if device inserted */
> +			return bdrv_change_hostcache(bs, enable);
> +		} else {
> +			qerror_report(QERR_DEVICE_NOT_INSERTED, device);
> +			return -1;
> +		}

I'm not so sure about this one. Why shouldn't I change the cache mode
for a device which is currently? The next thing I want to do could be
inserting a medium and using it with the new cache mode.

> +	}
> +
> +	return 0;
> +}
> +
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -651,6 +651,33 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +	BlockDriver *drv = bs->drv;
> +	int ret = 0;
> +
> +	/* No need to reopen as no change in flags */
> +	if (bdrv_flags == bs->open_flags)
> +		return 0;

There could be other reasons for reopening besides changing flags, e.g.
invalidating cached metadata.

> +
> +	/* Quiesce IO for the given block device */
> +	qemu_aio_flush();
> +	bdrv_flush(bs);

Missing error handling.

> +
> +	bdrv_close(bs);

Here, too.

> +	ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +
> +	/*
> +	* A failed attempt to reopen the image file must lead to 'abort()'
> +	*/
> +	if (ret != 0) {
> +		qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +		abort();
> +	}

Maybe we can retry with the old flags at least before aborting?

Also I would like to see a (Linux specific) version that uses the old fd
for the reopen, so that we can handle files that aren't accessible with
their old name any more. This would mean adding a .bdrv_reopen callback
in raw-posix.

> +
> +	return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -691,6 +718,20 @@ void bdrv_close_all(void)
>      }
>  }
>  
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
> +{
> +	int bdrv_flags = bs->open_flags;
> +
> +	/* set hostcache flags (without changing WCE/flush bits) */
> +	if (enable_host_cache)
> +		bdrv_flags &= ~BDRV_O_NOCACHE;
> +	else
> +		bdrv_flags |= BDRV_O_NOCACHE;
> +
> +	/* Reopen file with changed set of flags */
> +	return bdrv_reopen(bs, bdrv_flags);
> +}

Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the
monitor. We should probably expose the same functionality for the
command line, too.

Kevin