WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: [PATCH 2/3] xen/blkback: Fix the inhibition to map p

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: Re: [Xen-devel] Re: [PATCH 2/3] xen/blkback: Fix the inhibition to map pages when discarding sector ranges.
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Wed, 12 Oct 2011 11:30:25 +0100
Cc: Dong Yang Li <lidongyang@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Li Dongyang <lidongyang@xxxxxxxxxx>, "hch@xxxxxxxxxxxxx" <hch@xxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>
Delivery-date: Wed, 12 Oct 2011 03:31:25 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20111011205035.GA22668@xxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1318260494-27985-1-git-send-email-konrad.wilk@xxxxxxxxxx> <1318260494-27985-3-git-send-email-konrad.wilk@xxxxxxxxxx> <4E940B99020000780005AA12@xxxxxxxxxxxxxxxxxxxxxxx> <CAKH3R4-HJZBE-yWk=Dwkuwy13z1vdUrgEoJ9zFhTJgXgYfHHfA@xxxxxxxxxxxxxx> <20111011183909.GA2357@xxxxxxxxxxxxxxxxx> <20111011205035.GA22668@xxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Tue, 2011-10-11 at 21:50 +0100, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 11, 2011 at 02:39:09PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Oct 11, 2011 at 03:33:11PM +0800, Li Dongyang wrote:
> > > On Tue, Oct 11, 2011 at 3:25 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > >>>> On 10.10.11 at 17:28, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
> > > >>>> 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 <lidongyang@xxxxxxxxxx>
> > > >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > >> ---
> > > >>  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.

Either this field is nr_segments for op==DISCARD or it is not.

If it is then it should be named nr_segments and treated as such.

If it is not then it is a bug for anything to look at those bits in
memory and treat them as nr_segments.

There was a patch (from Own Smith) at one point to use a union in the
blkif request data type, would doing that help to make it clear which
fields overlap and which do not?

> > 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.
> 
> 
> So a patch like this (and there is another on top that moves the setting
> of nseg) should do it.
> 
> 
> commit 12679b29b2f828454f833e17e9090ed576c63afc
> Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date:   Mon Oct 10 00:47:49 2011 -0400
> 
>     xen/blkback: Fix the inhibition to map pages when discarding sector 
> ranges.
>     
>     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 an hypercall that did nothing. Lets guard against going in the
>     mapping function by checking for the amount of segments.
>     
>     CC: Li Dongyang <lidongyang@xxxxxxxxxx>
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index c15c559..94e659d 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -707,8 +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 &&
> -                     xen_blkbk_map(req, pending_req, seg))
> +     if (nseg && xen_blkbk_map(req, pending_req, seg))

nseg == reg->nr_segments, so as above either referencing nseg when
operation == BLKIF_OP_DISCARD is a bug or the field is badly named.

Ian.

>               goto fail_flush;
>  
>       /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>