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

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: [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