please DON'T run 2.5.27 with IDE!

please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

IDE 99 which is included in 2.5.27 introduced really nasty bug.
Possible lockups and data corruption. Please do not.

Regards
--
Bartlomiej

Re: please DON'T run 2.5.27 with IDE!

[Petr Vandrovec]

On 22 Jul 02 at 21:37, Bartlomiej Zolnierkiewicz wrote:

> IDE 99 which is included in 2.5.27 introduced really nasty bug.
> Possible lockups and data corruption. Please do not.

Too late... what should I expect?
                                                Petr Vandrovec
                                                
                                                

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Mon, 22 Jul 2002, Petr Vandrovec wrote:

> On 22 Jul 02 at 21:37, Bartlomiej Zolnierkiewicz wrote:
>
> > IDE 99 which is included in 2.5.27 introduced really nasty bug.
> > Possible lockups and data corruption. Please do not.
>
> Too late... what should I expect?

Ask Martin ;-).
BTW> I have a luck: compiled kernel + modules, lilo, was thinking about
     reboot and I remebered myself to go through IDE 99.

Regards
--
Bartlomiej

>                                                Petr Vandrovec
>

Re: please DON'T run 2.5.27 with IDE!

[Andries Brouwer]

On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:

> IDE 99 which is included in 2.5.27 introduced really nasty bug.
> Possible lockups and data corruption. Please do not.

On the other hand, thanks to Jens, I have been running 2.5.27 with 2.4 IDE
now for two days without any IDE-related trouble.

Andries


[usb still has some problems - hotplug is difficult;
another funny is that for me netscape 4.79 with flash works
under 2.4.17 and doesn't work under 2.5.25 or 2.5.27; have
not yet tried to trace it down; does this sound like
something well-known?]

Re: please DON'T run 2.5.27 with IDE!

[Thunder from the hill]

Hi,

On Mon, 22 Jul 2002, Andries Brouwer wrote:
> another funny is that for me netscape 4.79 with flash works
> under 2.4.17 and doesn't work under 2.5.25 or 2.5.27; have
> not yet tried to trace it down; does this sound like
> something well-known?

I use to get SIGBUS w/Netscape 4 all over the place. The well-working 
versions are 3 Gold (almost no crashes, but ugly), 4.79 works sometimes, 
and 6 works just fine, except the usual crap since they inherited an older 
version of Mozilla...

I'm not sure if the Netscape 4 crashes are supposed to be kernel bugs...

							Regards,
							Thunder
-- 
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o?  K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y- 
------END GEEK CODE BLOCK------

Re: please DON'T run 2.5.27 with IDE!

[A Guy Called Tyketto]

On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> IDE 99 which is included in 2.5.27 introduced really nasty bug.
> Possible lockups and data corruption. Please do not.
> 
> Regards
> --
> Bartlomiej

        Okay.. I have to ask the question.. Which version of IDE does 2.5.27 
have? you're saying IDE 99. According to the changelog Linus threw out with 
2.5.27:

Martin Dalecki <dalecki@evision-ventures.com>:
  o 2.5.26 IDE 99
  o IDE 100

        With this, I'm assuming 2.5.27 has IDE 100 in it, patched up from IDE 
99. Correct me if I'm wrong.

                                                        BL.
-- 
Brad Littlejohn                         | Email:        tyketto@wizard.com
Unix Systems Administrator,             |           tyketto@ozemail.com.au
Web + NewsMaster, BOFH.. Smeghead! :)   |   http://www.wizard.com/~tyketto
  PGP: 1024D/E319F0BF 6980 AAD6 7329 E9E6 D569  F620 C819 199A E319 F0BF

Re: please DON'T run 2.5.27 with IDE!

[Thunder from the hill]

Hi,

On Mon, 22 Jul 2002, A Guy Called Tyketto wrote:
> With this, I'm assuming 2.5.27 has IDE 100 in it, patched up from IDE
> 99. Correct me if I'm wrong.

Introducing a bug in 99 doesn't mean it has to be fixed in 100. However, 
it might be. Martin, could you please tell us more about it?

BTW, where did we end up? We don't even know the current IDE state...

							Regards,
							Thunder
-- 
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o?  K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y- 
------END GEEK CODE BLOCK------

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Mon, 22 Jul 2002, Thunder from the hill wrote:

> Hi,
>
> On Mon, 22 Jul 2002, A Guy Called Tyketto wrote:
> > With this, I'm assuming 2.5.27 has IDE 100 in it, patched up from IDE
> > 99. Correct me if I'm wrong.

Hi,

You are wrong. ;-)
IDE patches are incremetnal, IDE 100 is incremental to IDE 99.
Both are in 2.5.27.

 > Introducing a bug in 99 doesn't mean it has to be fixed in 100. However,
> it might be. Martin, could you please tell us more about it?

It IS NOT. IDE 100 is a trivia patch indendation + initializers etc.

