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

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

[Paolo Bonzini]

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>
---
 drivers/block/virtio_blk.c |   90 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_blk.h |    5 ++-
 2 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 693187d..5602505 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -397,6 +397,83 @@ static int virtblk_name_format(char *prefix, int index, char *buf, int buflen)
 	return 0;
 }
 
+static int virtblk_get_cache_mode(struct virtio_device *vdev)
+{
+	u8 writeback;
+	int err;
+
+	err = virtio_config_val(vdev, VIRTIO_BLK_F_CONFIG_WCE,
+				offsetof(struct virtio_blk_config, wce),
+				&writeback);
+	if (err)
+		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
+
+	return writeback;
+}
+
+static void virtblk_update_cache_mode(struct virtio_device *vdev)
+{
+	u8 writeback = virtblk_get_cache_mode(vdev);
+	struct virtio_blk *vblk = vdev->priv;
+
+	if (writeback)
+		blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
+	else
+		blk_queue_flush(vblk->disk->queue, 0);
+
+       revalidate_disk(vblk->disk);
+}
+
+static const char *virtblk_cache_types[] = {
+	"write through", "write back"
+};
+
+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]);
+}
+
+static const struct device_attribute dev_attr_cache_type_ro =
+	__ATTR(cache_type, S_IRUGO,
+	       virtblk_cache_type_show, NULL);
+static const struct device_attribute dev_attr_cache_type_rw =
+	__ATTR(cache_type, S_IRUGO|S_IWUSR,
+	       virtblk_cache_type_show, virtblk_cache_type_store);
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -474,8 +549,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	vblk->index = index;
 
 	/* configure queue flush support */
-	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
-		blk_queue_flush(q, REQ_FLUSH);
+	virtblk_update_cache_mode(vdev);
 
 	/* If disk is read-only in the host, the guest should obey */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))
@@ -553,6 +627,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	if (err)
 		goto out_del_disk;
 
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
+		err = device_create_file(disk_to_dev(vblk->disk),
+					 &dev_attr_cache_type_rw);
+	else
+		err = device_create_file(disk_to_dev(vblk->disk),
+					 &dev_attr_cache_type_ro);
+	if (err)
+		goto out_del_disk;
 	return 0;
 
 out_del_disk:
@@ -655,7 +737,7 @@ static const struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
-	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
+	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
 };
 
 /*
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
index e0edb40..18a1027 100644
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -37,8 +37,9 @@
 #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
 #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
 #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
-#define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
+#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
 #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
+#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
 
 #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
 
@@ -69,6 +70,8 @@ struct virtio_blk_config {
 	/* optimal sustained I/O size in logical blocks. */
 	__u32 opt_io_size;
 
+        /* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
+        __u8 wce;
 } __attribute__((packed));
 
 /*
-- 
1.7.1

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

[Rusty Russell]

On Tue,  3 Jul 2012 15:19:37 +0200, Paolo Bonzini <pbonzini@redhat.com> 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>

Applied, thanks!

Cheers,
Rusty.

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

[Rusty Russell]

On Tue,  3 Jul 2012 15:19:37 +0200, Paolo Bonzini <pbonzini@redhat.com> 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>

Fixed up some checkpatch warnings for you, too:

virtio:blk_allow_toggling_host_cache_between_writeback_and_writethrough.patch.new:51: WARNING: please, no spaces at the start of a line
#51: FILE: drivers/block/virtio_blk.c:424:
+       revalidate_disk(vblk->disk);$

.virtio:blk_allow_toggling_host_cache_between_writeback_and_writethrough.patch.new:54: WARNING: static const char * array should probably be static const char * const
#54: FILE: drivers/block/virtio_blk.c:427:
+static const char *virtblk_cache_types[] = {

.virtio:blk_allow_toggling_host_cache_between_writeback_and_writethrough.patch.new:160: ERROR: code indent should use tabs where possible
#160: FILE: include/linux/virtio_blk.h:73:
+        /* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */$

.virtio:blk_allow_toggling_host_cache_between_writeback_and_writethrough.patch.new:161: ERROR: code indent should use tabs where possible
#161: FILE: include/linux/virtio_blk.h:74:
+        __u8 wce;$

.virtio:blk_allow_toggling_host_cache_between_writeback_and_writethrough.patch.new:161: WARNING: please, no spaces at the start of a line
#161: FILE: include/linux/virtio_blk.h:74:
+        __u8 wce;$

total: 2 errors, 3 warnings, 132 lines checked

