[RFT PATCH] amiga, atari floppy: Use one request queue per disk

[RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Vivek Goyal]

Hi,

We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.

There see to be still some drivers sharing request queue across disks. Arch
specific floppy drivers like amiga and atari are doing so.

These are two patches which should fix the issue. But these patches are
completely untested. Not even compilte tested. Don't have hardware to test
them. 

Would be great if somebody who has the hardware can lend a hand here to
see if these patches work.

Thanks
Vivek

[PATCH 1/2] amiga floppy: Stop sharing request queue across multiple gendisks

[Vivek Goyal]

o Use one request queue per gendisk instead of sharing request queue

o Don't have hardware. No compile testing or run time testing done. Completely
  untested.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/amiflop.c |   59 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 76f114f..ead8b77 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -114,8 +114,6 @@ static unsigned long int fd_def_df0 = FD_DD_3;     /* default for df0 if it does
 module_param(fd_def_df0, ulong, 0);
 MODULE_LICENSE("GPL");
 
-static struct request_queue *floppy_queue;
-
 /*
  *  Macros
  */
@@ -164,6 +162,7 @@ static volatile int selected = -1;	/* currently selected drive */
 static int writepending;
 static int writefromint;
 static char *raw_buf;
+static int fdc_queue;
 
 static DEFINE_SPINLOCK(amiflop_lock);
 
@@ -1334,6 +1333,42 @@ static int get_track(int drive, int track)
 	return -1;
 }
 
+/*
+ * Round-robin between our available drives, doing one request from each
+ */
+static struct request *set_next_request(void)
+{
+	struct request_queue *q;
+	int cnt = FD_MAX_UNITS;
+	struct request *rq;
+
+	/* Find next queue we can dispatch from */
+	fdc_queue = fdc_queue + 1;
+	if (fdc_queue == FD_MAX_UNITS)
+		fdc_queue = 0;
+
+	for(cnt = FD_MAX_UNITS; cnt > 0, cnt--) {
+
+		if (unit[fdc_queue].type->code == FD_NODRIVE) {
+			if (++fdc_queue == FD_MAX_UNITS)
+				fdc_queue = 0;
+			cotinue;
+		}
+
+		q = unit[fdc_queue].gendisk->queue;
+		if (q) {
+			rq = blk_fetch_request(q);
+			if (rq)
+				break;
+		}
+
+		if (++fdc_queue == FD_MAX_UNITS)
+			fdc_queue = 0;
+	}
+
+	return rq;
+}
+
 static void redo_fd_request(void)
 {
 	struct request *rq;
@@ -1345,7 +1380,7 @@ static void redo_fd_request(void)
 	int err;
 
 next_req:
-	rq = blk_fetch_request(floppy_queue);
+	rq = set_next_request();
 	if (!rq) {
 		/* Nothing left to do */
 		return;
@@ -1682,6 +1717,13 @@ static int __init fd_probe_drives(void)
 			continue;
 		}
 		unit[drive].gendisk = disk;
+
+		disk->queue = blk_init_queue(do_fd_request, &amiflop_lock);
+		if (!disk->queue) {
+			unit[drive].type->code = FD_NODRIVE;
+			continue;
+		}
+
 		drives++;
 		if ((unit[drive].trackbuf = kmalloc(FLOPPY_MAX_SECTORS * 512, GFP_KERNEL)) == NULL) {
 			printk("no mem for ");
@@ -1695,7 +1737,6 @@ static int __init fd_probe_drives(void)
 		disk->fops = &floppy_fops;
 		sprintf(disk->disk_name, "fd%d", drive);
 		disk->private_data = &unit[drive];
-		disk->queue = floppy_queue;
 		set_capacity(disk, 880*2);
 		add_disk(disk);
 	}
@@ -1743,11 +1784,6 @@ static int __init amiga_floppy_probe(struct platform_device *pdev)
 		goto out_irq2;
 	}
 
-	ret = -ENOMEM;
-	floppy_queue = blk_init_queue(do_fd_request, &amiflop_lock);
-	if (!floppy_queue)
-		goto out_queue;
-
 	ret = -ENODEV;
 	if (fd_probe_drives() < 1) /* No usable drives */
 		goto out_probe;
@@ -1791,7 +1827,6 @@ static int __init amiga_floppy_probe(struct platform_device *pdev)
 	return 0;
 
 out_probe:
-	blk_cleanup_queue(floppy_queue);
 out_queue:
 	free_irq(IRQ_AMIGA_CIAA_TB, NULL);
 out_irq2:
@@ -1810,9 +1845,12 @@ static int __exit amiga_floppy_remove(struct platform_device *pdev)
 
 	for( i = 0; i < FD_MAX_UNITS; i++) {
 		if (unit[i].type->code != FD_NODRIVE) {
+			struct request_queue *q = unit[i].gendisk->queue;
 			del_gendisk(unit[i].gendisk);
 			put_disk(unit[i].gendisk);
 			kfree(unit[i].trackbuf);
+			if (q)
+				blk_cleanup_queue(q);
 		}
 	}
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
@@ -1820,7 +1858,6 @@ static int __exit amiga_floppy_remove(struct platform_device *pdev)
 	free_irq(IRQ_AMIGA_DSKBLK, NULL);
 	custom.dmacon = DMAF_DISK; /* disable DMA */
 	amiga_chip_free(raw_buf);