>
> BTW, where did we end up? We don't even know the current IDE state...
>
> 							Regards,
> 							Thunder
> --
> (Use http://www.ebb.org/ungeek if you can't decode)
> ------BEGIN GEEK CODE BLOCK------
> Version: 3.12
> GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
> N--- o?  K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
> e++++ h* r--- y-
> ------END GEEK CODE BLOCK------

Re: please DON'T run 2.5.27 with IDE!

[Morten Helgesen]

On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> IDE 99 which is included in 2.5.27 introduced really nasty bug.
> Possible lockups and data corruption. Please do not.

Could you please elaborate a bit ? 

> 
> Regards
> --
> Bartlomiej

-- 

"Livet er ikke for nybegynnere" - sitat fra en klok person.

mvh
Morten Helgesen 
UNIX System Administrator & C Developer 
Nextframe AS
admin@nextframe.net / 93445641
http://www.nextframe.net

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Tue, 23 Jul 2002, Morten Helgesen wrote:

> On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > IDE 99 which is included in 2.5.27 introduced really nasty bug.
> > Possible lockups and data corruption. Please do not.
>
> Could you please elaborate a bit ?

Bug is a result of Martin being careless and not sending patches for
public review. It is easy to fix, but I won't, please excuse me.
Also I wont go in technical details, lets see how quick it will be fixed.

Regards
--
Bartlomiej

> >
> > Regards
> > --
> > Bartlomiej
>
> --
>
> "Livet er ikke for nybegynnere" - sitat fra en klok person.
>
> mvh
> Morten Helgesen
> UNIX System Administrator & C Developer
> Nextframe AS
> admin@nextframe.net / 93445641
> http://www.nextframe.net
>

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:
> On Tue, 23 Jul 2002, Morten Helgesen wrote:
> 
> 
>>On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>
>>>IDE 99 which is included in 2.5.27 introduced really nasty bug.
>>>Possible lockups and data corruption. Please do not.
>>
>>Could you please elaborate a bit ?
> 
> 
> Bug is a result of Martin being careless and not sending patches for
> public review. It is easy to fix, but I won't, please excuse me.
> Also I wont go in technical details, lets see how quick it will be fixed.

The problem is of a somehow general nature.
Many of the block devices *need* a mechanism to run commands
asynchronously. The most preffered way to do this is
of course to go by the already present request queue.
However the generic queue handling layer
doesn't give us any mechanism to actually stuff
request from the driver and it doesn't behave well in boundary
conditions where the queues are nearly full.

So every single subsystem is (or at least should be) repeating something
along the lines of the following...

tatic void __scsi_insert_special(request_queue_t *q, struct request *rq,
				  void *data, int at_head)
{
	unsigned long flags;

	ASSERT_LOCK(q->queue_lock, 0);

	/*
	 * tell I/O scheduler that this isn't a regular read/write (ie
	 * must not attempt merges on this) and that it acts as a soft
	 * barrier
	 */
	rq->flags &= REQ_QUEUED;
	rq->flags |= REQ_SPECIAL | REQ_BARRIER;

	rq->special = data;

	spin_lock_irqsave(q->queue_lock, flags);
	/* If command is tagged, release the tag */
	if(blk_rq_tagged(rq))
		blk_queue_end_tag(q, rq);
	_elv_add_request(q, rq, !at_head, 0);
	q->request_fn(q);
	spin_unlock_irqrestore(q->queue_lock, flags);
}


int scsi_insert_special_req(Scsi_Request * SRpnt, int at_head)
{
	request_queue_t *q = &SRpnt->sr_device->request_queue;

	__scsi_insert_special(q, SRpnt->sr_request, SRpnt, at_head);
	return 0;
}

Well actually the proper patch will be modelled after what
is done in SCSI. Or maybe even unifying both.
At least it is immediately "obvious" that __scsi_insert_request()
has a signature which doesn't have anything to do with the SCSI
subsystem.
Becouse it is clear from the above as well
that for example at least setting the rq->flags should
be common among every kind of subsystem and it shouldn't
be done inside the subsystems implementation of this
method, since the flags are of a generic nature and there
are changes in this area from time to time.


For now the following *should* do for IDE:

===== drivers/ide/ide-taskfile.c 1.61 vs edited =====
--- 1.61/drivers/ide/ide-taskfile.c	Fri Jul 19 10:18:50 2002
+++ edited/drivers/ide/ide-taskfile.c	Tue Jul 23 12:12:55 2002
@@ -194,22 +194,16 @@
  	request_queue_t *q = &drive->queue;
  	struct list_head *queue_head = &q->queue_head;
  	DECLARE_COMPLETION(wait);
+	struct request req;

  #ifdef CONFIG_BLK_DEV_PDC4030
  	if (ch->chipset == ide_pdc4030 && buf)
  		return -ENOSYS;  /* special drive cmds not supported */
  #endif

-	rq = __blk_get_request(&drive->queue, READ);
-	if (!rq)
-		rq = __blk_get_request(&drive->queue, WRITE);
-
-	/*
-	 * FIXME: Make sure there is a free slot on the list!
-	 */
-
-	BUG_ON(!rq);
-
+	memset(&req, 0, sizeof(req));
+	rq = &req;
+	
  	rq->flags = REQ_SPECIAL;
  	rq->buffer = buf;
  	rq->special = ar;

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Tue, 23 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
> > On Tue, 23 Jul 2002, Morten Helgesen wrote:
> >
> >
> >>On Mon, Jul 22, 2002 at 09:37:13PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >>
> >>>IDE 99 which is included in 2.5.27 introduced really nasty bug.
> >>>Possible lockups and data corruption. Please do not.
> >>
> >>Could you please elaborate a bit ?
> >
> >
> > Bug is a result of Martin being careless and not sending patches for
> > public review. It is easy to fix, but I won't, please excuse me.
> > Also I wont go in technical details, lets see how quick it will be fixed.
>
> The problem is of a somehow general nature.
> Many of the block devices *need* a mechanism to run commands
> asynchronously. The most preffered way to do this is

Problem is of getting completion status of this commands
asynchronously, they are all run synchronously through request queue.

> of course to go by the already present request queue.
> However the generic queue handling layer
> doesn't give us any mechanism to actually stuff
> request from the driver and it doesn't behave well in boundary
> conditions where the queues are nearly full.
>
> So every single subsystem is (or at least should be) repeating something
> along the lines of the following...
>
> tatic void __scsi_insert_special(request_queue_t *q, struct request *rq,
> 				  void *data, int at_head)
> {
> 	unsigned long flags;
>
> 	ASSERT_LOCK(q->queue_lock, 0);
>
> 	/*
> 	 * tell I/O scheduler that this isn't a regular read/write (ie
> 	 * must not attempt merges on this) and that it acts as a soft
> 	 * barrier
> 	 */
> 	rq->flags &= REQ_QUEUED;
> 	rq->flags |= REQ_SPECIAL | REQ_BARRIER;
>
> 	rq->special = data;
>
> 	spin_lock_irqsave(q->queue_lock, flags);
> 	/* If command is tagged, release the tag */
> 	if(blk_rq_tagged(rq))
> 		blk_queue_end_tag(q, rq);
> 	_elv_add_request(q, rq, !at_head, 0);
> 	q->request_fn(q);
> 	spin_unlock_irqrestore(q->queue_lock, flags);
> }
>
>
> int scsi_insert_special_req(Scsi_Request * SRpnt, int at_head)
> {
> 	request_queue_t *q = &SRpnt->sr_device->request_queue;
>
> 	__scsi_insert_special(q, SRpnt->sr_request, SRpnt, at_head);
> 	return 0;
> }
>
> Well actually the proper patch will be modelled after what
> is done in SCSI. Or maybe even unifying both.
> At least it is immediately "obvious" that __scsi_insert_request()
> has a signature which doesn't have anything to do with the SCSI
> subsystem.
> Becouse it is clear from the above as well
> that for example at least setting the rq->flags should
> be common among every kind of subsystem and it shouldn't
> be done inside the subsystems implementation of this
> method, since the flags are of a generic nature and there
> are changes in this area from time to time.
>
>
> For now the following *should* do for IDE:

Martin why aren't you telling people all facts?
It was the default behaviour before your change in IDE 99.
This patch in practice reverts IDE 99 change.

You have INTRODUCED a bug and now you try to
pretend that it wasn't your fault and it was somehow broken before.
Before 2.5.27 code had the same functionality as scsi version.
And yes it will be useful to move it to block layer.

Regards
--
Bartlomiej

> ===== drivers/ide/ide-taskfile.c 1.61 vs edited =====
> --- 1.61/drivers/ide/ide-taskfile.c	Fri Jul 19 10:18:50 2002
> +++ edited/drivers/ide/ide-taskfile.c	Tue Jul 23 12:12:55 2002
> @@ -194,22 +194,16 @@
>   	request_queue_t *q = &drive->queue;
>   	struct list_head *queue_head = &q->queue_head;
>   	DECLARE_COMPLETION(wait);
> +	struct request req;
>
>   #ifdef CONFIG_BLK_DEV_PDC4030
>   	if (ch->chipset == ide_pdc4030 && buf)
>   		return -ENOSYS;  /* special drive cmds not supported */
>   #endif
>
> -	rq = __blk_get_request(&drive->queue, READ);
> -	if (!rq)
> -		rq = __blk_get_request(&drive->queue, WRITE);
> -
> -	/*
> -	 * FIXME: Make sure there is a free slot on the list!
> -	 */
> -
> -	BUG_ON(!rq);
> -
> +	memset(&req, 0, sizeof(req));
> +	rq = &req;
> +
>   	rq->flags = REQ_SPECIAL;
>   	rq->buffer = buf;
>   	rq->special = ar;
>

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:

> Martin why aren't you telling people all facts?
> It was the default behaviour before your change in IDE 99.
> This patch in practice reverts IDE 99 change.
> 
> You have INTRODUCED a bug and now you try to
> pretend that it wasn't your fault and it was somehow broken before.

Never said that. Sure it was my fault I looked in the wrong direction
I looked at the ide-tcq code, becouse I still dont like the
idea that we pass a pointer for a struct on the local stack down.
(It's preventing the futile hope to make this thingee somehow 
asynchronous form ever taking place.)

I should have looked at SCSI in first place instead indeed.

> Before 2.5.27 code had the same functionality as scsi version.

That's actually not true... At least the setting of the
request rq->flags is significantly different here and there.
However I think but I'm not sure that the fact aht we have rq->special 
!= NULL here has the hidded side effect that we indeed accomplish the
same semantics.

> And yes it will be useful to move it to block layer.

Done. Just needs testing. I have at least an ZIP parport drive, which
allows me to do some basic checks.


BTW.> Having a fill up request queue trashing data transfers
is indicating that there may be are bugs in the generic block layer too. 
If it gets pushed to boundary conditions it's apparently not very 
robust... BTW.> I never ever will understand why
request_fn returns void instead of an status value.
It could save many places where evry single driver
has to call end_request explicitely.



> Regards
> --
> Bartlomiej
> 
> 
>>===== drivers/ide/ide-taskfile.c 1.61 vs edited =====
>>--- 1.61/drivers/ide/ide-taskfile.c	Fri Jul 19 10:18:50 2002
>>+++ edited/drivers/ide/ide-taskfile.c	Tue Jul 23 12:12:55 2002
>>@@ -194,22 +194,16 @@
>>  	request_queue_t *q = &drive->queue;
>>  	struct list_head *queue_head = &q->queue_head;
>>  	DECLARE_COMPLETION(wait);
>>+	struct request req;
>>
>>  #ifdef CONFIG_BLK_DEV_PDC4030
>>  	if (ch->chipset == ide_pdc4030 && buf)
>>  		return -ENOSYS;  /* special drive cmds not supported */
>>  #endif
>>
>>-	rq = __blk_get_request(&drive->queue, READ);
>>-	if (!rq)
>>-		rq = __blk_get_request(&drive->queue, WRITE);
>>-
>>-	/*
>>-	 * FIXME: Make sure there is a free slot on the list!
>>-	 */
>>-
>>-	BUG_ON(!rq);
>>-
>>+	memset(&req, 0, sizeof(req));
>>+	rq = &req;
>>+
>>  	rq->flags = REQ_SPECIAL;
>>  	rq->buffer = buf;
>>  	rq->special = ar;
>>
> 
> 
> 

Re: please DON'T run 2.5.27 with IDE!

[Jan Harkes]

On Tue, Jul 23, 2002 at 03:58:58PM +0200, Marcin Dalecki wrote:
> That's actually not true... At least the setting of the
> request rq->flags is significantly different here and there.
> However I think but I'm not sure that the fact aht we have rq->special 
> != NULL here has the hidded side effect that we indeed accomplish the
> same semantics.
> 
> >And yes it will be useful to move it to block layer.
> 
> Done. Just needs testing. I have at least an ZIP parport drive, which
> allows me to do some basic checks.

Ehh, you are testing all those IDE changes with a ZIP drive connected to
the parallel port? Don't you have any real IDE devices?? I'm sure we can
all chip in and buy you a drive if you need one.

Jan

Re: please DON'T run 2.5.27 with IDE!

[Andre Hedrick]

On Tue, 23 Jul 2002, Jan Harkes wrote:

> On Tue, Jul 23, 2002 at 03:58:58PM +0200, Marcin Dalecki wrote:
> > That's actually not true... At least the setting of the
> > request rq->flags is significantly different here and there.
> > However I think but I'm not sure that the fact aht we have rq->special 
> > != NULL here has the hidded side effect that we indeed accomplish the
> > same semantics.
> > 
> > >And yes it will be useful to move it to block layer.
> > 
> > Done. Just needs testing. I have at least an ZIP parport drive, which
> > allows me to do some basic checks.
> 
> Ehh, you are testing all those IDE changes with a ZIP drive connected to
> the parallel port? Don't you have any real IDE devices?? I'm sure we can
> all chip in and buy you a drive if you need one.

Would be faster to get a real maintainer, but nobody cares about actually
finishing 2.5 or they would find one that could actually make it work
proper.

Andre Hedrick
LAD Storage Consulting Group

-------------------------------------------------------
2.4, has crappy code, an arse-hole maintainer, and a working safe driver.
2.5, erm "Two of Three, ain't BAD"

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

 Tue, 23 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
>
> > Martin why aren't you telling people all facts?
> > It was the default behaviour before your change in IDE 99.
> > This patch in practice reverts IDE 99 change.
> >
> > You have INTRODUCED a bug and now you try to
> > pretend that it wasn't your fault and it was somehow broken before.
>
> Never said that. Sure it was my fault I looked in the wrong direction
> I looked at the ide-tcq code, becouse I still dont like the
> idea that we pass a pointer for a struct on the local stack down.
> (It's preventing the futile hope to make this thingee somehow
> asynchronous form ever taking place.)
>
> I should have looked at SCSI in first place instead indeed.
>
> > Before 2.5.27 code had the same functionality as scsi version.
>
> That's actually not true... At least the setting of the
> request rq->flags is significantly different here and there.

You are right here...
Actually IDE request should have REQ_BARRIER bit also set for safety
and coherency.
Without barrier requests added after special command can be merged
with requests added before special command.

> However I think but I'm not sure that the fact aht we have rq->special
> != NULL here has the hidded side effect that we indeed accomplish the
> same semantics.

No.

> > And yes it will be useful to move it to block layer.
>
> Done. Just needs testing. I have at least an ZIP parport drive, which
> allows me to do some basic checks.

Test everything on your production machine + main hdd.
Will make you care more about code correctness ;-).

