From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: [PATCH] [RFC] qemu-upstream: add discard support for xen_disk Date: Fri, 17 Jan 2014 16:29:30 +0100 Message-ID: <20140117152930.GA13324@aepfle.de> References: <1389304875-4311-1-git-send-email-olaf@aepfle.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, Jan 17, Stefano Stabellini wrote: > On Thu, 9 Jan 2014, Olaf Hering wrote: > > The discard support is enabled unconditionally. But it would be worth to > > have a knob to disable it in case the backing file was intentionally > > created non-sparse to avoid fragmentation. > > How could this be knob be passed from domU.cfg:disk=[] to the actual > > qemu process? > > It would need to be on xenstore, because that is the only per-disk > interface xen_disk is listening to. I figured that out. There are already script=, backend= and other knobs. I will see how to add a discard=on|off to libxl and write that to the xenstore backend node so qemu can get it from there. What property name do you suggest? I have something like "toolstack-option-discard" in mind. > > blkfront_setup_discard should check for "qdisk" instead of (, or in > > addition to?) "file" to actually make use of this new feature. > > Why? I don't think that would be correct: if the feature is advertised > on xenstore by the backend (feature-discard) then blkfront can/should > use it. If it is not present then it is not going to use it. > Let's not complicate things further. blockfront is broken: http://lists.xenproject.org/archives/html/xen-devel/2014-01/msg00988.html > > +++ b/hw/block/xen_disk.c > > @@ -68,6 +68,8 @@ struct ioreq { > > int presync; > > int postsync; > > uint8_t mapped; > > + int64_t sector_num; > > + int nb_sectors; > > You have access to the original request via req, I don't think you need > these two fields, do you? I will double check if thats doable. Thanks, Olaf