-	blk_cleanup_queue(floppy_queue);
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 }
 #endif
-- 
1.7.2.3

Re: [PATCH 1/2] amiga floppy: Stop sharing request queue across multiple gendisks

[Geert Uytterhoeven]

On Thu, Sep 23, 2010 at 21:54, Vivek Goyal <vgoyal@redhat.com> wrote:
> o Use one request queue per gendisk instead of sharing request queue
>
> o Don't have hardware. No compile testing or run time testing done. Completely
>  untested.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  drivers/block/amiflop.c |   59 ++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
> index 76f114f..ead8b77 100644
> --- a/drivers/block/amiflop.c
> +++ b/drivers/block/amiflop.c
> @@ -114,8 +114,6 @@ static unsigned long int fd_def_df0 = FD_DD_3;     /* default for df0 if it does
>  module_param(fd_def_df0, ulong, 0);
>  MODULE_LICENSE("GPL");
>
> -static struct request_queue *floppy_queue;
> -
>  /*
>  *  Macros
>  */
> @@ -164,6 +162,7 @@ static volatile int selected = -1;  /* currently selected drive */
>  static int writepending;
>  static int writefromint;
>  static char *raw_buf;
> +static int fdc_queue;
>
>  static DEFINE_SPINLOCK(amiflop_lock);
>
> @@ -1334,6 +1333,42 @@ static int get_track(int drive, int track)
>        return -1;
>  }
>
> +/*
> + * Round-robin between our available drives, doing one request from each
> + */
> +static struct request *set_next_request(void)
> +{
> +       struct request_queue *q;
> +       int cnt = FD_MAX_UNITS;
> +       struct request *rq;
> +
> +       /* Find next queue we can dispatch from */
> +       fdc_queue = fdc_queue + 1;
> +       if (fdc_queue == FD_MAX_UNITS)
> +               fdc_queue = 0;
> +
> +       for(cnt = FD_MAX_UNITS; cnt > 0, cnt--) {
> +
> +               if (unit[fdc_queue].type->code == FD_NODRIVE) {
> +                       if (++fdc_queue == FD_MAX_UNITS)
> +                               fdc_queue = 0;
> +                       cotinue;
> +               }
> +
> +               q = unit[fdc_queue].gendisk->queue;
> +               if (q) {
> +                       rq = blk_fetch_request(q);
> +                       if (rq)
> +                               break;
> +               }
> +
> +               if (++fdc_queue == FD_MAX_UNITS)
> +                       fdc_queue = 0;
> +       }
> +
> +       return rq;
> +}

drivers/block/amiflop.c:1344: warning: ‘rq’ may be used uninitialized
in this function
drivers/block/ataflop.c:1402: warning: ‘rq’ may be used uninitialized
in this function

Should `rq' just be initialized to NULL? I looked at
floppy.c:set_next_request(), but it's
completely different.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] amiga floppy: Stop sharing request queue across multiple gendisks

[Vivek Goyal]

On Thu, Oct 28, 2010 at 07:38:33PM +0200, Geert Uytterhoeven wrote:
> On Thu, Sep 23, 2010 at 21:54, Vivek Goyal <vgoyal@redhat.com> wrote:
> > o Use one request queue per gendisk instead of sharing request queue
> >
> > o Don't have hardware. No compile testing or run time testing done. Completely
> >  untested.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  drivers/block/amiflop.c |   59 ++++++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 48 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
> > index 76f114f..ead8b77 100644
> > --- a/drivers/block/amiflop.c
> > +++ b/drivers/block/amiflop.c
> > @@ -114,8 +114,6 @@ static unsigned long int fd_def_df0 = FD_DD_3;     /* default for df0 if it does
> >  module_param(fd_def_df0, ulong, 0);
> >  MODULE_LICENSE("GPL");
> >
> > -static struct request_queue *floppy_queue;
> > -
> >  /*
> >  *  Macros
> >  */
> > @@ -164,6 +162,7 @@ static volatile int selected = -1;  /* currently selected drive */
> >  static int writepending;
> >  static int writefromint;
> >  static char *raw_buf;
> > +static int fdc_queue;
> >
> >  static DEFINE_SPINLOCK(amiflop_lock);
> >
> > @@ -1334,6 +1333,42 @@ static int get_track(int drive, int track)
> >        return -1;
> >  }
> >
> > +/*
> > + * Round-robin between our available drives, doing one request from each
> > + */
> > +static struct request *set_next_request(void)
> > +{
> > +       struct request_queue *q;
> > +       int cnt = FD_MAX_UNITS;
> > +       struct request *rq;
> > +
> > +       /* Find next queue we can dispatch from */
> > +       fdc_queue = fdc_queue + 1;
> > +       if (fdc_queue == FD_MAX_UNITS)
> > +               fdc_queue = 0;
> > +
> > +       for(cnt = FD_MAX_UNITS; cnt > 0, cnt--) {
> > +
> > +               if (unit[fdc_queue].type->code == FD_NODRIVE) {
> > +                       if (++fdc_queue == FD_MAX_UNITS)
> > +                               fdc_queue = 0;
> > +                       cotinue;
> > +               }
> > +
> > +               q = unit[fdc_queue].gendisk->queue;
> > +               if (q) {
> > +                       rq = blk_fetch_request(q);
> > +                       if (rq)
> > +                               break;
> > +               }
> > +
> > +               if (++fdc_queue == FD_MAX_UNITS)
> > +                       fdc_queue = 0;
> > +       }
> > +
> > +       return rq;
> > +}
> 
> drivers/block/amiflop.c:1344: warning: ‘rq’ may be used uninitialized
> in this function
> drivers/block/ataflop.c:1402: warning: ‘rq’ may be used uninitialized
> in this function
> 
> Should `rq' just be initialized to NULL? I looked at
> floppy.c:set_next_request(), but it's
> completely different.
> 

