From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755184Ab1JKSqV (ORCPT ); Tue, 11 Oct 2011 14:46:21 -0400 Received: from acsinet15.oracle.com ([141.146.126.227]:39996 "EHLO acsinet15.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754879Ab1JKSqT convert rfc822-to-8bit (ORCPT ); Tue, 11 Oct 2011 14:46:19 -0400 Date: Tue, 11 Oct 2011 14:39:09 -0400 From: Konrad Rzeszutek Wilk To: Li Dongyang Cc: Jan Beulich , hch@infradead.org, Dong Yang Li , xen-devel@lists.xensource.com, linux-kernel@vger.kernel.org Subject: Re: [Xen-devel] Re: [PATCH 2/3] xen/blkback: Fix the inhibition to map pages when discarding sector ranges. Message-ID: <20111011183909.GA2357@phenom.oracle.com> References: <1318260494-27985-1-git-send-email-konrad.wilk@oracle.com> <1318260494-27985-3-git-send-email-konrad.wilk@oracle.com> <4E940B99020000780005AA12@victor.provo.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Content-Transfer-Encoding: 8BIT X-Source-IP: acsinet21.oracle.com [141.146.126.237] X-Auth-Type: Internal IP X-CT-RefId: str=0001.0A090207.4E948EF4.0189:SCFMA922111,ss=1,re=-4.000,fgs=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 11, 2011 at 03:33:11PM +0800, Li Dongyang wrote: > On Tue, Oct 11, 2011 at 3:25 PM, Jan Beulich wrote: > >>>> On 10.10.11 at 17:28, Konrad Rzeszutek Wilk wrote: > >> The 'operation' parameters are the ones provided to the bio layer while > >> the req->operation are the ones passed in between the backend and > >> frontend. We used the wrong 'operation' value to squash the > >> call to map pages when processing the discard operation resulting > >> in mapping the pages unnecessarily. > >> > >> CC: Li Dongyang > >> Signed-off-by: Konrad Rzeszutek Wilk > >> --- > >>  drivers/block/xen-blkback/blkback.c |    2 +- > >>  1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/block/xen-blkback/blkback.c > >> b/drivers/block/xen-blkback/blkback.c > >> index 184b133..3da9a40 100644 > >> --- a/drivers/block/xen-blkback/blkback.c > >> +++ b/drivers/block/xen-blkback/blkback.c > >> @@ -707,7 +707,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif, > >>        * the hypercall to unmap the grants - that is all done in > >>        * xen_blkbk_unmap. > >>        */ > >> -     if (operation != BLKIF_OP_DISCARD && > >> +     if (operation != REQ_DISCARD && > > > > Why is that check necessary in the first place? xen_blkbk_map() doesn't > > do any harm when req->nr_segments is zero (as could also be the case > > on WRITE_FLUSH ones). > > > Ah, you are right, we could remove this check then, Thanks Except that req->nr_segments for blkif__request_discard is actually the reserved field. See: struct blkif_request { uint8_t operation; /* BLKIF_OP_??? */ uint8_t nr_segments; /* number of segments */ blkif_vdev_t handle; /* only for read/write requests */ .. snip.. and: struct blkif_request_discard { uint8_t operation; /* BLKIF_OP_DISCARD */ /* ignored if 'discard-secure=0' */ #define BLKIF_OP_DISCARD_FLAG_SECURE (1<<0) uint8_t flag; /* BLKIF_OP_DISCARD_FLAG_SECURE or 0 */ blkif_vdev_t handle; /* same as for read/write requests */ which will throw off the logic for nr_segments all wrong since for some discard operations it would read the nr_segments as 1. So we do need some logic in there to work with this.