FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 02-23-2010, 06:52 PM
Mikulas Patocka
 
Default dm mpath: fix stall when requeueing io

On Tue, 23 Feb 2010, Alasdair G Kergon wrote:

> > >> spin_unlock(q->queue_lock);
> > >> - map_request(ti, rq, md);
> > >> + if (map_request(ti, rq, md))
> > >> + goto requeued;
> > >> spin_lock_irq(q->queue_lock);
>
> > In the current device-mapper code, I would like to go with
> > spin_unlock/lock here.
> > However, there was a case to enable irq in map_requst() for request
> > allocation, and this spin_lock_irq was a work-around for the case.
> > Now, there is no such case in the device-mapper code, so spin_lock should
> > be enough here. But I'm still using spin_lock_irq for safeness, since
> > there might be some more cases to enable irq during request submission
> > to underlying devices.

Either map_requst() may enable irqs --- then you should enable them with
spin_unlock_irq (then, you'd have to review all the callers of
dm_request_fn that they are fine with enabling irqs).

Or map_request() must not enable irqs --- and then there is already a bug
and there is no point in using "spin_lock_irq" for safeness, because it
doesn't fix the bug (the interrupt may come anyway, before spin_lock_irq).

> I don't understand this.
>
> Are you saying that sometimes this fn is called with local interrupts disabled
> and other times with them still enabled?
>
> If they are sometimes left enabled, and the unlock() leaves them alone,
> then the lock_irq disables them, what piece of code then reenables them?
>
> Surely this code can only be working if local interrupts are always
> disabled prior to entry?
>
> Alasdair

Another problem:
dm_request_fn can be called in an interrupt context, I scanned it for
calling process-context functions and found:

It may call rq_completed (either directly, via
dm_request_fn->map_request->dm_kill_unmapped_request->dm_complete_request
->dm_done->dm_end_request->dm_put) or indirectly, when the request is
completed from host controller interrupt. And dm_put is a process_context
function.

I believe it doesn't cause a real crash, because dm_put is called in
dm_blk_close, thus there is always at least one reference. When the device
is closed with dm_blk_close, there should be no requests on it.

But it is simply a logic error to call a process-context function from
an interrupt context. I'd remove those dm_get/dm_put from
request-based-dm --- they are not needed anyway, as long as there are
requests, the "mapped_device" structure can't disappear.

You can apply this (in 2.6.34-rc1) to catch all the errorneous users of
dm_put.

Mikulas

---

Catch errorneous calls of dm_put from an interrupt context.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
drivers/md/dm.c | 1 +
1 file changed, 1 insertion(+)

Index: linux-2.6.33-rc8-fast/drivers/md/dm.c
================================================== =================
--- linux-2.6.33-rc8-fast.orig/drivers/md/dm.c 2010-02-23 20:45:31.000000000 +0100
+++ linux-2.6.33-rc8-fast/drivers/md/dm.c 2010-02-23 20:46:10.000000000 +0100
@@ -2188,6 +2188,7 @@ void dm_put(struct mapped_device *md)
struct dm_table *map;

BUG_ON(test_bit(DMF_FREEING, &md->flags));
+ might_sleep();

if (atomic_dec_and_lock(&md->holders, &_minor_lock)) {
map = dm_get_live_table(md);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 04:29 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org