lkml.org 
[lkml]   [2012]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough
    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


    \
     
     \ /
      Last update: 2012-07-04 18:41    [W:0.028 / U:29.988 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site