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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site