I think we should explicitly initialize rq=NULL so that we don't return
a garbage value if we can't find any rq to dispatch.

I will send a patch to fix that. 

Thanks
Vivek

[PATCH 2/2] atari floppy: Stop sharing request queue across multiple gendisks

[Vivek Goyal]

o Use one request queue per gendisk instead of sharing the queue.

o Don't have hardware. No compile testing or run time testing done. Completely
  untested.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/ataflop.c |   50 ++++++++++++++++++++++++++++++++++++----------
 1 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index aceb964..bad514cb 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -79,8 +79,8 @@
 
 #undef DEBUG
 
-static struct request_queue *floppy_queue;
 static struct request *fd_request;
+static int fdc_queue;
 
 /* Disk types: DD, HD, ED */
 static struct atari_disk_type {
@@ -1391,6 +1391,29 @@ static void setup_req_params( int drive )
 			ReqTrack, ReqSector, (unsigned long)ReqData ));
 }
 
+/*
+ * Round-robin between our available drives, doing one request from each
+ */
+static int set_next_request(void)
+{
+	struct request_queue *q;
+	int old_pos = fdc_queue;
+	struct request *rq;
+
+	do {
+		q = unit[fdc_queue].disk->queue;
+		if (++fdc_queue == FD_MAX_UNITS)
+			fdc_queue = 0;
+		if (q) {
+			rq = blk_fetch_request(q);
+			if (rq)
+				break;
+		}
+	} while (fdc_queue != old_pos);
+
+	return rq;
+}
+
 
 static void redo_fd_request(void)
 {
@@ -1405,7 +1428,7 @@ static void redo_fd_request(void)
 
 repeat:
 	if (!fd_request) {
-		fd_request = blk_fetch_request(floppy_queue);
+		fd_request = set_next_request();
 		if (!fd_request)
 			goto the_end;
 	}
@@ -1932,10 +1955,6 @@ static int __init atari_floppy_init (void)
 	PhysTrackBuffer = virt_to_phys(TrackBuffer);
 	BufferDrive = BufferSide = BufferTrack = -1;
 
-	floppy_queue = blk_init_queue(do_fd_request, &ataflop_lock);
-	if (!floppy_queue)
-		goto Enomem;
-
 	for (i = 0; i < FD_MAX_UNITS; i++) {
 		unit[i].track = -1;
 		unit[i].flags = 0;
@@ -1944,7 +1963,10 @@ static int __init atari_floppy_init (void)
 		sprintf(unit[i].disk->disk_name, "fd%d", i);
 		unit[i].disk->fops = &floppy_fops;
 		unit[i].disk->private_data = &unit[i];
-		unit[i].disk->queue = floppy_queue;
+		unit[i].disk->queue = blk_init_queue(do_fd_request,
+					&ataflop_lock);
+		if (!unit[i].disk->queue)
+			goto Enomem;
 		set_capacity(unit[i].disk, MAX_DISK_SIZE * 2);
 		add_disk(unit[i].disk);
 	}
@@ -1959,10 +1981,14 @@ static int __init atari_floppy_init (void)
 
 	return 0;
 Enomem:
-	while (i--)
+	while (i--) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		put_disk(unit[i].disk);
-	if (floppy_queue)
-		blk_cleanup_queue(floppy_queue);
+		if (q)
+			blk_cleanup_queue(q);
+	}
+
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 	return -ENOMEM;
 }
@@ -2011,12 +2037,14 @@ static void __exit atari_floppy_exit(void)
 	int i;
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	for (i = 0; i < FD_MAX_UNITS; i++) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		del_gendisk(unit[i].disk);
 		put_disk(unit[i].disk);
+		blk_cleanup_queue(q);
 	}
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 
-	blk_cleanup_queue(floppy_queue);
 	del_timer_sync(&fd_timer);
 	atari_stram_free( DMABuffer );
 }
-- 
1.7.2.3

Re: [PATCH 2/2] atari floppy: Stop sharing request queue across multiple gendisks

[Vivek Goyal]

On Thu, Sep 23, 2010 at 03:54:06PM -0400, Vivek Goyal wrote:
> o Use one request queue per gendisk instead of sharing the queue.
> 
> o Don't have hardware. No compile testing or run time testing done. Completely
>   untested.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  drivers/block/ataflop.c |   50 ++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index aceb964..bad514cb 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -79,8 +79,8 @@
>  
>  #undef DEBUG
>  
> -static struct request_queue *floppy_queue;
>  static struct request *fd_request;
> +static int fdc_queue;
>  
>  /* Disk types: DD, HD, ED */
>  static struct atari_disk_type {
> @@ -1391,6 +1391,29 @@ static void setup_req_params( int drive )
>  			ReqTrack, ReqSector, (unsigned long)ReqData ));
>  }
>  
> +/*
> + * Round-robin between our available drives, doing one request from each
> + */
> +static int set_next_request(void)

