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

>>> On 11.10.11 at 22:50, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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.
>> 
>> 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.

With that later patch, you should then probably also check that none
of the so far unassigned bits in u1.flag are being set.

Jan

> 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))
>               goto fail_flush;
>  
>       /*



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

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