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

[Kevin Wolf]

Am 22.06.2011 18:09, schrieb Supriya Kannery:
> On 06/20/2011 08:04 PM, Kevin Wolf wrote:
>> Am 17.06.2011 18:38, schrieb Supriya Kannery:
> 
>>> +           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.
> 
> Uninserted device has no file to be re-opened. So such a check was used 
> to avoid calling bdrv_reopen without a filename.
> 
> I agree with your point. Hostcache value change is applicable
> for devices that are not inserted as well. If device is inserted,
> request for hostcache setting change can lead to change in open_flags 
> and then reopen of file. Otherwise, the request can result in just 
> open_flags updation, which can be used for opening a file when
> a device gets inserted.
> 
> Will make the above modification in V3
> 
>>
>>> +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.
>>
> 
> will add in V3
> 
> 
>>> +   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.
>>
> 
> Will work on to implement version of re-open using fd. Since for 
> block_set command processing, re-open using filename already works, is
> it ok to have the 're-open using fd' done as a separate patchset ?

Sure, it's just an obvious improvement.

>>> +   /* 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.
>>
> 
> hostcache is indirectly set/reset from qemu commandline, using "cache="
> (none/writeback/writethrough/unsafe) option. So should we be having 
> "hostcache=xx" option added to -drive  in addition to "cache=" ?

I think that's the right thing to do in the long run. Christoph?

Kevin