* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough [not found] <1341321577-24435-1-git-send-email-pbonzini@redhat.com> @ 2012-07-04 15:42 ` Michael S. Tsirkin 2012-07-04 15:54 ` Paolo Bonzini [not found] ` <4FF46728.7030302@redhat.com> 0 siblings, 2 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2012-07-04 15:42 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, virtualization 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough 2012-07-04 15:42 ` [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough Michael S. Tsirkin @ 2012-07-04 15:54 ` Paolo Bonzini [not found] ` <4FF46728.7030302@redhat.com> 1 sibling, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2012-07-04 15:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: linux-kernel, kvm, virtualization 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <4FF46728.7030302@redhat.com>]
* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough [not found] ` <4FF46728.7030302@redhat.com> @ 2012-07-04 16:02 ` Michael S. Tsirkin 2012-07-04 16:08 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2012-07-04 16:02 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, virtualization 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough 2012-07-04 16:02 ` Michael S. Tsirkin @ 2012-07-04 16:08 ` Paolo Bonzini 2012-07-04 21:30 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2012-07-04 16:08 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: linux-kernel, kvm, virtualization 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough 2012-07-04 16:08 ` Paolo Bonzini @ 2012-07-04 21:30 ` Michael S. Tsirkin 2012-07-05 6:45 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2012-07-04 21:30 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm, virtualization 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough 2012-07-04 21:30 ` Michael S. Tsirkin @ 2012-07-05 6:45 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2012-07-05 6:45 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: linux-kernel, kvm, virtualization 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-05 6:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1341321577-24435-1-git-send-email-pbonzini@redhat.com>
2012-07-04 15:42 ` [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough Michael S. Tsirkin
2012-07-04 15:54 ` Paolo Bonzini
[not found] ` <4FF46728.7030302@redhat.com>
2012-07-04 16:02 ` Michael S. Tsirkin
2012-07-04 16:08 ` Paolo Bonzini
2012-07-04 21:30 ` Michael S. Tsirkin
2012-07-05 6:45 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).