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

DISCUSSION Roberto Nibali 5 participants 14 messages Mar 14, 2002 – Mar 18, 2002

Share & tools

Roberto Nibali

author

2002-03-14 02:16 UTC #0 lore raw

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

Andrew Morton

2002-03-14 02:27 UTC #1 lore raw

Quoted 4 lines · Roberto Nibali

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.

Roberto Nibali

author

2002-03-14 08:00 UTC #2 lore raw

In reply to Roberto Nibali

Quoted 3 lines

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

Andrew Morton

2002-03-14 08:15 UTC #3 lore raw

In reply to Roberto Nibali

Quoted 7 lines · Roberto Nibali

Quoted 4 lines
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 ;)

Quoted 1 line

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.

Quoted 3 lines

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...

Martin Dalecki

2002-03-14 12:06 UTC #4 lore raw

In reply to Roberto Nibali

Quoted 21 lines · Andrew Morton

Quoted 7 lines · Roberto Nibali
Quoted 4 lines
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 ;)
Quoted 2 lines
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.

Quoted 6 lines

Quoted 4 lines
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!

Martin Dalecki

2002-03-14 12:13 UTC #5 lore raw

In reply to Roberto Nibali

Quoted 10 lines · Andrew Morton

Quoted 4 lines · Roberto Nibali
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.

Jeff Garzik

2002-03-14 12:44 UTC #6 lore raw

In reply to Roberto Nibali

Quoted 7 lines · Martin Dalecki

Quoted 5 lines · Andrew Morton
Quoted 1 line · Roberto Nibali
What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed?

Quoted 8 lines

Quoted 4 lines
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

Martin Dalecki

2002-03-14 12:54 UTC #7 lore raw

In reply to Roberto Nibali

Quoted 21 lines · Jeff Garzik

Quoted 11 lines · Martin Dalecki
Quoted 9 lines · Andrew Morton
Quoted 1 line · Roberto Nibali
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
Quoted 6 lines
Quoted 1 line
resurrects them.
This is WRONG. What I did here was just removal of unused code. They got obsoleted by the BIO infrastructure changes.

Quoted 7 lines

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?

Andrew Morton

2002-03-14 17:41 UTC #8 lore raw

In reply to Roberto Nibali

Quoted 9 lines · Martin Dalecki

Quoted 7 lines · Andrew Morton
Quoted 4 lines · Roberto Nibali
Hi,
What for are BLKRAGET, BLKFRAGET and BLKSECTGET still needed?

Quoted 6 lines

Quoted 3 lines
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

Quoted 2 lines · inoperative. You

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

I asked:

Quoted 5 lines

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.

Quoted 7 lines · I also

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.

Roberto Nibali

author

2002-03-14 20:04 UTC #9 lore raw

In reply to Roberto Nibali

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
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

Martin Dalecki

2002-03-18 13:30 UTC #10 lore raw

In reply to Roberto Nibali

Quoted 12 lines · Roberto Nibali

Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
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!

Quoted 5 lines

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 :-).

Jens Axboe

2002-03-18 13:35 UTC #11 lore raw

In reply to Roberto Nibali

On Mon, Mar 18 2002, Martin Dalecki wrote:
[BLKRAGET etc]

Quoted 3 lines

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

Signature · 1 line

Jens Axboe

Martin Dalecki

2002-03-18 13:44 UTC #12 lore raw

In reply to Roberto Nibali

Quoted 8 lines · Jens Axboe

On Mon, Mar 18 2002, Martin Dalecki wrote:
[BLKRAGET etc]
Quoted 3 lines
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 :-).

Quoted 4 lines

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.

Jens Axboe

2002-03-18 13:51 UTC #13 lore raw

In reply to Roberto Nibali

Quoted 33 lines · Martin Dalecki

Quoted 8 lines · Jens Axboe
On Mon, Mar 18 2002, Martin Dalecki wrote:
[BLKRAGET etc]
Quoted 3 lines
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 :-).
Quoted 4 lines
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;

Quoted 6 lines

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.

Signature · 1 line

Jens Axboe