Cheers,
Rusty.

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

[Michael S. Tsirkin]

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>

In the future, please copy virtio patches to all interested parties
listed in MAINTAINERS.  Several are missing. You can use
scripts/get_maintainer.pl to do this automatically.


> ---
>  drivers/block/virtio_blk.c |   90 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/virtio_blk.h |    5 ++-
>  2 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 693187d..5602505 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -397,6 +397,83 @@ static int virtblk_name_format(char *prefix, int index, char *buf, int buflen)
>  	return 0;
>  }
>  
> +static int virtblk_get_cache_mode(struct virtio_device *vdev)
> +{
> +	u8 writeback;
> +	int err;
> +
> +	err = virtio_config_val(vdev, VIRTIO_BLK_F_CONFIG_WCE,
> +				offsetof(struct virtio_blk_config, wce),
> +				&writeback);
> +	if (err)
> +		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
> +
> +	return writeback;
> +}
> +
> +static void virtblk_update_cache_mode(struct virtio_device *vdev)
> +{
> +	u8 writeback = virtblk_get_cache_mode(vdev);
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	if (writeback)
> +		blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
> +	else
> +		blk_queue_flush(vblk->disk->queue, 0);
> +
> +       revalidate_disk(vblk->disk);
> +}
> +
> +static const char *virtblk_cache_types[] = {
> +	"write through", "write back"
> +};
> +
> +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]);
> +}
> +
> +static const struct device_attribute dev_attr_cache_type_ro =
> +	__ATTR(cache_type, S_IRUGO,
> +	       virtblk_cache_type_show, NULL);
> +static const struct device_attribute dev_attr_cache_type_rw =
> +	__ATTR(cache_type, S_IRUGO|S_IWUSR,
> +	       virtblk_cache_type_show, virtblk_cache_type_store);
> +
>  static int __devinit virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -474,8 +549,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	vblk->index = index;
>  
>  	/* configure queue flush support */
> -	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> -		blk_queue_flush(q, REQ_FLUSH);
> +	virtblk_update_cache_mode(vdev);
>  
>  	/* If disk is read-only in the host, the guest should obey */
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))
> @@ -553,6 +627,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	if (err)
>  		goto out_del_disk;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_rw);
> +	else
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_ro);
> +	if (err)
> +		goto out_del_disk;
>  	return 0;
>  
>  out_del_disk:
> @@ -655,7 +737,7 @@ static const struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
> -	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
> +	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
>  };
>  
>  /*
> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
> index e0edb40..18a1027 100644
> --- a/include/linux/virtio_blk.h
> +++ b/include/linux/virtio_blk.h
> @@ -37,8 +37,9 @@
>  #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
>  #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
>  #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
> -#define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
> +#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
>  #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
> +#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
>  
>  #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
>  
> @@ -69,6 +70,8 @@ struct virtio_blk_config {
>  	/* optimal sustained I/O size in logical blocks. */
>  	__u32 opt_io_size;
>  
> +        /* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
> +        __u8 wce;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Michael S. Tsirkin]

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?

Some more minor nits below.
Also Cc lists from MAINTAINERS.

> ---
>  drivers/block/virtio_blk.c |   90 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/virtio_blk.h |    5 ++-
>  2 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 693187d..5602505 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -397,6 +397,83 @@ static int virtblk_name_format(char *prefix, int index, char *buf, int buflen)
>  	return 0;
>  }
>  
> +static int virtblk_get_cache_mode(struct virtio_device *vdev)

Why are you converting u8 to int here?
All users convert it back ...
Also, this is not really "get cache mode" it's more of a
"writeback_enabled".


> +{
> +	u8 writeback;
> +	int err;
> +
> +	err = virtio_config_val(vdev, VIRTIO_BLK_F_CONFIG_WCE,
> +				offsetof(struct virtio_blk_config, wce),
> +				&writeback);
> +	if (err)
> +		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
> +
> +	return writeback;
> +}
> +
> +static void virtblk_update_cache_mode(struct virtio_device *vdev)
> +{
> +	u8 writeback = virtblk_get_cache_mode(vdev);
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	if (writeback)
> +		blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
> +	else
> +		blk_queue_flush(vblk->disk->queue, 0);
> +
> +       revalidate_disk(vblk->disk);
> +}
> +
> +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?


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

> +}
> +
> +static const struct device_attribute dev_attr_cache_type_ro =
> +	__ATTR(cache_type, S_IRUGO,
> +	       virtblk_cache_type_show, NULL);
> +static const struct device_attribute dev_attr_cache_type_rw =
> +	__ATTR(cache_type, S_IRUGO|S_IWUSR,
> +	       virtblk_cache_type_show, virtblk_cache_type_store);
> +
>  static int __devinit virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -474,8 +549,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	vblk->index = index;
>  
>  	/* configure queue flush support */
> -	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> -		blk_queue_flush(q, REQ_FLUSH);
> +	virtblk_update_cache_mode(vdev);
>  
>  	/* If disk is read-only in the host, the guest should obey */
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))
> @@ -553,6 +627,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	if (err)
>  		goto out_del_disk;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_rw);
> +	else
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_ro);
> +	if (err)
> +		goto out_del_disk;
>  	return 0;
>  
>  out_del_disk:
> @@ -655,7 +737,7 @@ static const struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
> -	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
> +	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
>  };
>  
>  /*
> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
> index e0edb40..18a1027 100644
> --- a/include/linux/virtio_blk.h
> +++ b/include/linux/virtio_blk.h
> @@ -37,8 +37,9 @@
>  #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
>  #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
>  #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
> -#define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
> +#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
>  #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
> +#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
>  
>  #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
>  
> @@ -69,6 +70,8 @@ struct virtio_blk_config {
>  	/* optimal sustained I/O size in logical blocks. */
>  	__u32 opt_io_size;
>  
> +        /* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
> +        __u8 wce;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Michael S. Tsirkin]

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?

Some more minor nits below.
Also Cc lists from MAINTAINERS.

> ---
>  drivers/block/virtio_blk.c |   90 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/virtio_blk.h |    5 ++-
>  2 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 693187d..5602505 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -397,6 +397,83 @@ static int virtblk_name_format(char *prefix, int index, char *buf, int buflen)
>  	return 0;
>  }
>  
> +static int virtblk_get_cache_mode(struct virtio_device *vdev)

Why are you converting u8 to int here?
All users convert it back ...
Also, this is not really "get cache mode" it's more of a
"writeback_enabled".


> +{
> +	u8 writeback;
> +	int err;
> +
> +	err = virtio_config_val(vdev, VIRTIO_BLK_F_CONFIG_WCE,
> +				offsetof(struct virtio_blk_config, wce),
> +				&writeback);
> +	if (err)
> +		writeback = virtio_has_feature(vdev, VIRTIO_BLK_F_WCE);
> +
> +	return writeback;
> +}
> +
> +static void virtblk_update_cache_mode(struct virtio_device *vdev)
> +{
> +	u8 writeback = virtblk_get_cache_mode(vdev);
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	if (writeback)
> +		blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
> +	else
> +		blk_queue_flush(vblk->disk->queue, 0);
> +
> +       revalidate_disk(vblk->disk);
> +}
> +
> +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?


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

> +}
> +
> +static const struct device_attribute dev_attr_cache_type_ro =
> +	__ATTR(cache_type, S_IRUGO,
> +	       virtblk_cache_type_show, NULL);
> +static const struct device_attribute dev_attr_cache_type_rw =
> +	__ATTR(cache_type, S_IRUGO|S_IWUSR,
> +	       virtblk_cache_type_show, virtblk_cache_type_store);
> +
>  static int __devinit virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -474,8 +549,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	vblk->index = index;
>  
>  	/* configure queue flush support */
> -	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> -		blk_queue_flush(q, REQ_FLUSH);
> +	virtblk_update_cache_mode(vdev);
>  
>  	/* If disk is read-only in the host, the guest should obey */
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_RO))
> @@ -553,6 +627,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	if (err)
>  		goto out_del_disk;
>  
> +	if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_rw);
> +	else
> +		err = device_create_file(disk_to_dev(vblk->disk),
> +					 &dev_attr_cache_type_ro);
> +	if (err)
> +		goto out_del_disk;
>  	return 0;
>  
>  out_del_disk:
> @@ -655,7 +737,7 @@ static const struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
>  	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
> -	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
> +	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
>  };
>  
>  /*
> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
> index e0edb40..18a1027 100644
> --- a/include/linux/virtio_blk.h
> +++ b/include/linux/virtio_blk.h
> @@ -37,8 +37,9 @@
>  #define VIRTIO_BLK_F_RO		5	/* Disk is read-only */
>  #define VIRTIO_BLK_F_BLK_SIZE	6	/* Block size of disk is available*/
>  #define VIRTIO_BLK_F_SCSI	7	/* Supports scsi command passthru */
> -#define VIRTIO_BLK_F_FLUSH	9	/* Cache flush command support */
> +#define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
>  #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
> +#define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
>  
>  #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
>  
> @@ -69,6 +70,8 @@ struct virtio_blk_config {
>  	/* optimal sustained I/O size in logical blocks. */
>  	__u32 opt_io_size;
>  
> +        /* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
> +        __u8 wce;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Paolo Bonzini]

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

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

[Paolo Bonzini]

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

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

[Michael S. Tsirkin]

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

Hmm. Want to try fixing scsi? Need to be compatible but it could
maybe ignore 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.

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

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

[Michael S. Tsirkin]

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

Hmm. Want to try fixing scsi? Need to be compatible but it could
maybe ignore 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.

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

Re: [PATCH] virtio-blk: allow toggling host cache between writeback and 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@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.
> 
> 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

Re: [PATCH] virtio-blk: allow toggling host cache between writeback and 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@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.
> 
> 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

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

[Sasha Levin]

On Tue, 2012-07-03 at 15:19 +0200, Paolo Bonzini wrote:
> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
> index e0edb40..18a1027 100644
> --- a/include/linux/virtio_blk.h
> +++ b/include/linux/virtio_blk.h
> @@ -37,8 +37,9 @@
>  #define VIRTIO_BLK_F_RO                5       /* Disk is read-only */
>  #define VIRTIO_BLK_F_BLK_SIZE  6       /* Block size of disk is available*/
>  #define VIRTIO_BLK_F_SCSI      7       /* Supports scsi command passthru */
> -#define VIRTIO_BLK_F_FLUSH     9       /* Cache flush command support */
> +#define VIRTIO_BLK_F_WCE       9       /* Writeback mode enabled after reset */
>  #define VIRTIO_BLK_F_TOPOLOGY  10      /* Topology information is available */
> +#define VIRTIO_BLK_F_CONFIG_WCE        11      /* Writeback mode available in config */ 

Wouldn't this change break any usermode code that implements virtio-blk?

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

[Paolo Bonzini]

Il 04/07/2012 18:26, Sasha Levin ha scritto:
> On Tue, 2012-07-03 at 15:19 +0200, Paolo Bonzini wrote:
>> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
>> index e0edb40..18a1027 100644
>> --- a/include/linux/virtio_blk.h
>> +++ b/include/linux/virtio_blk.h
>> @@ -37,8 +37,9 @@
>>  #define VIRTIO_BLK_F_RO                5       /* Disk is read-only */
>>  #define VIRTIO_BLK_F_BLK_SIZE  6       /* Block size of disk is available*/
>>  #define VIRTIO_BLK_F_SCSI      7       /* Supports scsi command passthru */
>> -#define VIRTIO_BLK_F_FLUSH     9       /* Cache flush command support */
>> +#define VIRTIO_BLK_F_WCE       9       /* Writeback mode enabled after reset */
>>  #define VIRTIO_BLK_F_TOPOLOGY  10      /* Topology information is available */
>> +#define VIRTIO_BLK_F_CONFIG_WCE        11      /* Writeback mode available in config */ 
> 
> Wouldn't this change break any usermode code that implements virtio-blk?

No, the change is really just clarifying the existing spec, and
mandating that virtio-blk implementations follow certain assumptions of
the Linux driver.

In particular, the Linux driver is already assuming that the host
exposes VIRTIO_BLK_F_FLUSH if and only if it exposes a volatile write
cache.  This works because if you have a writeback cache, but provide no
way to flush it, the guest driver really cannot do anything about it
anyway.  Might as well treat it as writethrough, i.e. blk_queue_flush(q, 0).

QEMU in fact has already behaved like that, and even called the flag
VIRTIO_BLK_F_WCACHE instead of VIRRTIO_BLK_F_FLUSH.

Paolo

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

[Sasha Levin]

On Wed, 2012-07-04 at 18:32 +0200, Paolo Bonzini wrote:
> Il 04/07/2012 18:26, Sasha Levin ha scritto:
> > On Tue, 2012-07-03 at 15:19 +0200, Paolo Bonzini wrote:
> >> diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
> >> index e0edb40..18a1027 100644
> >> --- a/include/linux/virtio_blk.h
> >> +++ b/include/linux/virtio_blk.h
> >> @@ -37,8 +37,9 @@
> >>  #define VIRTIO_BLK_F_RO                5       /* Disk is read-only */
> >>  #define VIRTIO_BLK_F_BLK_SIZE  6       /* Block size of disk is available*/
> >>  #define VIRTIO_BLK_F_SCSI      7       /* Supports scsi command passthru */
> >> -#define VIRTIO_BLK_F_FLUSH     9       /* Cache flush command support */
> >> +#define VIRTIO_BLK_F_WCE       9       /* Writeback mode enabled after reset */
> >>  #define VIRTIO_BLK_F_TOPOLOGY  10      /* Topology information is available */
> >> +#define VIRTIO_BLK_F_CONFIG_WCE        11      /* Writeback mode available in config */ 
> > 
> > Wouldn't this change break any usermode code that implements virtio-blk?
> 
> No, the change is really just clarifying the existing spec, and
> mandating that virtio-blk implementations follow certain assumptions of
> the Linux driver.
> 
> In particular, the Linux driver is already assuming that the host
> exposes VIRTIO_BLK_F_FLUSH if and only if it exposes a volatile write
> cache.  This works because if you have a writeback cache, but provide no
> way to flush it, the guest driver really cannot do anything about it
> anyway.  Might as well treat it as writethrough, i.e. blk_queue_flush(q, 0).
> 
> QEMU in fact has already behaved like that, and even called the flag
> VIRTIO_BLK_F_WCACHE instead of VIRRTIO_BLK_F_FLUSH.

There are two things going on here:
 1. Rename VIRTIO_BLK_F_FLUSH to VIRTIO_BLK_F_WCE
 2. Add a new VIRTIO_BLK_F_CONFIG_WCE

I'm concerned that the first change is going to break compilation for
any code that included linux/virtio-blk.h and used VIRTIO_BLK_F_FLUSH.

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

[Michael S. Tsirkin]

On Wed, Jul 04, 2012 at 06:08:50PM +0200, Paolo Bonzini wrote:
> 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@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.
> > 
> > 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.

Right. Oh well.

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

Yes but if the other side does not validate values implementations
*will* have bugs. Why not declare bits 1-7 reserved?
Then we can reuse other bits later.

>  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

OK, that's good.

-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jul 04, 2012 at 06:08:50PM +0200, Paolo Bonzini wrote:
> 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@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.
> > 
> > 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.

Right. Oh well.

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

Yes but if the other side does not validate values implementations
*will* have bugs. Why not declare bits 1-7 reserved?
Then we can reuse other bits later.

>  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

OK, that's good.

-- 
MST

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

[Paolo Bonzini]

Il 04/07/2012 23:30, Michael S. Tsirkin ha scritto:
>>>>>> +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.
> 
> Yes but if the other side does not validate values implementations
> *will* have bugs. Why not declare bits 1-7 reserved?

Ok, that would be a different change.  I thought about it yesterday, but
it seemed like a useless complication.  It's not like we have been
adding configuration fields every other week. :)