While scanning the patch found one bug. set_next_request() should be 
returning "struct request *". Second version of patch is attached.


o Use one request queue per gendisk instead of sharing the queue.

o Don't have hardware. No compile testing or run time testing done. Completely
  untested.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/ataflop.c |   50 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

Index: linux-2.6-block/drivers/block/ataflop.c
===================================================================
--- linux-2.6-block.orig/drivers/block/ataflop.c	2010-09-21 15:56:35.000000000 -0400
+++ linux-2.6-block/drivers/block/ataflop.c	2010-09-23 16:12:35.826001335 -0400
@@ -79,8 +79,8 @@
 
 #undef DEBUG
 
-static struct request_queue *floppy_queue;
 static struct request *fd_request;
+static int fdc_queue;
 
 /* Disk types: DD, HD, ED */
 static struct atari_disk_type {
@@ -1391,6 +1391,29 @@ static void setup_req_params( int drive 
 			ReqTrack, ReqSector, (unsigned long)ReqData ));
 }
 
+/*
+ * Round-robin between our available drives, doing one request from each
+ */
+static struct request *set_next_request(void)
+{
+	struct request_queue *q;
+	int old_pos = fdc_queue;
+	struct request *rq;
+
+	do {
+		q = unit[fdc_queue].disk->queue;
+		if (++fdc_queue == FD_MAX_UNITS)
+			fdc_queue = 0;
+		if (q) {
+			rq = blk_fetch_request(q);
+			if (rq)
+				break;
+		}
+	} while (fdc_queue != old_pos);
+
+	return rq;
+}
+
 
 static void redo_fd_request(void)
 {
@@ -1405,7 +1428,7 @@ static void redo_fd_request(void)
 
 repeat:
 	if (!fd_request) {
-		fd_request = blk_fetch_request(floppy_queue);
+		fd_request = set_next_request();
 		if (!fd_request)
 			goto the_end;
 	}
@@ -1932,10 +1955,6 @@ static int __init atari_floppy_init (voi
 	PhysTrackBuffer = virt_to_phys(TrackBuffer);
 	BufferDrive = BufferSide = BufferTrack = -1;
 
-	floppy_queue = blk_init_queue(do_fd_request, &ataflop_lock);
-	if (!floppy_queue)
-		goto Enomem;
-
 	for (i = 0; i < FD_MAX_UNITS; i++) {
 		unit[i].track = -1;
 		unit[i].flags = 0;
@@ -1944,7 +1963,10 @@ static int __init atari_floppy_init (voi
 		sprintf(unit[i].disk->disk_name, "fd%d", i);
 		unit[i].disk->fops = &floppy_fops;
 		unit[i].disk->private_data = &unit[i];
-		unit[i].disk->queue = floppy_queue;
+		unit[i].disk->queue = blk_init_queue(do_fd_request,
+					&ataflop_lock);
+		if (!unit[i].disk->queue)
+			goto Enomem;
 		set_capacity(unit[i].disk, MAX_DISK_SIZE * 2);
 		add_disk(unit[i].disk);
 	}
@@ -1959,10 +1981,14 @@ static int __init atari_floppy_init (voi
 
 	return 0;
 Enomem:
-	while (i--)
+	while (i--) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		put_disk(unit[i].disk);
-	if (floppy_queue)
-		blk_cleanup_queue(floppy_queue);
+		if (q)
+			blk_cleanup_queue(q);
+	}
+
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 	return -ENOMEM;
 }
@@ -2011,12 +2037,14 @@ static void __exit atari_floppy_exit(voi
 	int i;
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	for (i = 0; i < FD_MAX_UNITS; i++) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		del_gendisk(unit[i].disk);
 		put_disk(unit[i].disk);
+		blk_cleanup_queue(q);
 	}
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 
-	blk_cleanup_queue(floppy_queue);
 	del_timer_sync(&fd_timer);
 	atari_stram_free( DMABuffer );
 }

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Vivek Goyal]

On Thu, Sep 23, 2010 at 03:54:04PM -0400, Vivek Goyal wrote:
> 
> Hi,
> 
> We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
> 
> There see to be still some drivers sharing request queue across disks. Arch
> specific floppy drivers like amiga and atari are doing so.
> 
> These are two patches which should fix the issue. But these patches are
> completely untested. Not even compilte tested. Don't have hardware to test
> them. 
> 
> Would be great if somebody who has the hardware can lend a hand here to
> see if these patches work.

Sending this mail to linux-m68k@lists.linux-m68k.org list, incase somebody
can help there with testing.

Vivek

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Andreas Bombe]

On Thu, Sep 23, 2010 at 06:35:30PM -0400, Vivek Goyal wrote:
> On Thu, Sep 23, 2010 at 03:54:04PM -0400, Vivek Goyal wrote:
> > Would be great if somebody who has the hardware can lend a hand here to
> > see if these patches work.

Last time I looked at it (well over a year ago), amiflop was seriously
bitrotted from disuse. Just a warning, it may not work properly even
before applying these patches.


Andreas Bombe

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Jens Axboe]

