From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] virtio-blk: allow toggling host cache between writeback and writethrough Date: Thu, 5 Jul 2012 00:30:37 +0300 Message-ID: <20120704213037.GA27713@redhat.com> References: <1341321577-24435-1-git-send-email-pbonzini@redhat.com> <20120704154220.GB27064@redhat.com> <4FF46728.7030302@redhat.com> <20120704160258.GB27271@redhat.com> <4FF46A92.3020009@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <4FF46A92.3020009@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org 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 > >>> > >>> 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