But I can certainly prepare patches to both virtio-blk and the spec for
that if you prefer.

Paolo

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

[Paolo Bonzini]

Il 04/07/2012 23:30, Michael S. Tsirkin ha scritto:
>>>>>> +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.
> 
> Yes but if the other side does not validate values implementations
> *will* have bugs. Why not declare bits 1-7 reserved?

Ok, that would be a different change.  I thought about it yesterday, but
it seemed like a useless complication.  It's not like we have been
adding configuration fields every other week. :)

But I can certainly prepare patches to both virtio-blk and the spec for
that if you prefer.

Paolo

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

[Paolo Bonzini]

Il 04/07/2012 23:11, Sasha Levin ha scritto:
> There are two things going on here:
>  1. Rename VIRTIO_BLK_F_FLUSH to VIRTIO_BLK_F_WCE
>  2. Add a new VIRTIO_BLK_F_CONFIG_WCE
> 
> I'm concerned that the first change is going to break compilation for
> any code that included linux/virtio-blk.h and used VIRTIO_BLK_F_FLUSH.

That would be nlkt, right? :)

We can add back VIRTIO_BLK_F_FLUSH as a synonym then.

Paolo

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

[Sasha Levin]

On Thu, 2012-07-05 at 08:47 +0200, Paolo Bonzini wrote:
> Il 04/07/2012 23:11, Sasha Levin ha scritto:
> > There are two things going on here:
> >  1. Rename VIRTIO_BLK_F_FLUSH to VIRTIO_BLK_F_WCE
> >  2. Add a new VIRTIO_BLK_F_CONFIG_WCE
> > 
> > I'm concerned that the first change is going to break compilation for
> > any code that included linux/virtio-blk.h and used VIRTIO_BLK_F_FLUSH.
> 
> That would be nlkt, right? :)

