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
