Re: [PATCH] virtio-blk: allow toggling host cache between writebackand writethrough

[Paolo Bonzini]

Il 04/07/2012 18:02, Michael S. Tsirkin ha scritto:
> On Wed, Jul 04, 2012 at 05:54:16PM +0200, Paolo Bonzini wrote:
>> Il 04/07/2012 17:42, Michael S. Tsirkin ha scritto:
>>> On Tue, Jul 03, 2012 at 03:19:37PM +0200, Paolo Bonzini wrote:
>>>> This patch adds support for the new VIRTIO_BLK_F_CONFIG_WCE feature,
>>>> which exposes the cache mode in the configuration space and lets the
>>>> driver modify it.  The cache mode is exposed via sysfs.
>>>>
>>>> Even if the host does not support the new feature, the cache mode is
>>>> visible (thanks to the existing VIRTIO_BLK_F_WCE), but not modifiable.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>>
>>> I note this has been applied but I think the userspace
>>> API is a bit painful to use. Let's fix it before
>>> it gets set in stone?
>>
>> I'm trying to mimic the existing userspace API for SCSI disks.  FWIW I
>> would totally agree with you.
> 
> Hmm. Want to try fixing scsi? Need to be compatible but it could
> maybe ignore spaces.

Honestly I'm not sure it's really worthwhile...  And you also have the
same problem when printing.  You cannot remove the spaces, because
clients will look for the "old" string, with the spaces.

>>>> +static int virtblk_get_cache_mode(struct virtio_device *vdev)
>>>
>>> Why are you converting u8 to int here?
>>
>> The fact that it is a u8 is really an internal detail.  Perhaps the bug
>> is using u8 in the callers.
> 
> Make it bool then?
> 
> You are using u8 in the config. So you could get any value
> besides 0 and 1, and you interpret that as 1.
> Is 1 always a safe value? If not maybe it's better to set
> to a safe value if it is not 0 or 1, that is we don't know how to interpret it.

That would be a host bug; the spec only gives meaning to 0 and 1.  In
any case, "have a cache" means "needs to flush", so it's always safe.
If you interpret another value as 0, you risk omitting flushes.

Paolo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/