patch reverted
On Tue, Jul 21, 2020 at 8:52 AM zhangxiaoxu (A) <zhangxiaoxu5@xxxxxxxxxx> wrote:
>
> Thanks for your report.
>
> Since commit 9ffad9263b46 ("cifs: Fix the target file was deleted
> when rename failed.") want to fix the problem when rename(file1, file2)
> with file2 exist, the server return -EACESS, then cifs client will
> delete the file2 and rename it again, but 2nd rename on server also
> return -EACESS, then the file2 was deleted.
>
> It can be reproduce by xfstests generic/035.
> When 't_rename_overwrite file1 file2':
> open(file2)
> rename(file1, file2)
> fstat(file2).st_nlink should be 0.
>
> The solution on commit 9ffad9263b46 ("cifs: Fix the target file was
> deleted when rename failed.") was wrong. we should revert it.
>
> The root cause of the file2 deleted maybe the file2 was opened
> when rename(file1, file2), I will re-debug it.
>
> 在 2020/7/21 1:09, Patrick Fernie 写道:
> > # One line summary of the problem:
> >
> > mv command fails: "File exists" on cifs mount on kernel>=5.7.8
> >
> > # Full description of the problem/report:
> >
> > Since v5.7.8 (v5.4.51 for -lts), there appears to be a regression with
> > cifs mounts; mv commands fail with a "File exists" when attempting to
> > overwrite a file. Similarly, rsync commands which create a temporary
> > file during transfer and then attempt to move it into place after
> > copying fail ("File Exists (17)"). I believe this is related to this
> > commit: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/fs/cifs/inode.c?id=9ffad9263b467efd8f8dc7ae1941a0a655a2bab2
> >
> > The mount in question is from a Drobo FS NAS device, and is forced to
> > SMB1 (`vers=1.0` specified).
> >
> > Running v5.7.7 or 5.4.50 does not exhibit this behavior, behavior was
> > confirmed on 5.7.8, 5.7.9, 5.4.51 and 5.4.52.
> >
> > These users appear to be experiencing the same issue:
> > 1) https://serverfault.com/questions/1025734/cifs-automounts-suddenly-stopped-working
> > 2) https://unix.stackexchange.com/questions/599281/cifs-mount-is-returning-errors-when-operating-on-remote-files-file-exists-inte
> >
> > # Most recent kernel version which did not have the bug:
> >
> > 5.7.7 / 5.4.50
> >
> > # A small shell script or example program which triggers the problem
> > (if possible):
> >
> > [vagrant@archlinux ~]$ uname -a
> > Linux archlinux 5.7.9-arch1-1 #1 SMP PREEMPT Thu, 16 Jul 2020 19:34:49
> > +0000 x86_64 GNU/Linux
> > # Same behavior seen on Linux archlinux 5.4.52-1-lts #1 SMP Thu, 16
> > Jul 2020 19:35:06 +0000 x86_64 GNU/Linux
> > [vagrant@archlinux ~]$ cd /mnt/drobo/Share/cifs-test/
> > [vagrant@archlinux cifs-test]$ touch a b
> > [vagrant@archlinux cifs-test]$ mv a b
> > mv: cannot move 'a' to 'b': File exists
> > [vagrant@archlinux cifs-test]$ mkdir -p /tmp/sync_dir
> > [vagrant@archlinux cifs-test]$ touch /tmp/sync_dir/foo
> > [vagrant@archlinux cifs-test]$ rsync -rap /tmp/sync_dir .
> > [vagrant@archlinux cifs-test]$ touch /tmp/sync_dir/foo
> > [vagrant@archlinux cifs-test]$ rsync -rap /tmp/sync_dir .
> > rsync: [receiver] rename
> > "/mnt/drobo/Share/cifs-test/sync_dir/.foo.FQiit5" -> "sync_dir/foo":
> > File exists (17)
> > rsync error: some files/attrs were not transferred (see previous
> > errors) (code 23) at main.c(1287) [sender=3.2.2]
> >
> > ## Behavior as expected on older kernel:
> >
> > [vagrant@archlinux ~]$ uname -a
> > Linux archlinux 5.7.7-arch1-1 #1 SMP PREEMPT Wed, 01 Jul 2020 14:53:16
> > +0000 x86_64 GNU/Linux
> > # Same behavior seen on Linux archlinux 5.4.50-1-lts #1 SMP Wed, 01
> > Jul 2020 14:53:03 +0000 x86_64 GNU/Linux
> > [vagrant@archlinux ~]$ cd /mnt/drobo/Share/cifs-test/
> > [vagrant@archlinux cifs-test]$ touch a b
> > [vagrant@archlinux cifs-test]$ mv a b
> > [vagrant@archlinux cifs-test]$ mkdir -p /tmp/sync_dir
> > [vagrant@archlinux cifs-test]$ touch /tmp/sync_dir/foo
> > [vagrant@archlinux cifs-test]$ rsync -rap /tmp/sync_dir .
> > [vagrant@archlinux cifs-test]$ touch /tmp/sync_dir/foo
> > [vagrant@archlinux cifs-test]$ rsync -rap /tmp/sync_dir .
> > [vagrant@archlinux cifs-test]$
> >
> > # Environment:
> >
> > Arch Linux
> >
> > CIFS mount (vers=1.0) from Drobo FS NAS device
> >
> > CIFS share mount information:
> >
> > systemd-1 on /mnt/drobo/Share type autofs
> > (rw,relatime,fd=44,pgrp=1,timeout=0,minproto=5,maxproto=5,direct,pipe_ino=12139)
> > //10.76.9.11/Share on /mnt/drobo/Share type cifs
> > (rw,relatime,vers=1.0,cache=strict,username=XXXXXXX,uid=0,noforceuid,gid=0,noforcegid,addr=10.76.9.11,file_mode=0775,dir_mode=0775,nocase,soft,nounix,serverino,mapposix,nobrl,noperm,rsize=61440,wsize=65536,bsize=1048576,echo_interval=60,actimeo=1,x-systemd.automount)
> >
> > Regards,
> > Patrick Fernie
> >
> > .
> >
>
--
Thanks,
Steve
From fa1fd2d8eef89bb1696c64b81a911c1f7a12222d Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@xxxxxxxxxxxxx>
Date: Thu, 23 Jul 2020 14:41:29 -0500
Subject: [PATCH] Revert "cifs: Fix the target file was deleted when rename
failed."
This reverts commit 9ffad9263b467efd8f8dc7ae1941a0a655a2bab2.
Upon additional testing with older servers, it was found that
the original commit introduced a regression when using the old SMB1
dialect and rsyncing over an existing file.
The patch will need to be respun to address this, likely including
a larger refactoring of the SMB1 and SMB3 rename code paths to make
it less confusing and also to address some additional rename error
cases that SMB3 may be able to workaround.
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
Reported-by: Patrick Fernie <patrick.fernie@xxxxxxxxx>
CC: Stable <stable@xxxxxxxxxxxxxxx>
Acked-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
Acked-by: Zhang Xiaoxu <zhangxiaoxu5@xxxxxxxxxx>
---
fs/cifs/inode.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 49c3ea8aa845..ce95801e9b66 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2044,7 +2044,6 @@ cifs_rename2(struct inode *source_dir, struct dentry *source_dentry,
FILE_UNIX_BASIC_INFO *info_buf_target;
unsigned int xid;
int rc, tmprc;
- bool new_target = d_really_is_negative(target_dentry);
if (flags & ~RENAME_NOREPLACE)
return -EINVAL;
@@ -2121,13 +2120,8 @@ cifs_rename2(struct inode *source_dir, struct dentry *source_dentry,
*/
unlink_target:
- /*
- * If the target dentry was created during the rename, try
- * unlinking it if it's not negative
- */
- if (new_target &&
- d_really_is_positive(target_dentry) &&
- (rc == -EACCES || rc == -EEXIST)) {
+ /* Try unlinking the target dentry if it's not negative */
+ if (d_really_is_positive(target_dentry) && (rc == -EACCES || rc == -EEXIST)) {
if (d_is_dir(target_dentry))
tmprc = cifs_rmdir(target_dir, target_dentry);
else
--
2.25.1
