Re: [TEST PATCH] xen/blkfront: use blk_rq_map_sg to generate ring entries

[Greg Harris <greg.harris@metacarta.com>]

It looks like this patch applied against 2.6.28.2 resolves the issue for us -- our spinner has been running for two days now without a panic.  We've been testing this by spinning around an installer which creates file systems across two disks and then installs a base Linux system and some additional software.  Previously we were running into these panics towards the end of file-system creation.  

Will this patch be part of the next upstream kernel release?

Thank you all for your help in tracking down this issue.  

---

Greg Harris
System Administrator
MetaCarta, Inc.

(O) +1 (617) 301-5530
(M) +1 (781) 258-4474

----- "Jeremy Fitzhardinge" <jeremy@goop.org> wrote:

> From: Jens Axboe <jens.axboe@oracle.com>
> 
> On occasion, the request will apparently have more segments than we
> fit into the ring. Jens says:
> 
> > The second problem is that the block layer then appears to create
> one
> > too many segments, but from the dump it has rq->nr_phys_segments ==
> > BLKIF_MAX_SEGMENTS_PER_REQUEST. I suspect the latter is due to
> > xen-blkfront not handling the merging on its own. It should check
> that
> > the new page doesn't form part of the previous page. The
> > rq_for_each_segment() iterates all single bits in the request, not
> dma
> > segments. The "easiest" way to do this is to call blk_rq_map_sg()
> and
> > then iterate the mapped sg list. That will give you what you are
> > looking for.
> 
> > Here's a test patch, compiles but otherwise untested. I spent more
> > time figuring out how to enable XEN than to code it up, so YMMV!
> > Probably the sg list wants to be put inside the ring and only
> > initialized on allocation, then you can get rid of the sg on stack
> and
> > sg_init_table() loop call in the function. I'll leave that, and the
> > testing, to you.
> 
> [Moved sg array into info structure, and initialize once. -J]
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> ---
>  drivers/block/xen-blkfront.c |   30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> ===================================================================
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -40,6 +40,7 @@
>  #include <linux/hdreg.h>
>  #include <linux/cdrom.h>
>  #include <linux/module.h>
> +#include <linux/scatterlist.h>
>  
>  #include <xen/xenbus.h>
>  #include <xen/grant_table.h>
> @@ -82,6 +83,7 @@
>  	enum blkif_state connected;
>  	int ring_ref;
>  	struct blkif_front_ring ring;
> +	struct scatterlist sg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>  	unsigned int evtchn, irq;
>  	struct request_queue *rq;
>  	struct work_struct work;
> @@ -204,12 +206,11 @@
>  	struct blkfront_info *info = req->rq_disk->private_data;
>  	unsigned long buffer_mfn;
>  	struct blkif_request *ring_req;
> -	struct req_iterator iter;
> -	struct bio_vec *bvec;
>  	unsigned long id;
>  	unsigned int fsect, lsect;
> -	int ref;
> +	int i, ref;
>  	grant_ref_t gref_head;
> +	struct scatterlist *sg;
>  
>  	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
>  		return 1;
> @@ -238,12 +239,13 @@
>  	if (blk_barrier_rq(req))
>  		ring_req->operation = BLKIF_OP_WRITE_BARRIER;
>  
> -	ring_req->nr_segments = 0;
> -	rq_for_each_segment(bvec, req, iter) {
> -		BUG_ON(ring_req->nr_segments == BLKIF_MAX_SEGMENTS_PER_REQUEST);
> -		buffer_mfn = pfn_to_mfn(page_to_pfn(bvec->bv_page));
> -		fsect = bvec->bv_offset >> 9;
> -		lsect = fsect + (bvec->bv_len >> 9) - 1;
> +	ring_req->nr_segments = blk_rq_map_sg(req->q, req, info->sg);
> +	BUG_ON(ring_req->nr_segments > BLKIF_MAX_SEGMENTS_PER_REQUEST);
> +
> +	for_each_sg(info->sg, sg, ring_req->nr_segments, i) {
> +		buffer_mfn = pfn_to_mfn(page_to_pfn(sg_page(sg)));
> +		fsect = sg->offset >> 9;
> +		lsect = fsect + (sg->length >> 9) - 1;
>  		/* install a grant reference. */
>  		ref = gnttab_claim_grant_reference(&gref_head);
>  		BUG_ON(ref == -ENOSPC);
> @@ -254,16 +256,12 @@
>  				buffer_mfn,
>  				rq_data_dir(req) );
>  
> -		info->shadow[id].frame[ring_req->nr_segments] =
> -				mfn_to_pfn(buffer_mfn);
> -
> -		ring_req->seg[ring_req->nr_segments] =
> +		info->shadow[id].frame[i] = mfn_to_pfn(buffer_mfn);
> +		ring_req->seg[i] =
>  				(struct blkif_request_segment) {
>  					.gref       = ref,
>  					.first_sect = fsect,
>  					.last_sect  = lsect };
> -
> -		ring_req->nr_segments++;
>  	}
>  
>  	info->ring.req_prod_pvt++;
> @@ -622,6 +620,8 @@
>  	SHARED_RING_INIT(sring);
>  	FRONT_RING_INIT(&info->ring, sring, PAGE_SIZE);
>  
> +	sg_init_table(info->sg, BLKIF_MAX_SEGMENTS_PER_REQUEST);
> +
>  	err = xenbus_grant_ring(dev, virt_to_mfn(info->ring.sring));
>  	if (err < 0) {
>  		free_page((unsigned long)sring);