On 2010-09-24 00:35, Vivek Goyal wrote:
> On Thu, Sep 23, 2010 at 03:54:04PM -0400, Vivek Goyal wrote:
>>
>> Hi,
>>
>> We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
>>
>> There see to be still some drivers sharing request queue across disks. Arch
>> specific floppy drivers like amiga and atari are doing so.
>>
>> These are two patches which should fix the issue. But these patches are
>> completely untested. Not even compilte tested. Don't have hardware to test
>> them. 
>>
>> Would be great if somebody who has the hardware can lend a hand here to
>> see if these patches work.
> 
> Sending this mail to linux-m68k@lists.linux-m68k.org list, incase somebody
> can help there with testing.

I think for these drivers, the best we can probably due is ensure that it
cross compiles. As Andreas mentions, getting test coverage is hard and
it's not even given that they work as-is right now.

And since getting this done serves the greater higher purpose of getting
rid of some nasty hacks in the core kernel, we can't let some ancient
and bit rotted (and most likely completely unused) drivers stop this.

-- 
Jens Axboe

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Michael Schmitz]

Hi,

you seem to have dropped the patches from the resent mail - please
resend the patches in question to linux-m68k.

Last time I checked ataflop (around a year ago) it was still working.
I will give your patch a bit of testing once I'm back from traveling
overseas.

Cheers,

  Michael

On Fri, Sep 24, 2010 at 12:35 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Sep 23, 2010 at 03:54:04PM -0400, Vivek Goyal wrote:
>>
>> Hi,
>>
>> We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
>>
>> There see to be still some drivers sharing request queue across disks. Arch
>> specific floppy drivers like amiga and atari are doing so.
>>
>> These are two patches which should fix the issue. But these patches are
>> completely untested. Not even compilte tested. Don't have hardware to test
>> them.
>>
>> Would be great if somebody who has the hardware can lend a hand here to
>> see if these patches work.
>
> Sending this mail to linux-m68k@lists.linux-m68k.org list, incase somebody
> can help there with testing.
>
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Vivek Goyal]

On Sat, Sep 25, 2010 at 11:18:05AM +0200, Michael Schmitz wrote:
> Hi,
> 
> you seem to have dropped the patches from the resent mail - please
> resend the patches in question to linux-m68k.
> 
> Last time I checked ataflop (around a year ago) it was still working.
> I will give your patch a bit of testing once I'm back from traveling
> overseas.
> 
> Cheers,

Thanks a lot Michael. I am attaching the patch with this mail for ataflop.

Alternatively, patches are also available in Jens's block tree here, in
case you prefer to work with git directly.

git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git

As of now, patches are available in "for-next" branch.

Vivek

o Use one request queue per gendisk instead of sharing the queue.

o Don't have hardware. No compile testing or run time testing done. Completely
  untested.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/ataflop.c |   50 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

Index: linux-2.6-block/drivers/block/ataflop.c
===================================================================
--- linux-2.6-block.orig/drivers/block/ataflop.c	2010-09-21 15:56:35.000000000 -0400
+++ linux-2.6-block/drivers/block/ataflop.c	2010-09-23 16:12:35.826001335 -0400
@@ -79,8 +79,8 @@
 
 #undef DEBUG
 
-static struct request_queue *floppy_queue;
 static struct request *fd_request;
+static int fdc_queue;
 
 /* Disk types: DD, HD, ED */
 static struct atari_disk_type {
@@ -1391,6 +1391,29 @@ static void setup_req_params( int drive 
 			ReqTrack, ReqSector, (unsigned long)ReqData ));
 }
 