nlkt, lguest, and probably others.

linux/virtio_blk.h is a public kernel header, which is supposed to be
used from userspace - so I assume many others who implemented virtio-blk
for any reason took advantage of that header.

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

[Badari Pulavarty]

On Tue, 2012-07-03 at 15:19 +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>
> ---

Hi Paolo,

Curious - What is the host side change to support this ? QEMU would
close and re-open the device/file with the corresponding flags
(O_SYNC) ?

And also, is there a way to expose cache=none (O_DIRECT) to the guest ?
Our cluster filesystem folks need a way to verify/guarantee that
virtio-blk device has cache=none selected at host. Otherwise, they
can not support a cluster filesystem running inside a VM (on
virtio-blk).

Thoughts ?

Thanks,
Badari

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

[Paolo Bonzini]

Il 05/07/2012 20:39, Badari Pulavarty ha scritto:
> On Tue, 2012-07-03 at 15:19 +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>
>> ---
> 
> Hi Paolo,
> 
> Curious - What is the host side change to support this ? QEMU would
> close and re-open the device/file with the corresponding flags
> (O_SYNC) ?

QEMU is not using O_SYNC anymore; instead, it manually issues a flush
after each write.  We found this didn't penalize performance (in fact,
for qcow2 metadata writes the fdatasyncs will be more coarse and there
is a small improvement).  So, when you toggle writethrough to writeback
QEMU simply has to stop forcing flushes, and vice versa if you go to
writethrough.

