[linux-cifs-client] Re: [PATCH] cifs: fix busy-file renames and refactor cifs_rename ...

[Jeff Layton]

On Mon, 22 Sep 2008 18:50:39 -0500
"Steve French" <smfrench at gmail.com> wrote:

> A few comments about patch
> http://git.samba.org/?p=jlayton/cifs.git;a=commitdiff;h=3a611c1fed0879ba3b672e221b4a8fcb20d52ef2
> 
> 1) The check for from_dentry->d_parent != to_dentry->d_parent in
> cifs_do_rename seems unnecessary since it is not required that a
> server reject a rename by handle outside of the current directory
> although some MS servers do reject this.
> 
> 2) In cifs_do_rename, my preference would be to remove the goto and
> corresponding label (by reversing the if to "if (rc==0)
> CIFSSMBRenameOpenFile ,,,").  In general I prefer only using gotos
> when it simplifies the code
> 
> 3) by reordering the FreeXid statement at the beginning of cifs_rename
> you lose the logging of the function return code on a common case
> (EXDEV) when the debug flags are turned on in /proc/fs/cifs/
> 
> -       if (pTcon != cifs_sb_target->tcon) {
> -               FreeXid(xid);
> -               return -EXDEV;

I made the changes suggested above, but I seem to be getting different
results than when I had originally tested the patch. It's not clear to
me why since I don't think these changes caused it. I now get:

    rename: Permission denied

The server is returning NT_STATUS_ACCESS_DENIED when I try to rename
one open file on top of the other. I did an experiment and unlinked the
target before renaming. That fixes the problem but I assume we can
get NT_STATUS_ACCESS_DENIED for a lot of reasons. If we unlink the
target and then the rename fails, we can't easily reset things back to
their original state.

My thinking at this point is to do a RenameOpenFile on the target when
we get -EACCES back from an open-file rename attempt. We can then
move it back if the other rename fails...

-- 
Jeff Layton <jlayton at redhat.com>