+/*
+ * Round-robin between our available drives, doing one request from each
+ */
+static struct request *set_next_request(void)
+{
+	struct request_queue *q;
+	int old_pos = fdc_queue;
+	struct request *rq;
+
+	do {
+		q = unit[fdc_queue].disk->queue;
+		if (++fdc_queue == FD_MAX_UNITS)
+			fdc_queue = 0;
+		if (q) {
+			rq = blk_fetch_request(q);
+			if (rq)
+				break;
+		}
+	} while (fdc_queue != old_pos);
+
+	return rq;
+}
+
 
 static void redo_fd_request(void)
 {
@@ -1405,7 +1428,7 @@ static void redo_fd_request(void)
 
 repeat:
 	if (!fd_request) {
-		fd_request = blk_fetch_request(floppy_queue);
+		fd_request = set_next_request();
 		if (!fd_request)
 			goto the_end;
 	}
@@ -1932,10 +1955,6 @@ static int __init atari_floppy_init (voi
 	PhysTrackBuffer = virt_to_phys(TrackBuffer);
 	BufferDrive = BufferSide = BufferTrack = -1;
 
-	floppy_queue = blk_init_queue(do_fd_request, &ataflop_lock);
-	if (!floppy_queue)
-		goto Enomem;
-
 	for (i = 0; i < FD_MAX_UNITS; i++) {
 		unit[i].track = -1;
 		unit[i].flags = 0;
@@ -1944,7 +1963,10 @@ static int __init atari_floppy_init (voi
 		sprintf(unit[i].disk->disk_name, "fd%d", i);
 		unit[i].disk->fops = &floppy_fops;
 		unit[i].disk->private_data = &unit[i];
-		unit[i].disk->queue = floppy_queue;
+		unit[i].disk->queue = blk_init_queue(do_fd_request,
+					&ataflop_lock);
+		if (!unit[i].disk->queue)
+			goto Enomem;
 		set_capacity(unit[i].disk, MAX_DISK_SIZE * 2);
 		add_disk(unit[i].disk);
 	}
@@ -1959,10 +1981,14 @@ static int __init atari_floppy_init (voi
 
 	return 0;
 Enomem:
-	while (i--)
+	while (i--) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		put_disk(unit[i].disk);
-	if (floppy_queue)
-		blk_cleanup_queue(floppy_queue);
+		if (q)
+			blk_cleanup_queue(q);
+	}
+
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 	return -ENOMEM;
 }
@@ -2011,12 +2037,14 @@ static void __exit atari_floppy_exit(voi
 	int i;
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	for (i = 0; i < FD_MAX_UNITS; i++) {
+		struct request_queue *q = unit[i].disk->queue;
+
 		del_gendisk(unit[i].disk);
 		put_disk(unit[i].disk);
+		blk_cleanup_queue(q);
 	}
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 
-	blk_cleanup_queue(floppy_queue);
 	del_timer_sync(&fd_timer);
 	atari_stram_free( DMABuffer );
 }

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Michael Schmitz]

Hi Vivek,

finally tested your patch (after it had become available through git)
and I'm sorry to say the driver no longer works. No idea yet why that
is - this is the error from the block layer:

end_request: I/O error, dev fd0, sector 6400
Buffer I/O error on device fd0, logical block 800
end_request: I/O error, dev fd0, sector 6400
Buffer I/O error on device fd0, logical block 800

Is that the correct sector number for trying to mount a FAT disk?

I'll go back through git history next to see when it first broke.

Cheers,

  MIchael


On Sat, Sep 25, 2010 at 11:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Sat, Sep 25, 2010 at 11:18:05AM +0200, Michael Schmitz wrote:
>> Hi,
>>
>> you seem to have dropped the patches from the resent mail - please
>> resend the patches in question to linux-m68k.
>>
>> Last time I checked ataflop (around a year ago) it was still working.
>> I will give your patch a bit of testing once I'm back from traveling
>> overseas.
>>
>> Cheers,
>
> Thanks a lot Michael. I am attaching the patch with this mail for ataflop.
>
> Alternatively, patches are also available in Jens's block tree here, in
> case you prefer to work with git directly.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git
>
> As of now, patches are available in "for-next" branch.
>
> Vivek
>
> o Use one request queue per gendisk instead of sharing the queue.
>
> o Don't have hardware. No compile testing or run time testing done. Completely
>  untested.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  drivers/block/ataflop.c |   50 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 11 deletions(-)
>
> Index: linux-2.6-block/drivers/block/ataflop.c
> ===================================================================
> --- linux-2.6-block.orig/drivers/block/ataflop.c        2010-09-21 15:56:35.000000000 -0400
> +++ linux-2.6-block/drivers/block/ataflop.c     2010-09-23 16:12:35.826001335 -0400
> @@ -79,8 +79,8 @@
>
>  #undef DEBUG
>
> -static struct request_queue *floppy_queue;
>  static struct request *fd_request;
> +static int fdc_queue;
>
>  /* Disk types: DD, HD, ED */
>  static struct atari_disk_type {
> @@ -1391,6 +1391,29 @@ static void setup_req_params( int drive
>                        ReqTrack, ReqSector, (unsigned long)ReqData ));
>  }
>
> +/*
> + * Round-robin between our available drives, doing one request from each
> + */
> +static struct request *set_next_request(void)
> +{
> +       struct request_queue *q;
> +       int old_pos = fdc_queue;
> +       struct request *rq;
> +
> +       do {
> +               q = unit[fdc_queue].disk->queue;
> +               if (++fdc_queue == FD_MAX_UNITS)
> +                       fdc_queue = 0;
> +               if (q) {
> +                       rq = blk_fetch_request(q);
> +                       if (rq)
> +                               break;
> +               }
> +       } while (fdc_queue != old_pos);
> +
> +       return rq;
> +}
> +
>
>  static void redo_fd_request(void)
>  {
> @@ -1405,7 +1428,7 @@ static void redo_fd_request(void)
>
>  repeat:
>        if (!fd_request) {
> -               fd_request = blk_fetch_request(floppy_queue);
> +               fd_request = set_next_request();
>                if (!fd_request)
>                        goto the_end;
>        }
> @@ -1932,10 +1955,6 @@ static int __init atari_floppy_init (voi
>        PhysTrackBuffer = virt_to_phys(TrackBuffer);
>        BufferDrive = BufferSide = BufferTrack = -1;
>
> -       floppy_queue = blk_init_queue(do_fd_request, &ataflop_lock);
> -       if (!floppy_queue)
> -               goto Enomem;
> -
>        for (i = 0; i < FD_MAX_UNITS; i++) {
>                unit[i].track = -1;
>                unit[i].flags = 0;
> @@ -1944,7 +1963,10 @@ static int __init atari_floppy_init (voi
>                sprintf(unit[i].disk->disk_name, "fd%d", i);
>                unit[i].disk->fops = &floppy_fops;
>                unit[i].disk->private_data = &unit[i];
> -               unit[i].disk->queue = floppy_queue;
> +               unit[i].disk->queue = blk_init_queue(do_fd_request,
> +                                       &ataflop_lock);
> +               if (!unit[i].disk->queue)
> +                       goto Enomem;
>                set_capacity(unit[i].disk, MAX_DISK_SIZE * 2);
>                add_disk(unit[i].disk);
>        }
> @@ -1959,10 +1981,14 @@ static int __init atari_floppy_init (voi
>
>        return 0;
>  Enomem:
> -       while (i--)
> +       while (i--) {
> +               struct request_queue *q = unit[i].disk->queue;
> +
>                put_disk(unit[i].disk);
> -       if (floppy_queue)
> -               blk_cleanup_queue(floppy_queue);
> +               if (q)
> +                       blk_cleanup_queue(q);
> +       }
> +
>        unregister_blkdev(FLOPPY_MAJOR, "fd");
>        return -ENOMEM;
>  }
> @@ -2011,12 +2037,14 @@ static void __exit atari_floppy_exit(voi
>        int i;
>        blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
>        for (i = 0; i < FD_MAX_UNITS; i++) {
> +               struct request_queue *q = unit[i].disk->queue;
> +
>                del_gendisk(unit[i].disk);
>                put_disk(unit[i].disk);
> +               blk_cleanup_queue(q);
>        }
>        unregister_blkdev(FLOPPY_MAJOR, "fd");
>
> -       blk_cleanup_queue(floppy_queue);
>        del_timer_sync(&fd_timer);
>        atari_stram_free( DMABuffer );
>  }
>
>

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Michael Schmitz]

Hi,

On Mon, Nov 22, 2010 at 9:18 AM, Michael Schmitz
<schmitzmic@googlemail.com> wrote:
>
> end_request: I/O error, dev fd0, sector 6400
> Buffer I/O error on device fd0, logical block 800
> end_request: I/O error, dev fd0, sector 6400
> Buffer I/O error on device fd0, logical block 800
>
> Is that the correct sector number for trying to mount a FAT disk?

Answering myself - of course not; the number of clusters should not
exceed 2800.

Further testing demonstrates that the floppy driver works as long as I
specify the disk format to use with the mount command (i.e. mount
/dev/fd0h1440 /floppy). Bizarre.

I'll try to reproduce that and maybe track the problem down.

Cheers,

  Michael

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Jens Axboe]

On 2010-09-23 21:54, Vivek Goyal wrote:
> Hi,
> 
> We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
> 
> There see to be still some drivers sharing request queue across disks. Arch
> specific floppy drivers like amiga and atari are doing so.
> 
> These are two patches which should fix the issue. But these patches are
> completely untested. Not even compilte tested. Don't have hardware to test
> them. 
> 
> Would be great if somebody who has the hardware can lend a hand here to
> see if these patches work.

I have (tentatively) added both patches to for-2.6.37/drivers. I used the
latter version of 2/2.

-- 
Jens Axboe

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Vivek Goyal]

