[PATCH] umem: fix up unplugging.

[PATCH] umem: fix up unplugging.

[Tao Guo]

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>
---
 drivers/block/umem.c |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/drivers/block/umem.c b/drivers/block/umem.c
index aa27120..c3bf98e 100644
--- a/drivers/block/umem.c
+++ b/drivers/block/umem.c
@@ -111,7 +111,8 @@ struct cardinfo {
 	int		current_idx;
 	sector_t	current_sector;
 
-	struct request_queue *queue;
+	struct request_queue	*queue;
+	struct blk_plug_cb	plug_cb;
 
 	struct mm_page {
 		dma_addr_t		page_dma;
@@ -513,6 +514,33 @@ static void process_page(unsigned long data)
 	}
 }
 
+static void mm_unplug(struct blk_plug_cb *cb)
+{
+	struct cardinfo *card = container_of(cb, struct cardinfo, plug_cb);
+
+	spin_lock_irq(&card->lock);
+	activate(card);
+	spin_unlock_irq(&card->lock);
+}
+
+int mm_check_plugged(struct cardinfo *card)
+{
+	struct blk_plug *plug = current->plug;
+	struct cardinfo *c;
+
+	if (!plug)
+		return 0;
+
+	list_for_each_entry(c, &plug->cb_list, plug_cb.list) {
+		if (c == card)
+			return 1;
+	}
+
+	/* set up unplug callback */
+	list_add(&card->plug_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 +551,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;
@@ -884,6 +914,7 @@ static int __devinit mm_pci_probe(struct pci_dev *dev,
 	blk_queue_make_request(card->queue, mm_make_request);
 	card->queue->queue_lock = &card->lock;
 	card->queue->queuedata = card;
+	card->plug_cb.callback = mm_unplug;
 
 	tasklet_init(&card->tasklet, process_page, (unsigned long)card);
 
-- 
1.7.7

Re: [PATCH] umem: fix up unplugging.

[Andrew Morton]

On Fri, 30 Dec 2011 09:17:31 -0500

(Long delay...)

Tao Guo <glorioustao@gmail.com> wrote:

> We need to implement unplugging to make umem start to work, or I/O
> will never be triggered.
> 

I take it from this that umem simply doesn't work at all?

If so, for how long has this been the case and should we just delete the
thing?

> 
> --- a/drivers/block/umem.c
> +++ b/drivers/block/umem.c
> @@ -111,7 +111,8 @@ struct cardinfo {
>  	int		current_idx;
>  	sector_t	current_sector;
>  
> -	struct request_queue *queue;
> +	struct request_queue	*queue;
> +	struct blk_plug_cb	plug_cb;
>  
>  	struct mm_page {
>  		dma_addr_t		page_dma;
> @@ -513,6 +514,33 @@ static void process_page(unsigned long data)
>  	}
>  }
>  
> +static void mm_unplug(struct blk_plug_cb *cb)
> +{
> +	struct cardinfo *card = container_of(cb, struct cardinfo, plug_cb);
> +
> +	spin_lock_irq(&card->lock);
> +	activate(card);
> +	spin_unlock_irq(&card->lock);
> +}
> +
> +int mm_check_plugged(struct cardinfo *card)
> +{
> +	struct blk_plug *plug = current->plug;
> +	struct cardinfo *c;
> +
> +	if (!plug)
> +		return 0;
> +
> +	list_for_each_entry(c, &plug->cb_list, plug_cb.list) {
> +		if (c == card)
> +			return 1;
> +	}
> +
> +	/* set up unplug callback */
> +	list_add(&card->plug_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 +551,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;
> @@ -884,6 +914,7 @@ static int __devinit mm_pci_probe(struct pci_dev *dev,
>  	blk_queue_make_request(card->queue, mm_make_request);
>  	card->queue->queue_lock = &card->lock;
>  	card->queue->queuedata = card;
> +	card->plug_cb.callback = mm_unplug;
>  
>  	tasklet_init(&card->tasklet, process_page, (unsigned long)card);

Re: [PATCH] umem: fix up unplugging.

[Tao Guo]

Hi Andrew,

Thanks for your reply.

Yes, without this patch the umem driver just doesn't work.
It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
In that patch, Jens removed the whole mm_unplug_device() function,
which used to be
the trigger to make umem start to work.

Thanks,
-Tao


On Wed, Apr 4, 2012 at 2:46 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 30 Dec 2011 09:17:31 -0500
>
> (Long delay...)
>
> Tao Guo <glorioustao@gmail.com> wrote:
>
>> We need to implement unplugging to make umem start to work, or I/O
>> will never be triggered.
>>
>
> I take it from this that umem simply doesn't work at all?
>
> If so, for how long has this been the case and should we just delete the
> thing?
>
>>
>> --- a/drivers/block/umem.c
>> +++ b/drivers/block/umem.c
>> @@ -111,7 +111,8 @@ struct cardinfo {
>>       int             current_idx;
>>       sector_t        current_sector;
>>
>> -     struct request_queue *queue;
>> +     struct request_queue    *queue;
>> +     struct blk_plug_cb      plug_cb;
>>
>>       struct mm_page {
>>               dma_addr_t              page_dma;
>> @@ -513,6 +514,33 @@ static void process_page(unsigned long data)
>>       }
>>  }
>>
>> +static void mm_unplug(struct blk_plug_cb *cb)
>> +{
>> +     struct cardinfo *card = container_of(cb, struct cardinfo, plug_cb);
>> +
>> +     spin_lock_irq(&card->lock);
>> +     activate(card);
>> +     spin_unlock_irq(&card->lock);
>> +}
>> +
>> +int mm_check_plugged(struct cardinfo *card)
>> +{
>> +     struct blk_plug *plug = current->plug;
>> +     struct cardinfo *c;
>> +
>> +     if (!plug)
>> +             return 0;
>> +
>> +     list_for_each_entry(c, &plug->cb_list, plug_cb.list) {
>> +             if (c == card)
>> +                     return 1;
>> +     }
>> +
>> +     /* set up unplug callback */
>> +     list_add(&card->plug_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 +551,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;
>> @@ -884,6 +914,7 @@ static int __devinit mm_pci_probe(struct pci_dev *dev,
>>       blk_queue_make_request(card->queue, mm_make_request);
>>       card->queue->queue_lock = &card->lock;
>>       card->queue->queuedata = card;
>> +     card->plug_cb.callback = mm_unplug;
>>
>>       tasklet_init(&card->tasklet, process_page, (unsigned long)card);
>

Re: [PATCH] umem: fix up unplugging.

[Jens Axboe]

On 2012-04-04 16:20, Tao Guo wrote:
> Hi Andrew,
> 
> Thanks for your reply.
> 
> Yes, without this patch the umem driver just doesn't work.
> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
> In that patch, Jens removed the whole mm_unplug_device() function,
> which used to be
> the trigger to make umem start to work.

Hmm indeed, that's isn't terribly useful. Why aren't we just calling
activate_card() on addition of a bio?

But your patch looks fine too, if it's tested it beats what we have.

-- 
Jens Axboe

Re: [PATCH] umem: fix up unplugging.

[Tao Guo]

On Wed, Apr 4, 2012 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2012-04-04 16:20, Tao Guo wrote:
>> Hi Andrew,
>>
>> Thanks for your reply.
>>
>> Yes, without this patch the umem driver just doesn't work.
>> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
>> In that patch, Jens removed the whole mm_unplug_device() function,
>> which used to be
>> the trigger to make umem start to work.
>
> Hmm indeed, that's isn't terribly useful. Why aren't we just calling
> activate_card() on addition of a bio?
>
It is the original design idea, the umem driver also wants to do batch
IO the same as
hard disk drive to fully utilize the IO bandwidth.

Thanks,
-Tao
> But your patch looks fine too, if it's tested it beats what we have.
>
> --
> Jens Axboe
>

Re: [PATCH] umem: fix up unplugging.

[Jens Axboe]

On 2012-04-04 17:58, Tao Guo wrote:
> On Wed, Apr 4, 2012 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2012-04-04 16:20, Tao Guo wrote:
>>> Hi Andrew,
>>>
>>> Thanks for your reply.
>>>
>>> Yes, without this patch the umem driver just doesn't work.
>>> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
>>> In that patch, Jens removed the whole mm_unplug_device() function,
>>> which used to be
>>> the trigger to make umem start to work.
>>
>> Hmm indeed, that's isn't terribly useful. Why aren't we just calling
>> activate_card() on addition of a bio?
>>
> It is the original design idea, the umem driver also wants to do batch
> IO the same as
> hard disk drive to fully utilize the IO bandwidth.

Sure, just wondering whether the benefits have been tested. But yes, it
certainly makes sense.

Your patch looks buggy, though. What if the card is currently plugged on
another tasks plug list? You seem to assume that it can only be plugged
once.

-- 
Jens Axboe

Re: [PATCH] umem: fix up unplugging.

[Tao Guo]

On Thu, Apr 5, 2012 at 4:51 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2012-04-04 17:58, Tao Guo wrote:
>> On Wed, Apr 4, 2012 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 2012-04-04 16:20, Tao Guo wrote:
>>>> Hi Andrew,
>>>>
>>>> Thanks for your reply.
>>>>
>>>> Yes, without this patch the umem driver just doesn't work.
>>>> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
>>>> In that patch, Jens removed the whole mm_unplug_device() function,
>>>> which used to be
>>>> the trigger to make umem start to work.
>>>
>>> Hmm indeed, that's isn't terribly useful. Why aren't we just calling
>>> activate_card() on addition of a bio?
>>>
>> It is the original design idea, the umem driver also wants to do batch
>> IO the same as
>> hard disk drive to fully utilize the IO bandwidth.
>
> Sure, just wondering whether the benefits have been tested. But yes, it
> certainly makes sense.
>
> Your patch looks buggy, though. What if the card is currently plugged on
> another tasks plug list? You seem to assume that it can only be plugged
> once.
No, it will register in its own plug callback list successfully, so
the unplug functions
will be called separately(twice the same).
-Tao
>
> --
> Jens Axboe
>

Re: [PATCH] umem: fix up unplugging.

[Jens Axboe]

On 2012-04-05 22:09, Tao Guo wrote:
> On Thu, Apr 5, 2012 at 4:51 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2012-04-04 17:58, Tao Guo wrote:
>>> On Wed, Apr 4, 2012 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 2012-04-04 16:20, Tao Guo wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> Thanks for your reply.
>>>>>
>>>>> Yes, without this patch the umem driver just doesn't work.
>>>>> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
>>>>> In that patch, Jens removed the whole mm_unplug_device() function,
>>>>> which used to be
>>>>> the trigger to make umem start to work.
>>>>
>>>> Hmm indeed, that's isn't terribly useful. Why aren't we just calling
>>>> activate_card() on addition of a bio?
>>>>
>>> It is the original design idea, the umem driver also wants to do batch
>>> IO the same as
>>> hard disk drive to fully utilize the IO bandwidth.
>>
>> Sure, just wondering whether the benefits have been tested. But yes, it
>> certainly makes sense.
>>
>> Your patch looks buggy, though. What if the card is currently plugged on
>> another tasks plug list? You seem to assume that it can only be plugged
>> once.
> No, it will register in its own plug callback list successfully, so
> the unplug functions
> will be called separately(twice the same).

So the code loops through the current plug, checking if it's already on
that list. If not, then it adds it. But what if it's on another plug
list? The above loop could be done with just a

        if (!list_empty(&card->plug_cb.list))
                ...

instead. Right now you could have !list_empty(&card->plug_cb.list) but
still add it to a different list.

-- 
Jens Axboe

Re: [PATCH] umem: fix up unplugging.

[Tao Guo]

On Thu, Apr 5, 2012 at 9:19 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2012-04-05 22:09, Tao Guo wrote:
>> On Thu, Apr 5, 2012 at 4:51 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 2012-04-04 17:58, Tao Guo wrote:
>>>> On Wed, Apr 4, 2012 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 2012-04-04 16:20, Tao Guo wrote:
>>>>>> Hi Andrew,
>>>>>>
>>>>>> Thanks for your reply.
>>>>>>
>>>>>> Yes, without this patch the umem driver just doesn't work.
>>>>>> It is a bug introduced by commit 7eaceaccab5f40bbfda044629a6298616aeaed50.
>>>>>> In that patch, Jens removed the whole mm_unplug_device() function,
>>>>>> which used to be
>>>>>> the trigger to make umem start to work.
>>>>>
>>>>> Hmm indeed, that's isn't terribly useful. Why aren't we just calling
>>>>> activate_card() on addition of a bio?
>>>>>
>>>> It is the original design idea, the umem driver also wants to do batch
>>>> IO the same as
>>>> hard disk drive to fully utilize the IO bandwidth.
>>>
>>> Sure, just wondering whether the benefits have been tested. But yes, it
>>> certainly makes sense.
>>>
>>> Your patch looks buggy, though. What if the card is currently plugged on
>>> another tasks plug list? You seem to assume that it can only be plugged
>>> once.
>> No, it will register in its own plug callback list successfully, so
>> the unplug functions
>> will be called separately(twice the same).
>
> So the code loops through the current plug, checking if it's already on
> that list. If not, then it adds it. But what if it's on another plug
> list? The above loop could be done with just a
>
>        if (!list_empty(&card->plug_cb.list))
>                ...
>
> instead. Right now you could have !list_empty(&card->plug_cb.list) but
> still add it to a different list.
>
mmm, I was stupid here: card->plug_cb can not be in two list the same
time. But I still need to make sure the final unplug will trigger the
activate() function.
I will sent another version to fix this problem.

Thanks,
-Tao
> --
> Jens Axboe
>