>
> BTW.> Having a fill up request queue trashing data transfers
> is indicating that there may be are bugs in the generic block layer too.
> If it gets pushed to boundary conditions it's apparently not very

No it wasn't "pushed to boundary conditions", you screwed it, sorry.

> robust... BTW.> I never ever will understand why
> request_fn returns void instead of an status value.

So look at ide.c for example.

Regards
--
Bartlomiej

> It could save many places where evry single driver
> has to call end_request explicitely.
>
>
>
> > Regards
> > --
> > Bartlomiej
> >
> >
> >>===== drivers/ide/ide-taskfile.c 1.61 vs edited =====
> >>--- 1.61/drivers/ide/ide-taskfile.c	Fri Jul 19 10:18:50 2002
> >>+++ edited/drivers/ide/ide-taskfile.c	Tue Jul 23 12:12:55 2002
> >>@@ -194,22 +194,16 @@
> >>  	request_queue_t *q = &drive->queue;
> >>  	struct list_head *queue_head = &q->queue_head;
> >>  	DECLARE_COMPLETION(wait);
> >>+	struct request req;
> >>
> >>  #ifdef CONFIG_BLK_DEV_PDC4030
> >>  	if (ch->chipset == ide_pdc4030 && buf)
> >>  		return -ENOSYS;  /* special drive cmds not supported */
> >>  #endif
> >>
> >>-	rq = __blk_get_request(&drive->queue, READ);
> >>-	if (!rq)
> >>-		rq = __blk_get_request(&drive->queue, WRITE);
> >>-
> >>-	/*
> >>-	 * FIXME: Make sure there is a free slot on the list!
> >>-	 */
> >>-
> >>-	BUG_ON(!rq);
> >>-
> >>+	memset(&req, 0, sizeof(req));
> >>+	rq = &req;
> >>+
> >>  	rq->flags = REQ_SPECIAL;
> >>  	rq->buffer = buf;
> >>  	rq->special = ar;
> >>
> >
> >
> >
>
>
>

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Jan Harkes wrote:
> On Tue, Jul 23, 2002 at 03:58:58PM +0200, Marcin Dalecki wrote:
> 
>>That's actually not true... At least the setting of the
>>request rq->flags is significantly different here and there.
>>However I think but I'm not sure that the fact aht we have rq->special 
>>!= NULL here has the hidded side effect that we indeed accomplish the
>>same semantics.
>>
>>
>>>And yes it will be useful to move it to block layer.
>>
>>Done. Just needs testing. I have at least an ZIP parport drive, which
>>allows me to do some basic checks.
> 
> 
> Ehh, you are testing all those IDE changes with a ZIP drive connected to
> the parallel port? Don't you have any real IDE devices?? I'm sure we can
> all chip in and buy you a drive if you need one.

