> -----Original Message----- > From: ronnie sahlberg <ronniesahlberg@xxxxxxxxx> > Sent: Tuesday, August 6, 2019 6:00 PM > To: Tom Talpey <ttalpey@xxxxxxxxxxxxx> > Cc: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>; linux-cifs <linux- > cifs@xxxxxxxxxxxxxxx>; Steve French <smfrench@xxxxxxxxx> > Subject: Re: [PATCH] cifs: do not attempt unlink-and-retry-rename for SMB2+ > mounts > > 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. Huh. I'm going to look into this on the Windows side. I wonder if something in the Linux client is falling back to the ancient SMB_COM_RENAME or SMB_COM_DELETE ops, which of course are path-based and exhibit different behaviors. The mystery is that FILE_DELETE_ON_CLOSE is not relevant to those. Anyway, thanks for the report. You wouldn't have a network trace of these differences, would you? Tom. > > 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
