[linux-cifs-client] [PATCH] cifs: when renaming don't try to unlink negative dentry

[Jeff Layton]

On Mon, 11 May 2009 04:55:04 -0400
Christoph Hellwig <hch at infradead.org> wrote:

> > +	/* Try unlinking the target dentry if it's not negative */
> > +	if (target_dentry->d_inode && (rc == -EACCES || rc == -EEXIST)) {
> 
> That comment is rather confusing to the reader.  A negative dentry
> is a Linux implementation detail.  What is really mean here is "delete
> the target if it exists".
> 
> Also cifs_rename for that case seems like it's not atomic as required
> by posix, as cifs_do_rename might fail after the target has been removed
> already.
> 
> Mail servers won't be very happy on cifs :)
> 

Yes, the cifs_rename codepaths are not atomic. There's unfortunately
not much we can do about that given the limitations of the protocol and
servers. When I initially did this code, I attempted to at least make
them back out changes when somethng failed, but it became a real mess.
Unfortunately, a lot of apps rely on being able to delete and rename
open files on top of one another so the alternative is that these apps
just fail.

One possibility is to consider changing this so that these operations
just fail on these servers, and add a mount option to allow "relaxed"
posix standards that enables the non-atomic renames (and maybe other
places where we bend the posix rules a bit).

Regardless though, the mount.cifs manpage should probably make mention
of this fact. Eventually I'd like to see a cifs(5) manpage, similar to
the nfs(5) one.

> Please put the BUG_ON in.  The current code is typical cargo-cult
> programming mess, and the newly added comment ontop of cifs_unlink
> is highly confusing.

Agreed. Steve's already committed a patch to make it "safe" to call
cifs_unlink on a negative dentry, but I'd prefer to see that reverted
and the BUG_ON in its place. 

-- 
Jeff Layton <jlayton at redhat.com>