[GIT PATCH linux-2.6-block#for-linus] block: fix hd and mg_disk

[GIT PATCH linux-2.6-block#for-linus] block: fix hd and mg_disk

[Tejun Heo]

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

[PATCH 1/3] mg_disk: fix locking

[Tejun Heo]

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 <tj@kernel.org>
Cc: unsik Kim <donari75@gmail.com>
---
 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

[PATCH 2/3] mg_disk: fix CONFIG_LBD=y warning

[Tejun Heo]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

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 <donari75@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 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

Re: [PATCH 2/3] mg_disk: fix CONFIG_LBD=y warning

[Mark Lord]

Tejun Heo wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> 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 <donari75@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  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.

???

Re: [PATCH 2/3] mg_disk: fix CONFIG_LBD=y warning

[Bartlomiej Zolnierkiewicz]

On Tuesday 28 April 2009 17:19:34 Mark Lord wrote:
> Tejun Heo wrote:
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > 
> > 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 <donari75@gmail.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > ---
> >  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

Re: [PATCH 2/3] mg_disk: fix CONFIG_LBD=y warning

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 28 April 2009 17:19:34 Mark Lord wrote:
>> Tejun Heo wrote:
>>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>>
>>> 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 <donari75@gmail.com>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> ---
>>>  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

Re: [PATCH 2/3] mg_disk: fix CONFIG_LBD=y warning

[Bartlomiej Zolnierkiewicz]

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 <bzolnier@gmail.com>
> >>>
> >>> 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 <donari75@gmail.com>
> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>> Signed-off-by: Tejun Heo <tj@kernel.org>
> >>> ---
> >>>  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.

[PATCH 3/3] hd: fix locking

[Tejun Heo]

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 <tj@kernel.org>
---
 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

Re: [GIT PATCH linux-2.6-block#for-linus] block: fix hd and mg_disk

[Jens Axboe]

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