Messages in this thread | | | Date | Tue, 12 Feb 2002 14:17:48 +0100 | From | Martin Dalecki <> | Subject | Re: another IDE cleanup: kill duplicated code |
| |
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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |