Re: hardlink patch series

On Thu, 25 Jun 2009 17:05:12 -0500
Steve French <[email protected]> wrote:

> On Thu, Jun 25, 2009 at 4:43 PM, Jeff Layton<[email protected]> wrote:
> > On Thu, 25 Jun 2009 15:55:09 -0500
> > Steve French <[email protected]> wrote:
> >
> >> I have been reviewing Jeff's series of (large) patches for hardlink
> >> fixup.   The only problem I have noticed so far is the rename error
> >> path - we have cached inode information which could have stale inode
> >> numbers for source and target (they could be up to 1 second old) so we
> >> may incorrectly think that source and target are different inodes in
> >> the EEXIST case (Jeff removed the logic which checks if source/target
> >> inode numbers are the same on EEXIST).  It may not matter to current
> >> Samba (more recent POSIX extensions) but still trying to check that
> >> ...
> > I don't know that there's much we can do about that. This sort of thing
> > is always racy with a netfs like CIFS or NFS. The VFS does do a lookup
> > prior to calling cifs_rename. One thing we could do is force a
> > revalidation when LOOKUP_RENAME_TARGET is set (for the target dentry),
> > but even that won't be sufficient to eliminate races 100%, and that
> > doesn't help the case where the inode number of the source has changed.
> >
> > IMO, that's not a case to worry overly much about. I doubt it happens
> > much in practice and we can consider how best to clean it up later.
> 
> The case of source and target recently hardlinked (even if revalidate
> is called prior to rename, revalidate is a noop when info is cached
> for less than a second) makes sense to continue to check since it is
> possible to current Samba. 

Ok so specifically, you're worried about:

We do lookups of source and target and find they are two different
inodes. Then within the 1s that lookups are cached, one of the dentries
is removed and turned into a hardlink to the other one.

Sounds plausible, I guess. Though it's an awful lot of code to keep
around for something that's unlikely. That also does nothing for the
opposite case where 2 dentries were hardlinks and then become 2 separate
inodes after the lookup.

My personal opinion would be to remove it and then worry about it if it
ever becomes a problem.

>  Since it only costs us an extra lookup
> when we get an error it doesn't hurt performance to continue to do the
> check - an easier way might be to simply force revalidate (set the
> cifs inode time to zero).
> 
> It would be nice if Samba (at least) didn't do this, but it is
> certainly possible for an app to do a create, hardlink and rename in 1
> second, and I think it makes sense to continue to check for that.
> 

It certainly sounds possible with windows servers too, however the
code doesn't do anything to handle non-posix mounts. Is it not a
problem there for some reason?

-- 
Jeff Layton <[email protected]>