For Gods sake not! The ZIP drive is a SCSI device. And the change to the
SCSI subsystems involves only trivial code movearound. For this
purpose it should be sufficient to trigger it with the above device.

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:
>  Tue, 23 Jul 2002, Marcin Dalecki wrote:
> 
> 
>>Bartlomiej Zolnierkiewicz wrote:
>>
>>
>>>Martin why aren't you telling people all facts?
>>>It was the default behaviour before your change in IDE 99.
>>>This patch in practice reverts IDE 99 change.
>>>
>>>You have INTRODUCED a bug and now you try to
>>>pretend that it wasn't your fault and it was somehow broken before.
>>
>>Never said that. Sure it was my fault I looked in the wrong direction
>>I looked at the ide-tcq code, becouse I still dont like the
>>idea that we pass a pointer for a struct on the local stack down.
>>(It's preventing the futile hope to make this thingee somehow
>>asynchronous form ever taking place.)
>>
>>I should have looked at SCSI in first place instead indeed.
>>
>>
>>>Before 2.5.27 code had the same functionality as scsi version.
>>
>>That's actually not true... At least the setting of the
>>request rq->flags is significantly different here and there.
> 
> 
> You are right here...
> Actually IDE request should have REQ_BARRIER bit also set for safety
> and coherency.
> Without barrier requests added after special command can be merged
> with requests added before special command.
> 
> 
>>However I think but I'm not sure that the fact aht we have rq->special
>>!= NULL here has the hidded side effect that we indeed accomplish the
>>same semantics.
> 
> 
> No.

There are some nasty checks for it != NULL in the generic BIO code.
And REQ_BARRIER got introduced just recently, so we can see the
error will have been propagated.

