Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Roberto Nibali]

Hi,

What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed? I mean 
readahead doesn't seem to be exported anymore (at least through those 
interfaces). hdparm v4.6 does of course not like this (But this is a 
user space problem). Following output describes what I mean:

laphish:/usr/src/linux-2.5.7-pre1-orig # egrep -r 
"BLKRAGET|BLKFRAGET|BLKSECTGET" .
./arch/sparc64/kernel/ioctl32.c:HANDLE_IOCTL(BLKSECTGET, w_long)
./arch/mips64/kernel/ioctl32.c: 
IOCTL32_HANDLER(BLKSECTGET, w_long),
./arch/ppc64/kernel/ioctl32.c:HANDLE_IOCTL(BLKRAGET, w_long),
./arch/ppc64/kernel/ioctl32.c:HANDLE_IOCTL(BLKFRAGET, w_long),
./arch/ppc64/kernel/ioctl32.c:HANDLE_IOCTL(BLKSECTGET, w_long),
./arch/x86_64/ia32/ia32_ioctl.c:HANDLE_IOCTL(BLKSECTGET, w_long)
./drivers/acorn/block/mfmhd.c: 
case BLKSECTGET:
./drivers/block/blkpg.c: 
	case BLKSECTGET:
./drivers/md/lvm.c: *    09/02/1999 - changed BLKRASET and BLKRAGET in 
lvm_chr_ioctl() to
./include/linux/fs.h:#define BLKRAGET   _IO(0x12,99)	/* get current read ahead setting */
./include/linux/fs.h:#define BLKFRAGET  _IO(0x12,101)/* get filesystem 
(mm/filemap.c) read-ahead */
./include/linux/fs.h:#define BLKSECTGET _IO(0x12,103)/* get max sectors 
per request (ll_rw_blk.c) */
laphish:/usr/src/linux-2.5.7-pre1-orig #

At least BLKRAGET and BLKFRAGET don't seem to be used anymore.

Regards,
Roberto Nibali, ratz

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Andrew Morton]

Roberto Nibali wrote:
> 
> Hi,
> 
> What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed?

They got collaterally damaged in the IDE "cleanup".  The patch at
http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
resurrects them.

-

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Roberto Nibali]

> They got collaterally damaged in the IDE "cleanup".  The patch at
> http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
> resurrects them.

Oh, I see. I've missed that patch of yours. I certainly enjoyed (maybe 
much to your grief) the comments in the code :).

Is GFP_READAHEAD still a wish or did you drop that idea? AFAICS you only 
addressed the i386 arch with that patch, do you want the specific arch 
maintainers to clean up their part when your patch is finished?

Cheers,
Roberto Nibali, ratz

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Andrew Morton]

Roberto Nibali wrote:
> 
> > They got collaterally damaged in the IDE "cleanup".  The patch at
> > http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
> > resurrects them.
> 
> Oh, I see. I've missed that patch of yours. I certainly enjoyed (maybe
> much to your grief) the comments in the code :).

hmm.  I'd better go back and check them then ;)

> Is GFP_READAHEAD still a wish or did you drop that idea?

Dropped.  Bad idea.  If we have to do I/O to gather the readahead pages
then so be it.  That I/O will cluster well, as will the subsequent readahead,
which is better than giving up on the readahead.

> AFAICS you only
> addressed the i386 arch with that patch, do you want the specific arch
> maintainers to clean up their part when your patch is finished?

?  There's nothing arch-specific in any of this...

-

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Martin Dalecki]

Andrew Morton wrote:
> Roberto Nibali wrote:
> 
>>>They got collaterally damaged in the IDE "cleanup".  The patch at
>>>http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
>>>resurrects them.
>>>
>>Oh, I see. I've missed that patch of yours. I certainly enjoyed (maybe
>>much to your grief) the comments in the code :).
>>
> 
> hmm.  I'd better go back and check them then ;)
> 
> 
>>Is GFP_READAHEAD still a wish or did you drop that idea?
>>
> 
> Dropped.  Bad idea.  If we have to do I/O to gather the readahead pages
> then so be it.  That I/O will cluster well, as will the subsequent readahead,
> which is better than giving up on the readahead.
> 
> 
>>AFAICS you only
>>addressed the i386 arch with that patch, do you want the specific arch
>>maintainers to clean up their part when your patch is finished?
>>
> 
> ?  There's nothing arch-specific in any of this...

And there is nothing IDE related either. The code removed
at the time wasn't used!

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Roberto Nibali]

>>> AFAICS you only
>>> addressed the i386 arch with that patch, do you want the specific arch
>>> maintainers to clean up their part when your patch is finished?
>>>
>>
>> ?  There's nothing arch-specific in any of this...
>  
> And there is nothing IDE related either. The code removed
> at the time wasn't used!

I see. So the trick is to fix hdparm and tell it not to use ioctl(fd, 
BLKRAGET, arg). But I don't think that the BIO changes introduce a means 
for readahead control/export from/to user space? Or would this be 
something like bio_ioctl(kdev_t, unsigned int, unsigned long), which is 
actually not used anywhere, or the request queue approach used by Andrew 
Morton?

