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

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	virtualization@lists.linux-foundation.org
Subject: Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough
Date: Wed, 04 Jul 2012 17:54:16 +0200	[thread overview]
Message-ID: <4FF46728.7030302__41390.5265661471$1341417276$gmane$org@redhat.com> (raw)
In-Reply-To: <20120704154220.GB27064@redhat.com>

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@redhat.com>
> 
> 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.

>> +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.

>> +static const char *virtblk_cache_types[] = {
>> +	"write through", "write back"
>> +};
>> +
> 
> I wonder whether something that lacks space would have been better,
> especially for show: shells might get confused and split
> a string at a space. How about we change it to writethrough,
> writeback before it's too late? It's part of a userspace API
> after all, and you see to prefer writeback in one word in your own
> code so let's not inflict pain on others :)
> 
> Also, would be nice to make it discoverable what the legal
> values are. Another attribute valid_cache_types with all values
> space separated?

We probably should add such an attribute to SCSI disks too... Even with
the spaces we could make the values \n-separated.

>> +static ssize_t
>> +virtblk_cache_type_store(struct device *dev, struct device_attribute *attr,
>> +			 const char *buf, size_t count)
>> +{
>> +	struct gendisk *disk = dev_to_disk(dev);
>> +	struct virtio_blk *vblk = disk->private_data;
>> +	struct virtio_device *vdev = vblk->vdev;
>> +	int i;
>> +	u8 writeback;
>> +
>> +	BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE));
>> +	for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; )
>> +		if (sysfs_streq(buf, virtblk_cache_types[i]))
>> +			break;
>> +
>> +	if (i < 0)
>> +		return -EINVAL;
>> +
>> +	writeback = i;
>> +	vdev->config->set(vdev,
>> +			  offsetof(struct virtio_blk_config, wce),
>> +			  &writeback, sizeof(writeback));
>> +
>> +	virtblk_update_cache_mode(vdev);
>> +	return count;
>> +}
>> +
>> +static ssize_t
>> +virtblk_cache_type_show(struct device *dev, struct device_attribute *attr,
>> +			 char *buf)
>> +{
>> +	struct gendisk *disk = dev_to_disk(dev);
>> +	struct virtio_blk *vblk = disk->private_data;
>> +	u8 writeback = virtblk_get_cache_mode(vblk->vdev);
>> +
>> +	BUG_ON(writeback >= ARRAY_SIZE(virtblk_cache_types));
>> +	return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]);
> 
> Why 40? Why snprintf? A plain sprintf won't do?

I plead guilty of cut-and-paste from drivers/scsi/sd.c. :)

Paolo