>>>And yes it will be useful to move it to block layer.
>>
>>Done. Just needs testing. I have at least an ZIP parport drive, which
>>allows me to do some basic checks.
> 
> 
> Test everything on your production machine + main hdd.
> Will make you care more about code correctness ;-).

Nah... that's just for the SCSI code "move around". The rest
I usually run continuously on two systems.

>>BTW.> Having a fill up request queue trashing data transfers
>>is indicating that there may be are bugs in the generic block layer too.
>>If it gets pushed to boundary conditions it's apparently not very
> 
> 
> No it wasn't "pushed to boundary conditions", you screwed it, sorry.
> 
> 
>>robust... BTW.> I never ever will understand why
>>request_fn returns void instead of an status value.
> 
> 
> So look at ide.c for example.

So look at drivers which call blk_start_queue() from within 
q->request_fn context, which is, well, causing deliberate *recursion*.

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
> >  Tue, 23 Jul 2002, Marcin Dalecki wrote:
> >
> >>Bartlomiej Zolnierkiewicz wrote:
> >>
> >>>Martin why aren't you telling people all facts?
> >>>It was the default behaviour before your change in IDE 99.
> >>>This patch in practice reverts IDE 99 change.
> >>>
> >>>You have INTRODUCED a bug and now you try to
> >>>pretend that it wasn't your fault and it was somehow broken before.
> >>
> >>Never said that. Sure it was my fault I looked in the wrong direction
> >>I looked at the ide-tcq code, becouse I still dont like the
> >>idea that we pass a pointer for a struct on the local stack down.
> >>(It's preventing the futile hope to make this thingee somehow
> >>asynchronous form ever taking place.)
> >>
> >>I should have looked at SCSI in first place instead indeed.
> >>
> >>
> >>>Before 2.5.27 code had the same functionality as scsi version.
> >>
> >>That's actually not true... At least the setting of the
> >>request rq->flags is significantly different here and there.
> >
> >
> > You are right here...
> > Actually IDE request should have REQ_BARRIER bit also set for safety
> > and coherency.
> > Without barrier requests added after special command can be merged
> > with requests added before special command.
> >
> >
> >>However I think but I'm not sure that the fact aht we have rq->special
> >>!= NULL here has the hidded side effect that we indeed accomplish the
> >>same semantics.
> >
> >
> > No.
>
> There are some nasty checks for it != NULL in the generic BIO code.

No, there are no checks there.

> And REQ_BARRIER got introduced just recently, so we can see the
> error will have been propagated.
>
> >>>And yes it will be useful to move it to block layer.
> >>
> >>Done. Just needs testing. I have at least an ZIP parport drive, which
> >>allows me to do some basic checks.
> >
> >
> > Test everything on your production machine + main hdd.
> > Will make you care more about code correctness ;-).
>
> Nah... that's just for the SCSI code "move around". The rest
> I usually run continuously on two systems.
>
> >>BTW.> Having a fill up request queue trashing data transfers
> >>is indicating that there may be are bugs in the generic block layer too.
> >>If it gets pushed to boundary conditions it's apparently not very
> >
> >
> > No it wasn't "pushed to boundary conditions", you screwed it, sorry.
> >
> >
> >>robust... BTW.> I never ever will understand why
> >>request_fn returns void instead of an status value.
> >
> >
> > So look at ide.c for example.
>
> So look at drivers which call blk_start_queue() from within
> q->request_fn context, which is, well, causing deliberate *recursion*.
>

Are you sure? If so they should first check whether queue is
started/stopped, if they don't it is a bug.

Look once agin at ide.c, it is called, starts request (if not busy) and
returns, no waiting for finishing request == no return value.

Regards
--
Bartlomiej

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:

>>There are some nasty checks for it != NULL in the generic BIO code.
> 
> 
> No, there are no checks there.

Hello?:

[root@localhost block]# grep \>special *.c
elevator.c:         !rq->waiting && !rq->special)
^^^^^^ This one is supposed to have the required barrier effect.
ll_rw_blk.c:    if (req->special || next->special)
ll_rw_blk.c:            rq->special = NULL;
ll_rw_blk.c:    rq->special = data;
^^^^^^^ This one is me :-).
ll_rw_blk.c:        || next->waiting || next->special)
[root@localhost block]#


>>>So look at ide.c for example.
>>
>>So look at drivers which call blk_start_queue() from within
>>q->request_fn context, which is, well, causing deliberate *recursion*.
>>
> 
> 
> Are you sure? If so they should first check whether queue is
> started/stopped, if they don't it is a bug.

void blk_start_queue(request_queue_t *q)
{
         if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
                 unsigned long flags;

================== possigle race here for qeue_flags BTW.

                 spin_lock_irqsave(q->queue_lock, flags);
                 clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);

                 if (!elv_queue_empty(q))
                         q->request_fn(q);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If we call it from within request_fn then if this isn't recursion on the
kernel stack then I don't know...

                 spin_unlock_irqrestore(q->queue_lock, flags);
         }
}

Re: please DON'T run 2.5.27 with IDE!

[Jens Axboe]

On Wed, Jul 24 2002, Marcin Dalecki wrote:
> >>So look at drivers which call blk_start_queue() from within
> >>q->request_fn context, which is, well, causing deliberate *recursion*.
> >>
> >
> >
> >Are you sure? If so they should first check whether queue is
> >started/stopped, if they don't it is a bug.
> 
> void blk_start_queue(request_queue_t *q)
> {
>         if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
>                 unsigned long flags;
> 
> ================== possigle race here for qeue_flags BTW.
> 
>                 spin_lock_irqsave(q->queue_lock, flags);
>                 clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
> 
>                 if (!elv_queue_empty(q))
>                         q->request_fn(q);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> If we call it from within request_fn then if this isn't recursion on the
> kernel stack then I don't know...
> 
>                 spin_unlock_irqrestore(q->queue_lock, flags);
>         }
> }

Care to enlighten us on exactly which block drivers call
blk_start_queue() from request_fn?

-- 
Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Jens Axboe wrote:

> 
> Care to enlighten us on exactly which block drivers call
> blk_start_queue() from request_fn?