The reason I was confused about the arch was that sparc64, ppc64, 
mips64, s390x and x86_64 still provide a ioctl handler for those ioctl's 
hooking up the the w_long (interestig naming concept btw) function.

Am I completely off the track here, mixing things up?

Best regards,
Roberto Nibali, ratz

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Martin Dalecki]

Roberto Nibali wrote:
>>>> AFAICS you only
>>>> addressed the i386 arch with that patch, do you want the specific arch
>>>> maintainers to clean up their part when your patch is finished?
>>>>
>>>
>>> ?  There's nothing arch-specific in any of this...
>>
>>  
>> And there is nothing IDE related either. The code removed
>> at the time wasn't used!
> 
> 
> I see. So the trick is to fix hdparm and tell it not to use ioctl(fd, 
> BLKRAGET, arg). But I don't think that the BIO changes introduce a means 
> for readahead control/export from/to user space? Or would this be 
> something like bio_ioctl(kdev_t, unsigned int, unsigned long), which is 
> actually not used anywhere, or the request queue approach used by Andrew 
> Morton?
> 
> The reason I was confused about the arch was that sparc64, ppc64, 
> mips64, s390x and x86_64 still provide a ioctl handler for those ioctl's 
> hooking up the the w_long (interestig naming concept btw) function.
> 
> Am I completely off the track here, mixing things up?

No I think you are entierly on track.

BTW> It's quite propably right now, that I will just reintroduce them
myself and give them the semantics of the multi-write hardware settings,
just to fix the multi write PIO problem :-).

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Jens Axboe]

On Mon, Mar 18 2002, Martin Dalecki wrote:
[BLKRAGET etc]
> BTW> It's quite propably right now, that I will just reintroduce them
> myself and give them the semantics of the multi-write hardware settings,
> just to fix the multi write PIO problem :-).

What would that fix?

I've still got the multi-write fixes pending, out tomorrow I hope. Other
stuff keeps getting in the way. I haven't forgotten :-)

-- 
Jens Axboe

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Martin Dalecki]

Jens Axboe wrote:
> On Mon, Mar 18 2002, Martin Dalecki wrote:
> [BLKRAGET etc]
> 
>>BTW> It's quite propably right now, that I will just reintroduce them
>>myself and give them the semantics of the multi-write hardware settings,
>>just to fix the multi write PIO problem :-).
> 
> 
> What would that fix?
> 
> I've still got the multi-write fixes pending, out tomorrow I hope. Other
> stuff keeps getting in the way. I haven't forgotten :-)

I think that it would make the write operation atomic in respect
to the BIO chunk write order. Or please have a look at
the following piece of cr... code from ide-taskfile.c:

	/* (ks/hs): See task_mulin_intr */
	msect = drive->mult_count;
	nsect = rq->current_nr_sectors;
	if (nsect > msect)
		nsect = msect;

	pBuf = ide_map_rq(rq, &flags);
	DTF("Multiwrite: %p, nsect: %d , rq->current_nr_sectors: %ld\n",
		pBuf, nsect, rq->current_nr_sectors);
	drive->io_32bit = 0;
	taskfile_output_data(drive, pBuf, nsect * SECTOR_WORDS);
	ide_unmap_rq(rq, pBuf, &flags);
	drive->io_32bit = io_32bit;
	

in esp. the nsect versus msect games whould go away and
if we have current_nr_sectors > drive->mult_count, we
are not going to write everything in one operation as it stands...
The above just *feels* to me like something that should
be pushed one layer upwards, since the sematics it has fits
quite nicely into what the ioctl in question should be about.

Plase note that I'm just speculating and feels free to correct me
if I'm entierly mistaken for some reason.

Please note as well that this is more about extending then
about fixing.

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Jens Axboe]

On Mon, Mar 18 2002, Martin Dalecki wrote:
> Jens Axboe wrote:
> >On Mon, Mar 18 2002, Martin Dalecki wrote:
> >[BLKRAGET etc]
> >
> >>BTW> It's quite propably right now, that I will just reintroduce them
> >>myself and give them the semantics of the multi-write hardware settings,
> >>just to fix the multi write PIO problem :-).
> >
> >
> >What would that fix?
> >
> >I've still got the multi-write fixes pending, out tomorrow I hope. Other
> >stuff keeps getting in the way. I haven't forgotten :-)
> 
> I think that it would make the write operation atomic in respect
> to the BIO chunk write order. Or please have a look at
> the following piece of cr... code from ide-taskfile.c:
> 
> 	/* (ks/hs): See task_mulin_intr */
> 	msect = drive->mult_count;
> 	nsect = rq->current_nr_sectors;
> 	if (nsect > msect)
> 		nsect = msect;
> 
> 	pBuf = ide_map_rq(rq, &flags);
> 	DTF("Multiwrite: %p, nsect: %d , rq->current_nr_sectors: %ld\n",
> 		pBuf, nsect, rq->current_nr_sectors);
> 	drive->io_32bit = 0;
> 	taskfile_output_data(drive, pBuf, nsect * SECTOR_WORDS);
> 	ide_unmap_rq(rq, pBuf, &flags);
> 	drive->io_32bit = io_32bit;
> 	
> 
> in esp. the nsect versus msect games whould go away and
> if we have current_nr_sectors > drive->mult_count, we
> are not going to write everything in one operation as it stands...
> The above just *feels* to me like something that should
> be pushed one layer upwards, since the sematics it has fits
> quite nicely into what the ioctl in question should be about.

