Re: [Qemu-devel] [v1 Patch 3/10]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[Supriya Kannery]

On 06/16/2012 03:26 AM, Eric Blake wrote:
> On 06/15/2012 02:47 PM, Supriya Kannery wrote:
> > New command "block_set_hostcache" added for dynamically changing
> > host pagecache setting of a block device.
> >
> > Usage:
> >   block_set_hostcache<device>  <option>
> >     <device>  = block device
> >     <option>  = on/off
> >
> > Example:
> >   (qemu) block_set_hostcache ide0-hd0 off
> >
> > Signed-off-by: Supriya Kannery<address@hidden>
> >
> > ---
> >   block.c         |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   block.h         |    2 ++
> >   blockdev.c      |   26 ++++++++++++++++++++++++++
> >   blockdev.h      |    2 ++
> >   hmp-commands.hx |   14 ++++++++++++++
> >   qmp-commands.hx |   27 +++++++++++++++++++++++++++
> >   6 files changed, 125 insertions(+)
>
> Doesn't this also need to touch qapi-schema.json?
> [/me reads full patch]
> Oh, you did - but your diffstat is stale.  It might be worth figuring
> out what in your workflow leads to stale diffstats.
>
> > Index: qemu/block.c
> > ===================================================================
> > --- qemu.orig/block.c
> > +++ qemu/block.c
> > @@ -858,6 +858,35 @@ unlink_and_fail:
> >       return ret;
> >   }
> >
> > +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> > +{
> > +    BlockDriver *drv = bs->drv;
> > +    int ret = 0, open_flags;
> > +
> > +    /* Quiesce IO for the given block device */
> > +    qemu_aio_flush();
> > +    ret = bdrv_flush(bs);
> > +    if (ret != 0) {
> > +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
> > +        return ret;
> > +    }
> > +    open_flags = bs->open_flags;
> > +    bdrv_close(bs);
> > +
> > +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
>
> Yuck.  This is bad, and why 'transaction' was invented.  Any time you
> close() before open() you risk completely losing the file...
>
> > +    if (ret<  0) {
> > +        /* Reopen failed. Try to open with original flags */
> > +        qerror_report(QERR_OPEN_FILE_FAILED, bs->filename);
> > +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> > +        if (ret<  0) {
> > +            /* Reopen failed with orig and modified flags */
> > +            abort();
>
> and an abort() is not a nice reaction to that.
>
> I think we should rebase the series to do the safe reopen prior to
> adding this command (at least, just judging by the title of 4/10), to
> avoid intermediate bad code.

 Yes, will reorder the patches to have safe reopen done first and
then block_set_hostcache use it.

-thanks, Supriya