Re: [PATCH V2] umem: fix up unplugging

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

On Thu,  7 Jun 2012 13:07:15 -0400 Tao Guo <glorioustao@gmail.com> wrote:

> Fix a regression introduced by 7eaceaccab5f40 ("block: remove per-queue
> plugging").  In that patch, Jens removed the whole mm_unplug_device()
> function, which used to be the trigger to make umem start to work.
> 
> We need to implement unplugging to make umem start to work, or I/O will
> never be triggered.
> 
> Signed-off-by: Tao Guo <Tao.Guo@emc.com>
> Cc: Neil Brown <neilb@suse.de>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: <stable@vger.kernel.org>

This is certainly a simpler patch that the first 2 and that it important if
it is heading for 3.4.y.
It looks like it will do what it should so assuming you have tested it (which
I'm sure you have)

  Acked-by: NeilBrown <neilb@suse.de>

Longer term I think it does make sense to move some of this plugging code
into block/blk-XXX so that it can be shared by umem and md, and improved to
serve them both better.

In particular I think that the _check_plugged function should return the 
struct xx_plug_cb structure (rather than 'true' or 'false') and the
make_request function should then link any new requests into that structure.
The unplug function can then activate that list of requests in whatever way
suits the particular device.
This will ensure that if separate threads are submitting requests at the same
time, they won't compete with each other, and an unplug on one thread won't
release the requests that are plugged by the other.

NeilBrown

> ---
>  drivers/block/umem.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/umem.c b/drivers/block/umem.c
> index aa27120..9a72277 100644
> --- a/drivers/block/umem.c
> +++ b/drivers/block/umem.c
> @@ -513,6 +513,44 @@ static void process_page(unsigned long data)
>  	}
>  }
>  
> +struct mm_plug_cb {
> +	struct blk_plug_cb cb;
> +	struct cardinfo *card;
> +};
> +
> +static void mm_unplug(struct blk_plug_cb *cb)
> +{
> +	struct mm_plug_cb *mmcb = container_of(cb, struct mm_plug_cb, cb);
> +
> +	spin_lock_irq(&mmcb->card->lock);
> +	activate(mmcb->card);
> +	spin_unlock_irq(&mmcb->card->lock);
> +	kfree(mmcb);
> +}
> +
> +static int mm_check_plugged(struct cardinfo *card)
> +{
> +	struct blk_plug *plug = current->plug;
> +	struct mm_plug_cb *mmcb;
> +
> +	if (!plug)
> +		return 0;
> +
> +	list_for_each_entry(mmcb, &plug->cb_list, cb.list) {
> +		if (mmcb->cb.callback == mm_unplug && mmcb->card == card)
> +			return 1;
> +	}
> +	/* Not currently on the callback list */
> +	mmcb = kmalloc(sizeof(*mmcb), GFP_ATOMIC);
> +	if (!mmcb)
> +		return 0;
> +
> +	mmcb->card = card;
> +	mmcb->cb.callback = mm_unplug;
> +	list_add(&mmcb->cb.list, &plug->cb_list);
> +	return 1;
> +}
> +
>  static void mm_make_request(struct request_queue *q, struct bio *bio)
>  {
>  	struct cardinfo *card = q->queuedata;
> @@ -523,6 +561,8 @@ static void mm_make_request(struct request_queue *q, struct bio *bio)
>  	*card->biotail = bio;
>  	bio->bi_next = NULL;
>  	card->biotail = &bio->bi_next;
> +	if (bio->bi_rw & REQ_SYNC || !mm_check_plugged(card))
> +		activate(card);
>  	spin_unlock_irq(&card->lock);
>  
>  	return;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]