You cannot guarentee that current_nr_sectors will be >= mult_count just
by doing that. One fix I did that was in the kernel for a while was to
just always setup the transfer for mult_count number of sectors at most.
This worked, but meant that we would potentially have to restart (output
command etc from scratch) a command a lot of times.

There a a lot of solutions to the problem, some better than others. The
whole issue boils dow to two things. One is that right after this
starts:

 	taskfile_output_data(drive, pBuf, nsect * SECTOR_WORDS);

you must be ready to handle the next transfer, ie the request state must
be sane. The other is that signalling request completion can get tricky,
as always with pio writes.

-- 
Jens Axboe

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Martin Dalecki]

Andrew Morton wrote:
> Roberto Nibali wrote:
> 
>>Hi,
>>
>>What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed?
>>
> 
> They got collaterally damaged in the IDE "cleanup".  The patch at
> http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
> resurrects them.

This is WRONG. What I did here was just removal of unused code.
They got obsoleted by the BIO infrastructure changes.

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Jeff Garzik]

Martin Dalecki wrote:

> Andrew Morton wrote:
>
>> Roberto Nibali wrote:
>>
>>> What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed? 
>>
>>
>> They got collaterally damaged in the IDE "cleanup".  The patch at
>> http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch 
>>
>> resurrects them.
>
>
> This is WRONG. What I did here was just removal of unused code.
> They got obsoleted by the BIO infrastructure changes.

Martin,

Did Andrew really deserve that?

Andrew's patch -implements- those ioctls.

Can our new IDE maintainer please have a little bit more patience and 
respect to those who have been hacking the kernel actively for a while? 
 Andrew certainly has earned our respect... calling changes wrong 
without reading them does not.

    Jeff

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Martin Dalecki]

Jeff Garzik wrote:
> Martin Dalecki wrote:
> 
>> Andrew Morton wrote:
>>
>>> Roberto Nibali wrote:
>>>
>>>> What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed? 
>>>
>>>
>>>
>>> They got collaterally damaged in the IDE "cleanup".  The patch at
>>> http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch 
>>>
>>> resurrects them.
>>
>>
>>
>> This is WRONG. What I did here was just removal of unused code.
>> They got obsoleted by the BIO infrastructure changes.
> 
> 
> Martin,
> 
> Did Andrew really deserve that?
> 
> Andrew's patch -implements- those ioctls.
> 
> Can our new IDE maintainer please have a little bit more patience and 
> respect to those who have been hacking the kernel actively for a while? 
> Andrew certainly has earned our respect... calling changes wrong without 
> reading them does not.

It was just a note on the who "broke them". However I welcome his
efforts to actually implement the functionality in a clean manner.
Still something unclear? I wish not?

Re: Question about the ide related ioctl's BLK* in 2.5.7-pre1 kernel

[Andrew Morton]

Martin Dalecki wrote:
> 
> Andrew Morton wrote:
> > Roberto Nibali wrote:
> >
> >>Hi,
> >>
> >>What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed?
> >>
> >
> > They got collaterally damaged in the IDE "cleanup".  The patch at
> > http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.6/dallocbase-10-readahead.patch
> > resurrects them.
> 
> This is WRONG. What I did here was just removal of unused code.
> They got obsoleted by the BIO infrastructure changes.

Actually, it's right.

In mid-February you asserted that the IDE readahead controls were
inoperative.  You said:

> You are missing one simple thing: The removed values doen't control
> ANYTHING!

I asked:

> Please explain, in detail, why /proc/ide/hdX/settings:file_readhead
> no longer controls the readhead for that device.  If this is
> the case in thr current 2.4 kernel, or if it will become the
> case if/when the IDE patches are merged then that needs fixing.
> 

You didn't answer.

I tested them.  They still worked.

I also said:

> 
> Look, I agree that the current readhead controls are junk, and
> do not belong in the driver layer at all.   All I'm saying is
> that we need per-device controls, for whatever scheme we end
> up using for readhead in 2.5.   We really don't want to have
> the same sized readhead for CDROMs, floppies and super-duper
> RAID arrays.

Then the device-driver-based readahead controls got taken out.

I really do agree that it was a pile of crap.  I've turned them
into a property of the request queue, which is a more appropriate
place for them.

In my current patch, the per-queue readahead parameter is controlled
via the old ioctls.  Probably, these will be retired in favour of
/proc/iosched, when that turns up.

In the meantime, I *need* those tunables for ongoing development.

-