Well, admittedly, my direct suspect (cpqarry.c) turned out after a 
closer look to be "not guilty" :-). Still a bit of docu found be nice 
there. And you can of course guess what the bounty is I'm hunting for:
removal of IDE_BUSY ... so I did the mistake myself in first place 8-).

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
>
> >>There are some nasty checks for it != NULL in the generic BIO code.
                                               ^^^^^^^^^^^^^^^^^^^^^^^
> >
> >
> > No, there are no checks there.
>
> Hello?:

Yes, hello Martin, read you own sentence ;-)

Generic BIO code is bio.c and bio.h.

> [root@localhost block]# grep \>special *.c
> elevator.c:         !rq->waiting && !rq->special)
> ^^^^^^ This one is supposed to have the required barrier effect.
> ll_rw_blk.c:    if (req->special || next->special)
> ll_rw_blk.c:            rq->special = NULL;
> ll_rw_blk.c:    rq->special = data;
> ^^^^^^^ This one is me :-).
> ll_rw_blk.c:        || next->waiting || next->special)
> [root@localhost block]#

You seem to confuse block layer with BIO layer.

> >>>So look at ide.c for example.
> >>
> >>So look at drivers which call blk_start_queue() from within
> >>q->request_fn context, which is, well, causing deliberate *recursion*.
> >>
> >
> > Are you sure? If so they should first check whether queue is
> > started/stopped, if they don't it is a bug.
>
> void blk_start_queue(request_queue_t *q)
> {
>          if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
>                  unsigned long flags;
>
> ================== possigle race here for qeue_flags BTW.
>
>                  spin_lock_irqsave(q->queue_lock, flags);
>                  clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
>
>                  if (!elv_queue_empty(q))
>                          q->request_fn(q);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> If we call it from within request_fn then if this isn't recursion on the
> kernel stack then I don't know...

You really don't know.

And funny thing is I have diffirent blk_start_queue() function in my tree
(2.5.27) ? Without described above race and without possibilty of
recursion...

2.5.27:drivers/block/ll_rw_blk.c
void blk_start_queue(request_queue_t *q)
{
        if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
                unsigned long flags;

                spin_lock_irqsave(q->queue_lock, flags);
                if (!elv_queue_empty(q))
                        q->request_fn(q);
                spin_unlock_irqrestore(q->queue_lock, flags);
        }
}

Regards
--
Bartlomiej

>                  spin_unlock_irqrestore(q->queue_lock, flags);
>          }
> }

Re: please DON'T run 2.5.27 with IDE!

[Jens Axboe]

On Wed, Jul 24 2002, Bartlomiej Zolnierkiewicz wrote:
> > void blk_start_queue(request_queue_t *q)
> > {
> >          if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> >                  unsigned long flags;
> >
> > ================== possigle race here for qeue_flags BTW.
> >
> >                  spin_lock_irqsave(q->queue_lock, flags);
> >                  clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
> >
> >                  if (!elv_queue_empty(q))
> >                          q->request_fn(q);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > If we call it from within request_fn then if this isn't recursion on the
> > kernel stack then I don't know...
> 
> You really don't know.
> 
> And funny thing is I have diffirent blk_start_queue() function in my tree
> (2.5.27) ? Without described above race and without possibilty of
> recursion...
> 
> 2.5.27:drivers/block/ll_rw_blk.c
> void blk_start_queue(request_queue_t *q)
> {
>         if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
>                 unsigned long flags;
> 
>                 spin_lock_irqsave(q->queue_lock, flags);
>                 if (!elv_queue_empty(q))
>                         q->request_fn(q);
>                 spin_unlock_irqrestore(q->queue_lock, flags);
>         }
> }

Yep, the version Martin posted must be really old.

-- 
Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Marcin Dalecki wrote:

> [root@localhost block]# grep \>special *.c
> elevator.c:         !rq->waiting && !rq->special)
> ^^^^^^ This one is supposed to have the required barrier effect.

Go reread, no barrier effect, requests can slip in before your
REQ_SPECIAL. They cannon only be merged with REQ_SPECIAL.

Regards
--
Bartlomiej

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Jens Axboe wrote:

> On Wed, Jul 24 2002, Bartlomiej Zolnierkiewicz wrote:
> > > void blk_start_queue(request_queue_t *q)
> > > {
> > >          if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> > >                  unsigned long flags;
> > >
> > > ================== possigle race here for qeue_flags BTW.
> > >
> > >                  spin_lock_irqsave(q->queue_lock, flags);
> > >                  clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
> > >
> > >                  if (!elv_queue_empty(q))
> > >                          q->request_fn(q);
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > If we call it from within request_fn then if this isn't recursion on the
> > > kernel stack then I don't know...
> >
> > You really don't know.
> >
> > And funny thing is I have diffirent blk_start_queue() function in my tree
> > (2.5.27) ? Without described above race and without possibilty of
> > recursion...
> >
> > 2.5.27:drivers/block/ll_rw_blk.c
> > void blk_start_queue(request_queue_t *q)
> > {
> >         if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> >                 unsigned long flags;
> >
> >                 spin_lock_irqsave(q->queue_lock, flags);
> >                 if (!elv_queue_empty(q))
> >                         q->request_fn(q);
> >                 spin_unlock_irqrestore(q->queue_lock, flags);
> >         }
> > }
>
> Yep, the version Martin posted must be really old.

It's worst. Martin's version exists only in his tree (mind?).
I checked back revision and blk_start_queue() was inroduced in 2.5.19
and it was the correct one.

> --
> Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Jens Axboe]

On Wed, Jul 24 2002, Bartlomiej Zolnierkiewicz wrote:
> 
> On Wed, 24 Jul 2002, Jens Axboe wrote:
> 
> > On Wed, Jul 24 2002, Bartlomiej Zolnierkiewicz wrote:
> > > > void blk_start_queue(request_queue_t *q)
> > > > {
> > > >          if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> > > >                  unsigned long flags;
> > > >
> > > > ================== possigle race here for qeue_flags BTW.
> > > >
> > > >                  spin_lock_irqsave(q->queue_lock, flags);
> > > >                  clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
> > > >
> > > >                  if (!elv_queue_empty(q))
> > > >                          q->request_fn(q);
> > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > If we call it from within request_fn then if this isn't recursion on the
> > > > kernel stack then I don't know...
> > >
> > > You really don't know.
> > >
> > > And funny thing is I have diffirent blk_start_queue() function in my tree
> > > (2.5.27) ? Without described above race and without possibilty of
> > > recursion...
> > >
> > > 2.5.27:drivers/block/ll_rw_blk.c
> > > void blk_start_queue(request_queue_t *q)
> > > {
> > >         if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> > >                 unsigned long flags;
> > >
> > >                 spin_lock_irqsave(q->queue_lock, flags);
> > >                 if (!elv_queue_empty(q))
> > >                         q->request_fn(q);
> > >                 spin_unlock_irqrestore(q->queue_lock, flags);
> > >         }
> > > }
> >
> > Yep, the version Martin posted must be really old.
> 
> It's worst. Martin's version exists only in his tree (mind?).

