Hello,
Upon ack, please pull from the following git tree.
git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git tj-block-for-linus
to receive the following patches.
0001-mg_disk-fix-locking.patch
0002-mg_disk-fix-CONFIG_LBD-y-warning.patch
0003-hd-fix-locking.patch
0001 and 0003 fix locking problem in mg_disk and hd. mg_disk is only
compile tested but hd received almost identical changes and is
verified to work so I'm relatively confident regarding the mg_disk.
0002 removes a build warning.
This patchset is on top of the current linux-2.6-block#for-linus[1]
and contains the following changes.
drivers/block/hd.c | 17 +++++++----------
drivers/block/mg_disk.c | 19 ++++++++++++++-----
2 files changed, 21 insertions(+), 15 deletions(-)
Thanks.
--
tejun
[1] f2d1f0ae7851be5ebd9613a80dac139270938809
hd dance around local irq and HD_IRQ enable without achieving much.
It ends up transferring data from irq handler with both local irq and
HD_IRQ disabled. The only place it actually does something is while
transferring the first block of a request which it does with HD_IRQ
disabled but local irq enabled.
Unfortunately, the dancing is horribly broken from locking POV. IRQ
and timeout handlers access block queue without grabbing the queue
lock and running the driver in SMP configuration crashes the whole
machine pretty quickly.
Remove meaningless irq enable/disable dancing and add proper locking
in issue, irq and timeout paths.
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/block/hd.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/block/hd.c b/drivers/block/hd.c
index 3c11f06..baaa9e4 100644
--- a/drivers/block/hd.c
+++ b/drivers/block/hd.c
@@ -509,7 +509,6 @@ ok_to_write:
if (i > 0) {
SET_HANDLER(&write_intr);
outsw(HD_DATA, req->buffer, 256);
- local_irq_enable();
} else {
#if (HD_DELAY > 0)
last_req = read_timer();
@@ -541,8 +540,7 @@ static void hd_times_out(unsigned long dummy)
if (!CURRENT)
return;
- disable_irq(HD_IRQ);
- local_irq_enable();
+ spin_lock_irq(hd_queue->queue_lock);
reset = 1;
name = CURRENT->rq_disk->disk_name;
printk("%s: timeout\n", name);
@@ -552,9 +550,8 @@ static void hd_times_out(unsigned long dummy)
#endif
end_request(CURRENT, 0);
}
- local_irq_disable();
hd_request();
- enable_irq(HD_IRQ);
+ spin_unlock_irq(hd_queue->queue_lock);
}
static int do_special_op(struct hd_i_struct *disk, struct request *req)
@@ -592,7 +589,6 @@ static void hd_request(void)
return;
repeat:
del_timer(&device_timer);
- local_irq_enable();
req = CURRENT;
if (!req) {
@@ -601,7 +597,6 @@ repeat:
}
if (reset) {
- local_irq_disable();
reset_hd();
return;
}
@@ -660,9 +655,7 @@ repeat:
static void do_hd_request(struct request_queue *q)
{
- disable_irq(HD_IRQ);
hd_request();
- enable_irq(HD_IRQ);
}
static int hd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
@@ -684,12 +677,16 @@ static irqreturn_t hd_interrupt(int irq, void *dev_id)
{
void (*handler)(void) = do_hd;
+ spin_lock(hd_queue->queue_lock);
+
do_hd = NULL;
del_timer(&device_timer);
if (!handler)
handler = unexpected_hd_interrupt;
handler();
- local_irq_enable();
+
+ spin_unlock(hd_queue->queue_lock);
+
return IRQ_HANDLED;
}
--
1.6.0.2
IRQ and timeout handlers call functions which expect locked queue lock
without locking it. Fix it.
While at it, convert 0s used as null pointer constant to NULLs.
[ Impact: fix locking, cleanup ]
Signed-off-by: Tejun Heo <[email protected]>
Cc: unsik Kim <[email protected]>
---
drivers/block/mg_disk.c | 17 +++++++++++++----
1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
index fb39d9a..d3e72ad 100644
--- a/drivers/block/mg_disk.c
+++ b/drivers/block/mg_disk.c
@@ -160,11 +160,16 @@ static irqreturn_t mg_irq(int irq, void *dev_id)
struct mg_host *host = dev_id;
void (*handler)(struct mg_host *) = host->mg_do_intr;
- host->mg_do_intr = 0;
+ spin_lock(&host->lock);
+
+ host->mg_do_intr = NULL;
del_timer(&host->timer);
if (!handler)
handler = mg_unexpected_intr;
handler(host);
+
+ spin_unlock(&host->lock);
+
return IRQ_HANDLED;
}
@@ -319,7 +324,7 @@ static void mg_read(struct request *req)
remains = req->nr_sectors;
- if (mg_out(host, req->sector, req->nr_sectors, MG_CMD_RD, 0) !=
+ if (mg_out(host, req->sector, req->nr_sectors, MG_CMD_RD, NULL) !=
MG_ERR_NONE)
mg_bad_rw_intr(host);
@@ -363,7 +368,7 @@ static void mg_write(struct request *req)
remains = req->nr_sectors;
- if (mg_out(host, req->sector, req->nr_sectors, MG_CMD_WR, 0) !=
+ if (mg_out(host, req->sector, req->nr_sectors, MG_CMD_WR, NULL) !=
MG_ERR_NONE) {
mg_bad_rw_intr(host);
return;
@@ -521,9 +526,11 @@ void mg_times_out(unsigned long data)
char *name;
struct request *req;
+ spin_lock_irq(&host->lock);
+
req = elv_next_request(host->breq);
if (!req)
- return;
+ goto out_unlock;
host->mg_do_intr = NULL;
@@ -534,6 +541,8 @@ void mg_times_out(unsigned long data)
mg_bad_rw_intr(host);
mg_request(host->breq);
+out_unlock:
+ spin_unlock_irq(&host->lock);
}
static void mg_request_poll(struct request_queue *q)
--
1.6.0.2
From: Bartlomiej Zolnierkiewicz <[email protected]>
drivers/block/mg_disk.c: In function ‘mg_dump_status’:
drivers/block/mg_disk.c:265: warning: format ‘%ld’ expects type ‘long int’, but
argument 2 has type ‘sector_t’
[ Impact: kill build warning ]
Cc: unsik Kim <[email protected]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
Signed-off-by: Tejun Heo <[email protected]>
---
drivers/block/mg_disk.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
index d3e72ad..f389835 100644
--- a/drivers/block/mg_disk.c
+++ b/drivers/block/mg_disk.c
@@ -79,7 +79,7 @@ static void mg_dump_status(const char *msg, unsigned int stat,
if (host->breq) {
req = elv_next_request(host->breq);
if (req)
- printk(", sector=%ld", req->sector);
+ printk(", sector=%u", (u32)req->sector);
}
}
--
1.6.0.2
On Tue, Apr 28 2009, Tejun Heo wrote:
> Hello,
>
> Upon ack, please pull from the following git tree.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git tj-block-for-linus
>
> to receive the following patches.
>
> 0001-mg_disk-fix-locking.patch
> 0002-mg_disk-fix-CONFIG_LBD-y-warning.patch
> 0003-hd-fix-locking.patch
>
> 0001 and 0003 fix locking problem in mg_disk and hd. mg_disk is only
> compile tested but hd received almost identical changes and is
> verified to work so I'm relatively confident regarding the mg_disk.
>
> 0002 removes a build warning.
>
> This patchset is on top of the current linux-2.6-block#for-linus[1]
> and contains the following changes.
>
> drivers/block/hd.c | 17 +++++++----------
> drivers/block/mg_disk.c | 19 ++++++++++++++-----
> 2 files changed, 21 insertions(+), 15 deletions(-)
Thanks Tejun, pulled!
--
Jens Axboe
Tejun Heo wrote:
> From: Bartlomiej Zolnierkiewicz <[email protected]>
>
> drivers/block/mg_disk.c: In function ‘mg_dump_status’:
> drivers/block/mg_disk.c:265: warning: format ‘%ld’ expects type ‘long int’, but
> argument 2 has type ‘sector_t’
>
> [ Impact: kill build warning ]
>
> Cc: unsik Kim <[email protected]>
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> Signed-off-by: Tejun Heo <[email protected]>
> ---
> drivers/block/mg_disk.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
> index d3e72ad..f389835 100644
> --- a/drivers/block/mg_disk.c
> +++ b/drivers/block/mg_disk.c
> @@ -79,7 +79,7 @@ static void mg_dump_status(const char *msg, unsigned int stat,
> if (host->breq) {
> req = elv_next_request(host->breq);
> if (req)
> - printk(", sector=%ld", req->sector);
> + printk(", sector=%u", (u32)req->sector);
..
Eh? Shouldn't that be fixed the other way around, like this:
+ printk(", sector=%llu", (u64)req->sector);
This way, it will still give correct data when sector_t is a u64.
???
On Tuesday 28 April 2009 17:19:34 Mark Lord wrote:
> Tejun Heo wrote:
> > From: Bartlomiej Zolnierkiewicz <[email protected]>
> >
> > drivers/block/mg_disk.c: In function ‘mg_dump_status’:
> > drivers/block/mg_disk.c:265: warning: format ‘%ld’ expects type ‘long int’, but
> > argument 2 has type ‘sector_t’
> >
> > [ Impact: kill build warning ]
> >
> > Cc: unsik Kim <[email protected]>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> > Signed-off-by: Tejun Heo <[email protected]>
> > ---
> > drivers/block/mg_disk.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
> > index d3e72ad..f389835 100644
> > --- a/drivers/block/mg_disk.c
> > +++ b/drivers/block/mg_disk.c
> > @@ -79,7 +79,7 @@ static void mg_dump_status(const char *msg, unsigned int stat,
> > if (host->breq) {
> > req = elv_next_request(host->breq);
> > if (req)
> > - printk(", sector=%ld", req->sector);
> > + printk(", sector=%u", (u32)req->sector);
> ..
>
> Eh? Shouldn't that be fixed the other way around, like this:
>
> + printk(", sector=%llu", (u64)req->sector);
>
> This way, it will still give correct data when sector_t is a u64.
shouldn't matter, req->sector is never > u32 for mg_disk
Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 28 April 2009 17:19:34 Mark Lord wrote:
>> Tejun Heo wrote:
>>> From: Bartlomiej Zolnierkiewicz <[email protected]>
>>>
>>> drivers/block/mg_disk.c: In function ‘mg_dump_status’:
>>> drivers/block/mg_disk.c:265: warning: format ‘%ld’ expects type ‘long int’, but
>>> argument 2 has type ‘sector_t’
>>>
>>> [ Impact: kill build warning ]
>>>
>>> Cc: unsik Kim <[email protected]>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
>>> Signed-off-by: Tejun Heo <[email protected]>
>>> ---
>>> drivers/block/mg_disk.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
>>> index d3e72ad..f389835 100644
>>> --- a/drivers/block/mg_disk.c
>>> +++ b/drivers/block/mg_disk.c
>>> @@ -79,7 +79,7 @@ static void mg_dump_status(const char *msg, unsigned int stat,
>>> if (host->breq) {
>>> req = elv_next_request(host->breq);
>>> if (req)
>>> - printk(", sector=%ld", req->sector);
>>> + printk(", sector=%u", (u32)req->sector);
>> ..
>>
>> Eh? Shouldn't that be fixed the other way around, like this:
>>
>> + printk(", sector=%llu", (u64)req->sector);
>>
>> This way, it will still give correct data when sector_t is a u64.
>
> shouldn't matter, req->sector is never > u32 for mg_disk
It never matters... until the code gets copied elsewhere. IMO wrong
code should never be kept -- "impossible to hit" just means it is low
priority :)
Jeff
On Tuesday 28 April 2009 20:09:54 Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 28 April 2009 17:19:34 Mark Lord wrote:
> >> Tejun Heo wrote:
> >>> From: Bartlomiej Zolnierkiewicz <[email protected]>
> >>>
> >>> drivers/block/mg_disk.c: In function ‘mg_dump_status’:
> >>> drivers/block/mg_disk.c:265: warning: format ‘%ld’ expects type ‘long int’, but
> >>> argument 2 has type ‘sector_t’
> >>>
> >>> [ Impact: kill build warning ]
> >>>
> >>> Cc: unsik Kim <[email protected]>
> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> >>> Signed-off-by: Tejun Heo <[email protected]>
> >>> ---
> >>> drivers/block/mg_disk.c | 2 +-
> >>> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
> >>> index d3e72ad..f389835 100644
> >>> --- a/drivers/block/mg_disk.c
> >>> +++ b/drivers/block/mg_disk.c
> >>> @@ -79,7 +79,7 @@ static void mg_dump_status(const char *msg, unsigned int stat,
> >>> if (host->breq) {
> >>> req = elv_next_request(host->breq);
> >>> if (req)
> >>> - printk(", sector=%ld", req->sector);
> >>> + printk(", sector=%u", (u32)req->sector);
> >> ..
> >>
> >> Eh? Shouldn't that be fixed the other way around, like this:
> >>
> >> + printk(", sector=%llu", (u64)req->sector);
> >>
> >> This way, it will still give correct data when sector_t is a u64.
> >
> > shouldn't matter, req->sector is never > u32 for mg_disk
>
> It never matters... until the code gets copied elsewhere. IMO wrong
> code should never be kept -- "impossible to hit" just means it is low
> priority :)
Well, if you feel strongly about it feel free to replace my patch with
your improved version. You may also fix hd.c (from which the above code
has been copied) while at it.