lkml.org 
[lkml]   [2010]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] d_ino considered harmful
    On Thu, Jun 17, 2010 at 06:54:29PM +0100, Jamie Lokier wrote:
    > Valerie Aurora wrote:
    > >> Who needs d_ino anyway? I am running a kernel with this patch -
    > >> Gnome, a browser, IRC, kernel compile, etc. and everything works.
    > > I'm running a kernel with the below patch and everything still works.
    > > Apparently "ls -i" is still using the bogus d_ino performance
    > > improvement mentioned here because it returns all 1's for inode
    > > number.
    >
    > I'm surprised at "ls -i", as a patch to change that has been submitted:
    >
    > http://marc.info/?l=linux-kernel&m=125181054102075
    > http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/17887

    I was surprised too. I guess people still want to optimize ls -i,
    even at the cost of wrong results.

    > > Use of d_ino without the corresponding st_dev is always buggy in the
    > > presence of submounts, bind mounts, and union mounts. E.g., the d_ino
    > > of a mountpoint will be the inode number of the directory under the
    > > mountpoint, not the mounted directory.
    >
    > It's not surprising everything seems to work.
    >
    > It can be useful as a performance hint, which you probably didn't test.

    I'm afraid I wasn't entirely serious with that patch. :) But it was an
    interesting exercise.

    > I strongly disagree that correct code must call stat(). Correct code
    > can check against the list of mountpoints in /proc/mounts, because it
    > is strictly only mountpoints where the number doesn't agree with
    > stat() -- prior to your patch :-)

    If you are assuming that the application is parsing /proc/mounts (does
    anyone actually do this?), then the application can also learn about
    union mounts and not trust d_ino in any directory below the union
    mount point. :)

    > Anyway, maybe your patch is not allowed by POSIX :-) as follows
    > (posted to linux-kernel some time ago):
    >
    > http://marc.info/?l=linux-kernel&m=125181054102075
    > http://www.gossamer-threads.com/lists/linux/kernel/1124140
    >
    > The POSIX readdir spec says this:
    >
    > The structure dirent defined in the <dirent.h> header describes a
    > directory entry. The value of the structure's d_ino member shall be set
    > to the file serial number of the file named by the d_name member.
    >
    > The description for sys/stat.h makes the connection between
    > "file serial number" and the stat.st_ino member:
    >
    > The <sys/stat.h> header shall define the stat structure, which shall
    > include at least the following members:
    > ...
    > ino_t st_ino File serial number.
    >
    > Returning the covered inode's number at a mountpoint is apparently not
    > POSIX compliant either, but is widespread. (I.e. all unixes except
    > Cygwin apparently.)
    >
    > > Gosh, maybe it would help to patch the currently used readdir instead
    > > of just old_readdir() (thanks, Arnd). And return 1 instead of 0 so ls
    > > doesn't think all files are deleted (thanks, Andreas).
    >
    > It's not just ls. Bash 3.0 ignores entries for completion if d_ino == 0.
    >
    > > I'm running a kernel with the below patch and everything still works.
    > > Apparently "ls -i" is still using the bogus d_ino performance
    > > improvement mentioned here because it returns all 1's for inode
    > > number.
    > >
    > > http://www.mail-archive.com/bug-findutils@gnu.org/msg02531.html
    >
    > I'm intrigued by the mentioned in that report that Linux bind mounts
    > return the covering inode number in d_ino, not the covered inode number.
    >
    > If true, that means mounts are already being checked when returning d_ino,
    > and suggests that doing it for all mounts isn't expensive.

    This surprises me too. I will check into it further.

    -VAL


    \
     
     \ /
      Last update: 2010-06-17 21:13    [W:0.028 / U:0.624 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site