:-)

> I checked back revision and blk_start_queue() was inroduced in 2.5.19
> and it was the correct one.

There were buggy versions at one point, however they may not have made it
into a full release. In that case it was just bk version of 2.5.19-pre
effectively. I forget the details :-)

-- 
Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Jens Axboe wrote:

>>>>2.5.27:drivers/block/ll_rw_blk.c
>>>>void blk_start_queue(request_queue_t *q)
>>>>{
>>>>        if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
>>>>                unsigned long flags;
>>>>
>>>>                spin_lock_irqsave(q->queue_lock, flags);
>>>>                if (!elv_queue_empty(q))
>>>>                        q->request_fn(q);
>>>>                spin_unlock_irqrestore(q->queue_lock, flags);
>>>>        }
>>>>}

> There were buggy versions at one point, however they may not have made it
> into a full release. In that case it was just bk version of 2.5.19-pre
> effectively. I forget the details :-)

Naj - it's far more trivial I just looked at wrong tree at hand...
But anyway. What happens if somone does set QUEUE_FLAG_STOPPED
between the test_and_claer_bit and taking the spin_lock? Setting
the QUEUE_FLAG_STOPPED isn't maintaining the spin_lock protection!

My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
*inside* the q->request_fn call.

This here is where it's supposed to be set:

void blk_stop_queue(request_queue_t *q)
{
         unsigned long flags;

         spin_lock_irqsave(q->queue_lock, flags);
         blk_remove_plug(q);
         spin_unlock_irqrestore(q->queue_lock, flags);

         set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
}

And I don't see anything preventing the above problem.

It sould perhaps be?

void blk_stop_queue(request_queue_t *q)
{
         unsigned long flags;

         spin_lock_irqsave(q->queue_lock, flags);
         blk_remove_plug(q);
         set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags); /* Notice 
spinlock still held! */
         spin_unlock_irqrestore(q->queue_lock, flags);
}

void blk_start_queue(request_queue_t *q)
{
         if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
                 unsigned long flags;

                 spin_lock_irqsave(q->queue_lock, flags);
		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
			spin_unlock_irqsave(q->queue_lock, flags);
			return;
		}
                 clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
                 if (!elv_queue_empty(q))
                         q->request_fn(q);
                 spin_unlock_irqrestore(q->queue_lock, flags);
         }
}

At least I couldn't see any harm in doing it like above.
And again. I think it would assert that the flag is well defined inside
q->request_fn().

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:
> On Wed, 24 Jul 2002, Marcin Dalecki wrote:
> 
> 
>>[root@localhost block]# grep \>special *.c
>>elevator.c:         !rq->waiting && !rq->special)
>>^^^^^^ This one is supposed to have the required barrier effect.
> 
> 
> Go reread, no barrier effect, requests can slip in before your
> REQ_SPECIAL. They cannon only be merged with REQ_SPECIAL.

Erm. Please note that I don't see any problem here. It's just
a matter of completeness.

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
> > On Wed, 24 Jul 2002, Marcin Dalecki wrote:
> >
> >
> >>[root@localhost block]# grep \>special *.c
> >>elevator.c:         !rq->waiting && !rq->special)
> >>^^^^^^ This one is supposed to have the required barrier effect.
> >
> >
> > Go reread, no barrier effect, requests can slip in before your
> > REQ_SPECIAL. They cannon only be merged with REQ_SPECIAL.
>
> Erm. Please note that I don't see any problem here. It's just
> a matter of completeness.

For now yes.

Re: please DON'T run 2.5.27 with IDE!

[Jens Axboe]

On Wed, Jul 24 2002, Marcin Dalecki wrote:
> Jens Axboe wrote:
> 
> >>>>2.5.27:drivers/block/ll_rw_blk.c
> >>>>void blk_start_queue(request_queue_t *q)
> >>>>{
> >>>>       if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> >>>>               unsigned long flags;
> >>>>
> >>>>               spin_lock_irqsave(q->queue_lock, flags);
> >>>>               if (!elv_queue_empty(q))
> >>>>                       q->request_fn(q);
> >>>>               spin_unlock_irqrestore(q->queue_lock, flags);
> >>>>       }
> >>>>}
> 
> >There were buggy versions at one point, however they may not have made it
> >into a full release. In that case it was just bk version of 2.5.19-pre
> >effectively. I forget the details :-)
> 
> Naj - it's far more trivial I just looked at wrong tree at hand...
> But anyway. What happens if somone does set QUEUE_FLAG_STOPPED
> between the test_and_claer_bit and taking the spin_lock? Setting
> the QUEUE_FLAG_STOPPED isn't maintaining the spin_lock protection!

It doesn't matter. If QUEUE_FLAG_STOPPED was set when entering
blk_start_queue(), it will call into the request_fn. If blk_stop_queue()
is called between clearing QUEUE_FLAG_STOPPED in blk_start_queue() and
grabbing the spin_lock, the worst that can happen is a spurios extra
request_fn call.

> My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
> *inside* the q->request_fn call.

So you want the queue_lock to protect the flags as well... I don't
really see the point of this.

-- 
Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Jens Axboe wrote:

