On Wed, Aug 7, 2019 at 1:19 AM Tom Talpey <ttalpey@xxxxxxxxxxxxx> wrote: > > > -----Original Message----- > > From: linux-cifs-owner@xxxxxxxxxxxxxxx <linux-cifs-owner@xxxxxxxxxxxxxxx> On > > Behalf Of Ronnie Sahlberg > > Sent: Monday, August 5, 2019 6:47 PM > > To: linux-cifs <linux-cifs@xxxxxxxxxxxxxxx> > > Cc: Steve French <smfrench@xxxxxxxxx>; Ronnie Sahlberg > > <lsahlber@xxxxxxxxxx> > > Subject: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ > > mounts > > > > Normally in smb you can not rename ontop of a destination that is held open > > except in SMB1 where this is allowed IFF the delete-on-close flag is also set. > > The FILE_DELETE_ON_CLOSE flag doesn't really have any significance in the > protocol, it is passed to the underlying filesystem except in one special case > where the server validates that the DELETE or GENERIC is granted on the > share. Did you find some reference in the documents to the contrary? > No but emperical testing, and xfstests, show it. It does not just affect rename() but the same thing happens with "create and replace an existing file". There is definitely a conditional somewhere in the SMB server that affects the behaviour or the delete-on-close flag. This particular scenario is for the behaviour when we try to rename onto an existing file that have open handles. The process used in cifs.ko is : 1, Try to rename the file. This fails with ACCESS_DENIED if there are open handles if this fails, then fallback to 2, set delete-on-close 3, try the rename again In SMB1, this works. In step 3 the file is renamed EVEN if there are still open handles to it. In SMB2+ this fails. If there are open handles then this fails with DELETE_PENDING and the rename will not happen. (then sometime later when the other process closes its handle then the file is deleted) This still happens in windows 2016. Delete-on-close and other similar unlink() related operations behave differently between SMB1 and SMB2. > In other words, I think the fix may be ok, but the server->vals->protocol_id != 0 > conditional may need more thought. > > Tom. > > > This special case is not supported in SMB2 so should not attempt the unlink- > > and-try-again > > since the rename will still fail but we now also delete the destination file. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> > > --- > > fs/cifs/inode.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index 56ca4b8ccaba..fdea45267a39 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -1777,6 +1777,7 @@ cifs_rename2(struct inode *source_dir, struct dentry > > *source_dentry, > > FILE_UNIX_BASIC_INFO *info_buf_target; > > unsigned int xid; > > int rc, tmprc; > > + struct TCP_Server_Info *server; > > > > if (flags & ~RENAME_NOREPLACE) > > return -EINVAL; > > @@ -1786,6 +1787,7 @@ cifs_rename2(struct inode *source_dir, struct dentry > > *source_dentry, > > if (IS_ERR(tlink)) > > return PTR_ERR(tlink); > > tcon = tlink_tcon(tlink); > > + server = tcon->ses->server; > > > > xid = get_xid(); > > > > @@ -1809,6 +1811,14 @@ cifs_rename2(struct inode *source_dir, struct > > dentry *source_dentry, > > to_name); > > > > /* > > + * Do not attempt unlink-then-try-rename-again for SMB2+. > > + * Renaming ontop of an existing open file IF the delete-on-close > > + * flag is set is only supported for SMB1. > > + */ > > + if (rc == -EACCES && server->vals->protocol_id != 0) > > + goto cifs_rename_exit; > > + > > + /* > > * No-replace is the natural behavior for CIFS, so skip unlink hacks. > > */ > > if (flags & RENAME_NOREPLACE) > > -- > > 2.13.6 > > This special case is not supported in SMB2 so should not attempt the unlink- > > and-try-again > > since the rename will still fail but we now also delete the destination file. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> > > --- > > fs/cifs/inode.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c > > index 56ca4b8ccaba..fdea45267a39 100644 > > --- a/fs/cifs/inode.c > > +++ b/fs/cifs/inode.c > > @@ -1777,6 +1777,7 @@ cifs_rename2(struct inode *source_dir, struct dentry > > *source_dentry, > > FILE_UNIX_BASIC_INFO *info_buf_target; > > unsigned int xid; > > int rc, tmprc; > > + struct TCP_Server_Info *server; > > > > if (flags & ~RENAME_NOREPLACE) > > return -EINVAL; > > @@ -1786,6 +1787,7 @@ cifs_rename2(struct inode *source_dir, struct dentry > > *source_dentry, > > if (IS_ERR(tlink)) > > return PTR_ERR(tlink); > > tcon = tlink_tcon(tlink); > > + server = tcon->ses->server; > > > > xid = get_xid(); > > > > @@ -1809,6 +1811,14 @@ cifs_rename2(struct inode *source_dir, struct > > dentry *source_dentry, > > to_name); > > > > /* > > + * Do not attempt unlink-then-try-rename-again for SMB2+. > > + * Renaming ontop of an existing open file IF the delete-on-close > > + * flag is set is only supported for SMB1. > > + */ > > + if (rc == -EACCES && server->vals->protocol_id != 0) > > + goto cifs_rename_exit; > > + > > + /* > > * No-replace is the natural behavior for CIFS, so skip unlink hacks. > > */ > > if (flags & RENAME_NOREPLACE) > > -- > > 2.13.6