On Fri, Sep 24, 2010 at 08:37:10PM +0200, Jens Axboe wrote:
> On 2010-09-23 21:54, Vivek Goyal wrote:
> > Hi,
> > 
> > We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
> > 
> > There see to be still some drivers sharing request queue across disks. Arch
> > specific floppy drivers like amiga and atari are doing so.
> > 
> > These are two patches which should fix the issue. But these patches are
> > completely untested. Not even compilte tested. Don't have hardware to test
> > them. 
> > 
> > Would be great if somebody who has the hardware can lend a hand here to
> > see if these patches work.
> 
> I have (tentatively) added both patches to for-2.6.37/drivers. I used the
> latter version of 2/2.

Thanks Jens.

Finally I could setup and do cross compile for m68k. amiga floppy driver
fails to compile. Please find attached the fix.

Thanks
Vivek

o Compile fixes for amiga floppy driver.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 drivers/block/amiflop.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: linux-2.6-block/drivers/block/amiflop.c
===================================================================
--- linux-2.6-block.orig/drivers/block/amiflop.c	2010-09-24 16:12:19.000000000 -0400
+++ linux-2.6-block/drivers/block/amiflop.c	2010-09-24 16:13:38.000000000 -0400
@@ -1347,12 +1347,12 @@ static struct request *set_next_request(
 	if (fdc_queue == FD_MAX_UNITS)
 		fdc_queue = 0;
 
-	for(cnt = FD_MAX_UNITS; cnt > 0, cnt--) {
+	for(cnt = FD_MAX_UNITS; cnt > 0; cnt--) {
 
 		if (unit[fdc_queue].type->code == FD_NODRIVE) {
 			if (++fdc_queue == FD_MAX_UNITS)
 				fdc_queue = 0;
-			cotinue;
+			continue;
 		}
 
 		q = unit[fdc_queue].gendisk->queue;
@@ -1827,7 +1827,6 @@ static int __init amiga_floppy_probe(str
 	return 0;
 
 out_probe:
-out_queue:
 	free_irq(IRQ_AMIGA_CIAA_TB, NULL);
 out_irq2:
 	free_irq(IRQ_AMIGA_DSKBLK, NULL);

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Vivek Goyal]

On Fri, Sep 24, 2010 at 08:37:10PM +0200, Jens Axboe wrote:
> On 2010-09-23 21:54, Vivek Goyal wrote:
> > Hi,
> > 
> > We seem to have deprecated the notion of sharing request queue across gendisks. Now we need to instanciate one request queue per disk.
> > 
> > There see to be still some drivers sharing request queue across disks. Arch
> > specific floppy drivers like amiga and atari are doing so.
> > 
> > These are two patches which should fix the issue. But these patches are
> > completely untested. Not even compilte tested. Don't have hardware to test
> > them. 
> > 
> > Would be great if somebody who has the hardware can lend a hand here to
> > see if these patches work.
> 
> I have (tentatively) added both patches to for-2.6.37/drivers. I used the
> latter version of 2/2.

Also which cross compiling for m68k, I noticed following commit fails
compilation. I had to reset git to older commit to test amiga and atari
flopply driver compilation.

*******************************************************************
block/blk-exec.c: In function 'blk_execute_rq':
block/blk-exec.c:101: error: 'sysctl_hung_task_timeout_secs' undeclared
(first use in this function)
block/blk-exec.c:101: error: (Each undeclared identifier is reported only
once
block/blk-exec.c:101: error: for each function it appears in.)
make[1]: *** [block/blk-exec.o] Error 1
make: *** [block] Error 2
***********************************************************************

commit 4b1977698ceb4c4caa800d475127139da49966f9
Author: Mark Lord <kernel@teksavvy.com>
Date:   Fri Sep 24 09:51:13 2010 -0400

    block: Prevent hang_check firing during long I/O
    
    During long I/O operations, the hang_check timer may fire,
    trigger stack dumps that unnecessarily alarm the user.
    
    Eg.  hdparm --security-erase NULL /dev/sdb  ## can take *hours* to
complete
    
    So, if hang_check is armed, we should wake up periodically
    to prevent it from triggering.  This patch uses a wake-up interval
    equal to half the hang_check timer period, which keeps overhead low
enough.
    
    Signed-off-by: Mark Lord <mlord@pobox.com>
    Signed-off-by: Jens Axboe <jaxboe@fusionio.com>

Thanks
Vivek

Re: [RFT PATCH] amiga, atari floppy: Use one request queue per disk

[Mark Lord]

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

Ensure that 'sysctl_hung_task_timeout_secs' is defined
even when CONFIG_DETECT_HUNG_TASK is not set.
This way we can safely reference it without need for
#ifdefs elsewhere in the code.  eg. in block/blk-exec.c

Signed-off-by: Mark Lord <mlord@pobox.com>

Jens: use the attachment.  My email client is having a fit today.

--- a/include/linux/sched.h	2010-09-20 19:56:53.000000000 -0400
+++ b/include/linux/sched.h	2010-09-24 17:22:01.707291995 -0400
@@ -336,6 +336,9 @@
  extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
  					 void __user *buffer,
  					 size_t *lenp, loff_t *ppos);
+#else
+/* Avoid need for ifdefs elsewhere in the code */
+enum { sysctl_hung_task_timeout_secs = 0 };
  #endif
  
  /* Attach to any functions which should be ignored in wchan output. */

[-- Attachment #2: fix_blk_exec_compile_error.patch --]
[-- Type: text/x-patch, Size: 475 bytes --]

--- a/include/linux/sched.h	2010-09-20 19:56:53.000000000 -0400
+++ b/include/linux/sched.h	2010-09-24 17:22:01.707291995 -0400
@@ -336,6 +336,9 @@
 extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
 					 void __user *buffer,
 					 size_t *lenp, loff_t *ppos);
+#else
+/* Avoid need for ifdefs elsewhere in the code */
+enum { sysctl_hung_task_timeout_secs = 0 };
 #endif
 
 /* Attach to any functions which should be ignored in wchan output. */

[PATCH] fix blk-exec.c compile error: always define 'sysctl_hung_task_timeout_secs' (resend)

[Mark Lord]

Ensure that 'sysctl_hung_task_timeout_secs' is defined
even when CONFIG_DETECT_HUNG_TASK is not set.
This way we can safely reference it without need for
ifdefs in the code elsewhere.  eg. in block/blk-exec.c

Signed-off-by: Mark Lord <mlord@pobox.com>

(Resending from a repaired email client.)
Jens: my apologies for not catching this this earlier.

--- a/include/linux/sched.h	2010-09-20 19:56:53.000000000 -0400
+++ b/include/linux/sched.h	2010-09-24 17:22:01.707291995 -0400
@@ -336,6 +336,9 @@
 extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
 					 void __user *buffer,
 					 size_t *lenp, loff_t *ppos);
+#else
+/* Avoid need for ifdefs elsewhere in the code */
+enum { sysctl_hung_task_timeout_secs = 0 };
 #endif
 
 /* Attach to any functions which should be ignored in wchan output. */

Re: [PATCH] fix blk-exec.c compile error: always define 'sysctl_hung_task_timeout_secs' (resend)

[Jens Axboe]

On 2010-09-24 23:46, Mark Lord wrote:
> Ensure that 'sysctl_hung_task_timeout_secs' is defined
> even when CONFIG_DETECT_HUNG_TASK is not set.
> This way we can safely reference it without need for
> ifdefs in the code elsewhere.  eg. in block/blk-exec.c

Thanks Mark, applied.

-- 
Jens Axboe