hardlink patch series

[Steve French-2]

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
...

--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Thu, 25 Jun 2009 15:55:09 -0500
Steve French <smfrench@...> 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.

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Thu, Jun 25, 2009 at 4:43 PM, Jeff Layton<jlayton@...> wrote:

> On Thu, 25 Jun 2009 15:55:09 -0500
> Steve French <smfrench@...> 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.  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.

--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Thu, 25 Jun 2009 17:05:12 -0500
Steve French <smfrench@...> wrote:

> On Thu, Jun 25, 2009 at 4:43 PM, Jeff Layton<jlayton@...> wrote:
> > On Thu, 25 Jun 2009 15:55:09 -0500
> > Steve French <smfrench@...> 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 <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Thu, Jun 25, 2009 at 7:07 PM, Jeff Layton<jlayton@...> wrote:
> On Thu, 25 Jun 2009 17:05:12 -0500
> 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?

It is a problem to Windows servers too - and we talked about fixing this
at SNIA Plugfest as soon as we confirmed how safe it would be to use
their UniqueId field, but ended up running out of time to address
this since there were plenty of other server bugs to workaround
and other problems in the client to fix.


--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client