Re: [PATCH, RFC] virtio_blk: add cache control support

[Rusty Russell <rusty@rustcorp.com.au>]

On Wed, 16 Mar 2011 15:09:58 +0100, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Mar 16, 2011 at 02:39:39PM +1030, Rusty Russell wrote:
> > > +	if (strncmp(buf, "write through", sizeof("write through") - 1) == 0) {
> > > +		;
> > > +	} else if (strncmp(buf, "write back", sizeof("write back") - 1) == 0) {
> > 
> >    Is there a reason we're not letting gcc and/or strcmp do the
> > optimization work here?
> 
> I'm happ to switch strcmp.

Of course, that's assuming buf is nul terminated.

> > > +	vdev->config->set(vdev, offsetof(struct virtio_blk_config, features),
> > > +	                  &features, sizeof(features));
> > > +
> > > +	vdev->config->get(vdev, offsetof(struct virtio_blk_config, features),
> > > +			  &features2, sizeof(features2));
> > > +
> > > +	if ((features & VIRTIO_BLK_RT_WCE) !=
> > > +	    (features2 & VIRTIO_BLK_RT_WCE))
> > > +		return -EIO;
> > 
> >    This seems like a debugging check you left in.  Or do you suspect
> > some issues?
> 
> No, it's intentional.  config space writes can't return errors, so we need
> to check that the value has really changed.  I'll add a comment explaining it.

OK, under what circumstances could it fail?

If you're using this mechanism to indicate that the host doesn't support
the feature, that's making an assumption about the nature of config
space writes which isn't true for non-PCI virtio.

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should?  But we should be explicit about needing it...

Thanks,
Rusty.