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

[Jeff Layton]

On Tue, 23 Sep 2008 07:07:23 -0400
Jeff Layton <jlayton at redhat.com> wrote:

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

I've respun and rebased the for-steve branch in my git tree based on
the comments you sent last night. Please have a look at the set of
patches there and let me know if there are other changes that need to
be made. 

I've also added a new patch for cifs_rename to have it handle -EACCES
errors. This fixes Wilhelm's testcase for the rename problems, and
should also make it so that we "undo" correctly in case of errors.

This is only lightly tested, so feedback is welcome...

PS: since I rebased the for-steve branch, you won't be able to just
pull it on top of an earlier pull...

New pull request follows:

--------------[snip]---------------

The following changes since commit 2846d3864738dd6e290755d0692cf377e09ba79f:
  Jeff Layton (1):
        cifs: have find_writeable_file prefer filehandles opened by same task

are available in the git repository at:

  git://git.samba.org/jlayton/cifs.git for-steve

Jeff Layton (12):
      cifs: explicitly revoke SPNEGO key after session setup
      cifs: no need to use rcu_assign_pointer on immutable keys
      cifs: clean up upcall handling for dns_resolver keys
      cifs: add constants for string lengths of keynames in SPNEGO upcall string
      cifs: remove NULL termination from rename target in CIFSSMBRenameOpenFIle
      cifs: fix busy-file renames and refactor cifs_rename logic
      cifs: move rename and delete-on-close logic into helper function
      cifs: add function to set file disposition
      cifs: undo changes in cifs_rename_pending_delete if it errors out
      cifs: clean up error handling in cifs_unlink
      cifs: update DOS attributes in cifsInode if we successfully changed them
      cifs: make cifs_rename handle -EACCES errors

 fs/cifs/cifs_spnego.c |   37 ++++--
 fs/cifs/cifsproto.h   |    4 +-
 fs/cifs/cifssmb.c     |   59 +++++++-
 fs/cifs/dns_resolve.c |   76 +++++----
 fs/cifs/inode.c       |  402 ++++++++++++++++++++++++++++++++-----------------
 fs/cifs/sess.c        |    4 +-
 6 files changed, 393 insertions(+), 189 deletions(-)

-- 
Jeff Layton <jlayton at redhat.com>