> On Wed, Jul 24 2002, Marcin Dalecki wrote:
> > Jens Axboe wrote:
> >
> > >>>>2.5.27:drivers/block/ll_rw_blk.c
> > >>>>void blk_start_queue(request_queue_t *q)
> > >>>>{
> > >>>>       if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> > >>>>               unsigned long flags;
> > >>>>
> > >>>>               spin_lock_irqsave(q->queue_lock, flags);
> > >>>>               if (!elv_queue_empty(q))
> > >>>>                       q->request_fn(q);
> > >>>>               spin_unlock_irqrestore(q->queue_lock, flags);
> > >>>>       }
> > >>>>}
> >
> > >There were buggy versions at one point, however they may not have made it
> > >into a full release. In that case it was just bk version of 2.5.19-pre
> > >effectively. I forget the details :-)
> >
> > Naj - it's far more trivial I just looked at wrong tree at hand...
> > But anyway. What happens if somone does set QUEUE_FLAG_STOPPED
> > between the test_and_claer_bit and taking the spin_lock? Setting
> > the QUEUE_FLAG_STOPPED isn't maintaining the spin_lock protection!
>
> It doesn't matter. If QUEUE_FLAG_STOPPED was set when entering
> blk_start_queue(), it will call into the request_fn. If blk_stop_queue()
> is called between clearing QUEUE_FLAG_STOPPED in blk_start_queue() and
> grabbing the spin_lock, the worst that can happen is a spurios extra
> request_fn call.
>
> > My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
> > *inside* the q->request_fn call.
>
> So you want the queue_lock to protect the flags as well... I don't
> really see the point of this.

If driver corectly uses blk_start/stop_queue() it is not needed.
Whole point of introducing this flag was not to take lock to check
status of queue.

--
Bartlomiej

> --
> Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

>>Naj - it's far more trivial I just looked at wrong tree at hand...
>>But anyway. What happens if somone does set QUEUE_FLAG_STOPPED
>>between the test_and_claer_bit and taking the spin_lock? Setting
>>the QUEUE_FLAG_STOPPED isn't maintaining the spin_lock protection!
> 
> 
> It doesn't matter. If QUEUE_FLAG_STOPPED was set when entering
> blk_start_queue(), it will call into the request_fn. If blk_stop_queue()
> is called between clearing QUEUE_FLAG_STOPPED in blk_start_queue() and
> grabbing the spin_lock, the worst that can happen is a spurios extra
> request_fn call.
> 
> 
>>My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
>>*inside* the q->request_fn call.
> 
> 
> So you want the queue_lock to protect the flags as well... I don't
> really see the point of this.

Well - OK it's maybe not obvious. So let me please explain: What I have 
in mind is...

1. It doesn't harm and it's a matter of completeness ...
(brain -pedantic)

2. QUEUE_FLAG_STOPPED would suddenly do the same trick as IDE_BUSY does
now and I could just do blk_start_queue() in timout and IRQ handlers in 
IDE. This would bring the "driver in question" in line with all the 
other drivers out there, which indeed do just that instead of explicite
recurrsion in to the request handler...

3. The while(test_and_set_bit(IDE_BUSY, ... ) on do_ide_request entry
could simply go away... and we would have just do_request() left.

4. I worry a bit how this interacts with tcq.c

5. I observed the BUG() during transfers running from one queue to
another comented as by you:

  /* There's a small window between where the queue could be
   * replugged while we are in here when using tcq (in which case
   * the queue is probably empty anyways...), so check and leave
   * if appropriate. When not using tcq, this is still a severe
   * BUG!
   */

in do_request() on a system with enabled preemption and without TCQ
enabled. And I think that the above would plug the possibility of it to 
happen. (Tought here I have to think harder.)

Re: please DON'T run 2.5.27 with IDE!

[Jens Axboe]

On Wed, Jul 24 2002, Bartlomiej Zolnierkiewicz wrote:
> 
> On Wed, 24 Jul 2002, Jens Axboe wrote:
> 
> > On Wed, Jul 24 2002, Marcin Dalecki wrote:
> > > Jens Axboe wrote:
> > >
> > > >>>>2.5.27:drivers/block/ll_rw_blk.c
> > > >>>>void blk_start_queue(request_queue_t *q)
> > > >>>>{
> > > >>>>       if (test_and_clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> > > >>>>               unsigned long flags;
> > > >>>>
> > > >>>>               spin_lock_irqsave(q->queue_lock, flags);
> > > >>>>               if (!elv_queue_empty(q))
> > > >>>>                       q->request_fn(q);
> > > >>>>               spin_unlock_irqrestore(q->queue_lock, flags);
> > > >>>>       }
> > > >>>>}
> > >
> > > >There were buggy versions at one point, however they may not have made it
> > > >into a full release. In that case it was just bk version of 2.5.19-pre
> > > >effectively. I forget the details :-)
> > >
> > > Naj - it's far more trivial I just looked at wrong tree at hand...
> > > But anyway. What happens if somone does set QUEUE_FLAG_STOPPED
> > > between the test_and_claer_bit and taking the spin_lock? Setting
> > > the QUEUE_FLAG_STOPPED isn't maintaining the spin_lock protection!
> >
> > It doesn't matter. If QUEUE_FLAG_STOPPED was set when entering
> > blk_start_queue(), it will call into the request_fn. If blk_stop_queue()
> > is called between clearing QUEUE_FLAG_STOPPED in blk_start_queue() and
> > grabbing the spin_lock, the worst that can happen is a spurios extra
> > request_fn call.
> >
> > > My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
> > > *inside* the q->request_fn call.
> >
> > So you want the queue_lock to protect the flags as well... I don't
> > really see the point of this.
> 
> If driver corectly uses blk_start/stop_queue() it is not needed.
> Whole point of introducing this flag was not to take lock to check
> status of queue.

Right. The way it was meant to be used it how cpqarray and cciss does
it -- stopping queue in request_fn, restarting it from isr.

-- 
Jens Axboe

Re: please DON'T run 2.5.27 with IDE!

[Marcin Dalecki]

Jens Axboe wrote:

>>>>My goal is to make sure that the QUEUE_FLAG_STOPPED has a valid value
>>>>*inside* the q->request_fn call.
>>>
>>>So you want the queue_lock to protect the flags as well... I don't
>>>really see the point of this.
>>
>>If driver corectly uses blk_start/stop_queue() it is not needed.
>>Whole point of introducing this flag was not to take lock to check
>>status of queue.
> 
> 
> Right. The way it was meant to be used it how cpqarray and cciss does
> it -- stopping queue in request_fn, restarting it from isr.

Yes got it. Of course. But please note that:

1. My suggestion doesn't change the checking case.

2. What you describe is precisely what I would like to be able to
do in my own "playground".