From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [PATCH] add trim command to blkback interface Date: Tue, 07 Dec 2010 10:12:30 -0800 Message-ID: <4CFE790E.6050608@goop.org> References: <4CFCDCC5020000780002617A@vpn.id2.novell.com> <4CFD2CFB.10200@goop.org> <291EDFCB1E9E224A99088639C47620228CF938A588@LONPMAILBOX01.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <291EDFCB1E9E224A99088639C47620228CF938A588@LONPMAILBOX01.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Paul Durrant Cc: Xen Devel , Owen Smith , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 12/07/2010 02:06 AM, Paul Durrant wrote: >> I don't think its so bad to have the name changes here, since if >> different operations take different argument formats, then its nice >> to >> explicitly name which operation args you're referring to. The fact >> that >> the two existing arguments happen to have sector_number as their >> first >> parameter doesn't mean the third will, so moving it into the union >> makes >> sense. >> > My feeling is that, for clarity, we should have something like this (and I haven't compiled this so there may be typos): > > struct blkif_rw_request { > uint8_t operation; /* BLKIF_OP_READ/WRITE */ > uint8_t nr_segments; /* number of segments */ > blkif_vdev_t handle; /* device handle */ > uint64_t id; /* private guest value, echoed in resp */ > blkif_sector_t sector_number;/* start sector idx on disk */ > struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > }; > > struct blkif_trim_request { > uint8_t operation; /* BLKIF_OP_TRIM */ > blkif_vdev_t handle; /* device handle */ > uint64_t id; /* private guest value, echoed in resp */ > blkif_sector_t sector_number;/* start sector idx on disk */ > uint64_t nr_sectors; /* number of sectors to trim */ > }; > > union blkif_request { > uint8_t operation; /* BLKIF_OP_??? */ > struct blkif_rw_request rw; > struct blkif_trim_request_t trim; Spurious _t there. > }; > > typedef union blkif_request blkif_request_t; > > then the specialization is done immediately after determining the op code. Sure. (But drop all the typedefs.) >> However, I'd prefer to see a separate patch do the rearrangement >> without >> adding any other functionality, and then a second patch adding trip >> support to this. >> >>> Isn't the whole patch also incomplete as it doesn't touch >>> blkfront at all (and hence will presumably cause build >>> errors)? >> Yes. How tested is this? >> > I believe Owen has tested this patch against a Windows frontend (which actually issues trims), and proven it does no harm against an existing linux frontend. Yes, but if a kernel with this patch applied as posted doesn't compile, it doesn't give much confidence in its testing. J