> And also, is there a way to expose cache=none (O_DIRECT) to the guest ?

Not yet.  The main problem is that while we can invent something in
virtio-blk, I'm not sure if there is an equivalent in SCSI for example.

Paolo

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

[Rusty Russell]

On Thu, 05 Jul 2012 09:02:23 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Thu, 2012-07-05 at 08:47 +0200, Paolo Bonzini wrote:
> > Il 04/07/2012 23:11, Sasha Levin ha scritto:
> > > There are two things going on here:
> > >  1. Rename VIRTIO_BLK_F_FLUSH to VIRTIO_BLK_F_WCE
> > >  2. Add a new VIRTIO_BLK_F_CONFIG_WCE
> > > 
> > > I'm concerned that the first change is going to break compilation for
> > > any code that included linux/virtio-blk.h and used VIRTIO_BLK_F_FLUSH.
> > 
> > That would be nlkt, right? :)
> 
> nlkt, lguest, and probably others.
> 
> linux/virtio_blk.h is a public kernel header, which is supposed to be
> used from userspace - so I assume many others who implemented virtio-blk
> for any reason took advantage of that header.

lguest doesn't have an API, so it can be patched.

But if someone else breaks, then yes, we need to define the old name
too.

Cheers,
Rusty.

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

[Rusty Russell]

