lkml.org 
[lkml]   [2002]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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/

    \
     
     \ /
      Last update: 2005-03-22 13:24    [W:0.034 / U:60.724 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site