LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jens Axboe <jens.axboe@oracle.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	lkml - Kernel Mailing List <linux-kernel@vger.kernel.org>,
	virtualization <virtualization@lists.osdl.org>
Subject: Re: [PATCH 7/8] lguest: trivial guest block driver
Date: Mon, 12 Feb 2007 05:43:39 +0100	[thread overview]
Message-ID: <20070212044339.GJ3685@kernel.dk> (raw)
In-Reply-To: <1171252474.10409.42.camel@localhost.localdomain>

On Mon, Feb 12 2007, Rusty Russell wrote:
> +static irqreturn_t lgb_irq(int irq, void *_bd)
> +{
> +	struct blockdev *bd = _bd;
> +	unsigned long flags;
> +
> +	if (!bd->req) {
> +		pr_debug("No work!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	if (!bd->lb_page->result) {
> +		pr_debug("No result!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	spin_lock_irqsave(&bd->lock, flags);
> +	end_request(bd->req, bd->lb_page->result == 1);
> +	bd->req = NULL;
> +	bd->dma.used_len = 0;
> +	blk_start_queue(bd->disk->queue);
> +	spin_unlock_irqrestore(&bd->lock, flags);
> +	return IRQ_HANDLED;
> +}

You are using the old-style end request handling. So while I generally
discourage use of end_request(), you seem to have a bigger problem here:

> +static unsigned int req_to_dma(struct request *req, struct lguest_dma *dma)
> +{
> +	unsigned int i = 0, idx, len = 0;
> +	struct bio *bio;
> +
> +	rq_for_each_bio(bio, req) {
> +		struct bio_vec *bvec;
> +		bio_for_each_segment(bvec, bio, idx) {
> +			BUG_ON(i == LGUEST_MAX_DMA_SECTIONS);
> +			BUG_ON(!bvec->bv_len);
> +			dma->addr[i] = page_to_phys(bvec->bv_page)
> +				+ bvec->bv_offset;
> +			dma->len[i] = bvec->bv_len;
> +			len += bvec->bv_len;
> +			i++;
> +		}
> +	}
> +	if (i < LGUEST_MAX_DMA_SECTIONS)
> +		dma->len[i] = 0;
> +	return len;
> +}

Here you map the entire request (lets call that segment A..Z), but
end_request() only completes the first chunk of the request. So
elv_next_request() will retrieve the same request again, and you'll then
map B..Z and repeat that transfer. So unless I'm missing some other part
here (just read it over quickly), you are re-doing large parts of a
merged request several times.

So: don't use end_request(). Add some driver helper that does:

static void lgb_end_request(struct blockdev *bd)
{
        int uptodate = bd->lb_page->result == 1;
        struct request *rq = bd->req;

        end_that_request_first(rq, uptodate, req->hard_nr_sectors);
        add_disk_randomness(rq->rq_disk);
        blkdev_dequeue_request(rq);
        end_that_request_last(rq, uptodate);
}

We could probably even make that a block layer helper, I'm sure others
could be cleaned up with that as well. You want to use that helper in
do_lgb_request() as well.

-- 
Jens Axboe


  parent reply	other threads:[~2007-02-12  4:43 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-12  3:32 [PATCH 1/7] cleanup: paravirt unhandled fallthrough Rusty Russell
2007-02-12  3:33 ` [PATCH 2/7] cleanup: Initialize esp0 properly all the time Rusty Russell
2007-02-12  3:34   ` [PATCH 3/7] cleanup: Make hvc_console.c compile on non-PowerPC Rusty Russell
2007-02-12  3:35     ` [PATCH 4/7] cleanup: Move mce_disabled to asm/mce.h Rusty Russell
2007-02-12  3:36       ` [PATCH 5/7] cleanup: Rename cpu_gdt_descr and remove extern declaration from smpboot.c Rusty Russell
2007-02-12  3:37         ` [PATCH 6/7] cleanup: Remove extern declaration from mm/discontig.c, put in header Rusty Russell
2007-02-12  3:39           ` [PATCH 7/7] cleanup: make disable_acpi() valid w/o CONFIG_ACPI Rusty Russell
2007-02-12  3:41             ` [PATCH 1/2] lguest preparation: EXPORT_SYMBOL_GPL 5 functions Rusty Russell
2007-02-12  3:42               ` [PATCH 2/2] lguest preparation: expose futex infrastructure: get_futex_key, get_key_refs and drop_key_refs Rusty Russell
2007-02-12  3:44                 ` [PATCH 1/8] lguest: Kconfig and headers Rusty Russell
2007-02-12  3:46                   ` [PATCH 2/8] lguest: the host code (lg.ko) Rusty Russell
2007-02-12  3:48                     ` [PATCH 3/8] lguest: Guest code Rusty Russell
2007-02-12  3:50                       ` [PATCH 4/8] lguest: Makefile Rusty Russell
2007-02-12  3:52                         ` [PATCH 5/8] lguest: trivial guest network driver Rusty Russell
2007-02-12  3:53                           ` [PATCH 6/8] lguest: trivial guest console driver Rusty Russell
2007-02-12  3:54                             ` [PATCH 7/8] lguest: trivial guest block driver Rusty Russell
2007-02-12  3:55                               ` [PATCH 8/8] lguest: documentatation and example launcher Rusty Russell
2007-02-12  4:43                               ` Jens Axboe [this message]
2007-02-12  5:27                                 ` [PATCH 7/8] lguest: trivial guest block driver Rusty Russell
2007-02-12  5:32                                   ` Jens Axboe
2007-02-12  5:33                                     ` Jens Axboe
2007-02-12  7:09                                     ` Rusty Russell
2007-02-12 15:01                                       ` Jens Axboe
2007-02-13  0:25                                         ` Rusty Russell
2007-02-13  0:44                                           ` Jens Axboe
2007-02-12 15:55                           ` [PATCH 5/8] lguest: trivial guest network driver Herbert Xu
2007-02-13  2:15                             ` Rusty Russell
2007-02-13 14:06                               ` Herbert Xu
2007-02-14  4:47                                 ` Rusty Russell
2007-02-14 13:57                                   ` Herbert Xu
2007-02-14 23:00                                     ` Rusty Russell
2007-02-12 16:02                   ` [PATCH 1/8] lguest: Kconfig and headers James Morris
2007-02-13  5:09             ` [PATCH 7/7] cleanup: make disable_acpi() valid w/o CONFIG_ACPI Len Brown
2007-02-12  9:16         ` [PATCH 5/7] cleanup: Rename cpu_gdt_descr and remove extern declaration from smpboot.c Zachary Amsden

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070212044339.GJ3685@kernel.dk \
    --to=jens.axboe@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.osdl.org \
    --subject='Re: [PATCH 7/8] lguest: trivial guest block driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).