Re: another IDE cleanup: kill duplicated code

Martin Dalecki
2002-02-12T13:18:20Z

Vojtech Pavlik wrote:

>On Tue, Feb 12, 2002 at 01:45:10PM +0100, Martin Dalecki wrote:
>
>>>>The first does control an array of values, which doesn't make sense in
>>>>first place. I.e. changing it doesn't
>>>>change ANY behaviour of the kernel.
>>>>
>
>>>Actually HFS uses it ...
>>>
>
>>Please note that the use in HFS is inappriopriate. It's supposed to
>>optimize the read throughput there, which is something that shouldn't
>>be done at the fs setup layer at all. The usage of read_ahead there
>>can be just removed and everything should be fine (modulo the fact
>>that in current state the HFS filesystem code is just non-working
>>obsolete code garbage anyway ;-).
>>
>
>Yes; I just commented on that it does change some behavior of the
>kernel.
>
>>>>The second of them is trying to control a file-system level constant
>>>>inside the actual block device driver. This is a blatant violation of
>>>>the layering principle in software design, and should go as soon as
>>>>possible.
>>>>
>
>>>Yes. But still block device drivers allocate the array ...
>>>
>
>>Well if we do:
>>
>>find ./ -name "*.c" -exec grep max_readahead /dev/null {} \;
>>
>>One can already easly see that apart from ide, md, and acorn drivers
>>this has been already abstracted away one level up at the block device
>>handling as well. My suspiction is that there is now already *double*
>>initialization of the sub-slices of this array there. Anyway ide
>>should be adapted here to the same way as it's done now for scsi.
>>
>>Will you look after this or should me? (I can't until afternoon,
>>becouse I'm at my "true live" job now and have other things to do...)
>>
>
>Am I understanding you correctly that we can just remove the
>max_readahead references in IDE (and possibly also md and acorn)?
>

Yes they can certailny be killed there. However please note that I'm not
quite sure whatever some other
adjustments may be needed. I suggest to double check with the way the
scsi driver middle layer
deals with them.

OK. I can't resist. Let's have a closer look together:

~/tmp/trans/linux-2.5.4# find ./ -name "*.[ch]" -exec grep max_readahead
/dev/null {} \;
./drivers/acorn/block/mfmhd.c: max_readahead[major][minor] = arg;
./drivers/acorn/block/mfmhd.c: return
put_user(max_readahead[major][minor], (long *) arg);

1. ^^^^ Here apparently no additional allocation is needed.

./drivers/block/blkpg.c: if (!(iptr =
max_readahead[major(dev)]))
./drivers/block/blkpg.c: if (!(iptr =
max_readahead[major(dev)]))

2. ^^^^ This is just read out to the user space through ioctl()..

./drivers/block/ll_rw_blk.c:int * max_readahead[MAX_BLKDEV];
./drivers/block/ll_rw_blk.c: memset(max_readahead, 0,
sizeof(max_readahead));

3. ^^^^ This is just initialisation.

./drivers/ide/ide-probe.c: max_readahead[hwif->major] = max_ra;
./drivers/ide/ide-disk.c: ide_add_setting(drive, "fi ....
1024, &max_readahead[major][minor], NULL);
./drivers/ide/ide-floppy.c: ide_add_setting(drive, "....,
&max_readahead[major][minor], NULL);
./drivers/ide/ide-cd.c: ide_add_setting(drive, "file_readahead",. ... ,
&max_readahead[major][minor], NULL);
./drivers/ide/ide.c: kfree(max_readahead[hwif->major]);
./drivers/ide/ataraid.c: max_readahead[ATAMAJOR] = NULL;

4. ^^^^ Still just initialization.

./drivers/md/md.c: max_readahead[MAJOR_NR] = md_maxreadahead;
./include/linux/blkdev.h:extern int * max_readahead[MAX_BLKDEV];
./include/linux/blkdev.h: max_readahead[major] = NULL;

5. Still no true usage.

./mm/filemap.c:static inline int get_max_readahead(struct inode * inode)
./mm/filemap.c: if (kdev_none(inode->i_dev) ||
!max_readahead[major(inode->i_dev)])
./mm/filemap.c: return
max_readahead[major(inode->i_dev)][minor(inode->i_dev)];
./mm/filemap.c: int max_readahead = get_max_readahead(inode);
./mm/filemap.c: if (filp->f_ramax > max_readahead)
./mm/filemap.c: filp->f_ramax = max_readahead;
./mm/filemap.c: int max_readahead = get_max_readahead(inode);
./mm/filemap.c: if (filp->f_ramax > max_readahead)
./mm/filemap.c: filp->f_ramax = max_readahead;
./mm/filemap.c: ra_window =
get_max_readahead(vma->vm_file->f_dentry->d_inode);

6. Hmm I wonder how the above is working in case somebody didn't care
about this array.

Let's have a closer look at it there (mm/filemap.c).

If max_readahead array is not set at all, the filemap code just assumes
a constant
MAX_READAHEAD value. max_readahead is an upper bound value for the number
for consecutive file sectors read at once.

static inline int get_max_readahead(struct inode * inode)
{
if (kdev_none(inode->i_dev) || !max_readahead[major(inode->i_dev)])
return MAX_READAHEAD;
return max_readahead[major(inode->i_dev)][minor(inode->i_dev)];
}

Other then this we have the following code in nopage_sequention_readahead:

ra_window = get_max_readahead(vma->vm_file->f_dentry->d_inode);
ra_window = CLUSTER_OFFSET(ra_window + CLUSTER_PAGES - 1);

Obviously the first file of the two can be just killed without any harm.

So the conclusions is that not just the read_ahead array is bogous now.
The max_readahead array can be killed entierly from the kernel as well ;-).

The answer is: I'm now confident that you can just remove all the
max_readahead initialization from the ide code.