Message ID | 1274375150-15275-1-git-send-email-jlayton@redhat.com |
---|---|
State | New |
Headers | show |
Are we certain of this for all server types (NetApp, EMC, Samba etc.)? On Thu, May 20, 2010 at 12:05 PM, Jeff Layton <jlayton@redhat.com> wrote: > Busy-file renames don't actually work across directories, so we need > to limit this code to renames within the same dir. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/inode.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > index 62b324f..6f0683c 100644 > --- a/fs/cifs/inode.c > +++ b/fs/cifs/inode.c > @@ -1401,6 +1401,10 @@ cifs_do_rename(int xid, struct dentry *from_dentry, const char *fromPath, > if (rc == 0 || rc != -ETXTBSY) > return rc; > > + /* open-file renames don't work across directories */ > + if (to_dentry->d_parent != from_dentry->d_parent) > + return rc; > + > /* open the file to be renamed -- we need DELETE perms */ > rc = CIFSSMBOpen(xid, pTcon, fromPath, FILE_OPEN, DELETE, > CREATE_NOT_DIR, &srcfid, &oplock, NULL, > -- > 1.6.6.1 > >
On Thu, 20 May 2010 12:15:37 -0500 Steve French <smfrench@gmail.com> wrote: > Are we certain of this for all server types (NetApp, EMC, Samba etc.)? > This is definitely the case with windows. The open-file rename call only accepts simple filenames (no paths). > On Thu, May 20, 2010 at 12:05 PM, Jeff Layton <jlayton@redhat.com> wrote: > > Busy-file renames don't actually work across directories, so we need > > to limit this code to renames within the same dir. > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/inode.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index 62b324f..6f0683c 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -1401,6 +1401,10 @@ cifs_do_rename(int xid, struct dentry *from_dentry, const char *fromPath, > > if (rc == 0 || rc != -ETXTBSY) > > return rc; > > > > + /* open-file renames don't work across directories */ > > + if (to_dentry->d_parent != from_dentry->d_parent) > > + return rc; > > + > > /* open the file to be renamed -- we need DELETE perms */ > > rc = CIFSSMBOpen(xid, pTcon, fromPath, FILE_OPEN, DELETE, > > CREATE_NOT_DIR, &srcfid, &oplock, NULL, > > -- > > 1.6.6.1 > > > > > > > > -- > Thanks, > > Steve
On Thu, May 20, 2010 at 12:23 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Thu, 20 May 2010 12:15:37 -0500 > Steve French <smfrench@gmail.com> wrote: > >> Are we certain of this for all server types (NetApp, EMC, Samba etc.)? >> > > This is definitely the case with windows. The open-file rename call > only accepts simple filenames (no paths). The restriction on rename always seemed to me just a Windows limitation not a "should" or "must" - we should check what MS-CIFS says
On Fri, 21 May 2010 11:37:25 -0500 Steve French <smfrench@gmail.com> wrote: > On Thu, May 20, 2010 at 12:23 PM, Jeff Layton <jlayton@redhat.com> wrote: > > On Thu, 20 May 2010 12:15:37 -0500 > > Steve French <smfrench@gmail.com> wrote: > > > >> Are we certain of this for all server types (NetApp, EMC, Samba etc.)? > >> > > > > This is definitely the case with windows. The open-file rename call > > only accepts simple filenames (no paths). > > The restriction on rename always seemed to me just a Windows > limitation not a "should" or "must" - we should check what MS-CIFS says > > > MS-CIFS doesn't cover this. This is a passthrough infolevel. MS-FSCC says this: ---------------[snip]-------------------- RootDirectory (4 bytes): A 32-bit unsigned integer that contains the file handle for the root directory. For network operations, this value MUST always be zero. FileNameLength (4 bytes): A 32-bit unsigned integer that specifies the length, in bytes, of the file name contained within the FileName member. FileName (variable): A sequence of Unicode characters containing the file name. When working with this field, use FileNameLength to determine the length of the file name rather than assuming the presence of a trailing null delimiter. If the RootDirectory member is 0, this member MUST specify a full pathname to be assigned to the file. For network operations, this pathname is relative to the root of the share. If the RootDirectory member is not 0, this member MUST specify a pathname, relative to RootDirectory, for the new name of the file. ---------------[snip]-------------------- ...which makes it look like renaming an open file across directories ought to work. However...experimentation has shown that this isn't actually correct. Anything other than a simple filename is rejected by all of the servers where this seems to matter (win2k3 is where I mainly tested this, but earlier NT-based servers worked the same way). This is the reason that the call in cifs_do_rename does this: rc = CIFSSMBRenameOpenFile(xid, pTcon, srcfid, (const char *) to_dentry->d_name.name, cifs_sb->local_nls, cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR); ...note that it uses 'd_name.name' instead of toName. Earlier efforts to use full paths for the target proved problematic. Now, we can debate what the spec says but the fact of the matter is that the proposed patch is quite correct for the code as it exists today and fixes an actual bug: https://bugzilla.redhat.com/show_bug.cgi?id=591938 I suggest that we take the patch as-is to fix that problem. If you want to do the legwork to try to expand this such that cross-directory busy-file renames work, then I'll be happy to help review it.
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 62b324f..6f0683c 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -1401,6 +1401,10 @@ cifs_do_rename(int xid, struct dentry *from_dentry, const char *fromPath, if (rc == 0 || rc != -ETXTBSY) return rc; + /* open-file renames don't work across directories */ + if (to_dentry->d_parent != from_dentry->d_parent) + return rc; + /* open the file to be renamed -- we need DELETE perms */ rc = CIFSSMBOpen(xid, pTcon, fromPath, FILE_OPEN, DELETE, CREATE_NOT_DIR, &srcfid, &oplock, NULL,
Busy-file renames don't actually work across directories, so we need to limit this code to renames within the same dir. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/inode.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)