On Thu, 05 Jul 2012 09:02:23 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Thu, 2012-07-05 at 08:47 +0200, Paolo Bonzini wrote:
> > Il 04/07/2012 23:11, Sasha Levin ha scritto:
> > > There are two things going on here:
> > >  1. Rename VIRTIO_BLK_F_FLUSH to VIRTIO_BLK_F_WCE
> > >  2. Add a new VIRTIO_BLK_F_CONFIG_WCE
> > > 
> > > I'm concerned that the first change is going to break compilation for
> > > any code that included linux/virtio-blk.h and used VIRTIO_BLK_F_FLUSH.
> > 
> > That would be nlkt, right? :)
> 
> nlkt, lguest, and probably others.
> 
> linux/virtio_blk.h is a public kernel header, which is supposed to be
> used from userspace - so I assume many others who implemented virtio-blk
> for any reason took advantage of that header.

BTW, I have patched this myself now:

From: Rusty Russell <rusty@rustcorp.com.au>
Subject: virtio-blk: return VIRTIO_BLK_F_FLUSH to header.

This got renamed and clarified, but let's not break any userspace out there.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/virtio_blk.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -41,6 +41,11 @@
 #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
 #define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
 
+#ifndef __KERNEL__
+/* Old (deprecated) name for VIRTIO_BLK_F_WCE. */
+#define VIRTIO_BLK_F_FLUSH VIRTIO_BLK_F_WCE
+#endif
+
 #define VIRTIO_BLK_ID_BYTES	20	/* ID string length */
 
 struct virtio_blk_config {