| Message ID | 20240419150507.554196-1-pc@manguebit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: client: fix rename(2) regression against samba | expand |
I merged this into cifs-2.6.git for-next (pending more testing) but
wasn't able to reproduce
the problem. Would be useful to know if others could repro the problem
without the patch
(and verify that the patch fixes the problem). Testing it now e.g.:
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/5/builds/94
Also would like any feedback/testing/comments/opinions on David Howell's patch:
cifs: Fix reacquisition of volume cookie on still-live connection
On Fri, Apr 19, 2024 at 10:05 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> After commit 2c7d399e551c ("smb: client: reuse file lease key in
> compound operations") the client started reusing lease keys for
> rename, unlink and set path size operations to prevent it from
> breaking its own leases and thus causing unnecessary lease breaks to
> same connection.
>
> The implementation relies on positive dentries and
> cifsInodeInfo::lease_granted to decide whether reusing lease keys for
> the compound requests. cifsInodeInfo::lease_granted was introduced by
> commit 0ab95c2510b6 ("Defer close only when lease is enabled.") to
> indicate whether lease caching is granted for a specific file, but
> that can only happen until file is open, so
> cifsInodeInfo::lease_granted was left uninitialised in ->alloc_inode
> and then client started sending random lease keys for files that
> hadn't any leases.
>
> This fixes the following test case against samba:
>
> mount.cifs //srv/share /mnt/1 -o ...,nosharesock
> mount.cifs //srv/share /mnt/2 -o ...,nosharesock
> touch /mnt/1/foo; tail -f /mnt/1/foo & pid=$!
> mv /mnt/2/foo /mnt/2/bar # fails with -EIO
> kill $pid
>
> Fixes: 0ab95c2510b6 ("Defer close only when lease is enabled.")
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> ---
> fs/smb/client/cifsfs.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index d41eedbff674..8907bbcd9f96 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -389,6 +389,7 @@ cifs_alloc_inode(struct super_block *sb)
> * server, can not assume caching of file data or metadata.
> */
> cifs_set_oplock_level(cifs_inode, 0);
> + cifs_inode->lease_granted = false;
> cifs_inode->flags = 0;
> spin_lock_init(&cifs_inode->writers_lock);
> cifs_inode->writers = 0;
> --
> 2.44.0
>
diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c index d41eedbff674..8907bbcd9f96 100644 --- a/fs/smb/client/cifsfs.c +++ b/fs/smb/client/cifsfs.c @@ -389,6 +389,7 @@ cifs_alloc_inode(struct super_block *sb) * server, can not assume caching of file data or metadata. */ cifs_set_oplock_level(cifs_inode, 0); + cifs_inode->lease_granted = false; cifs_inode->flags = 0; spin_lock_init(&cifs_inode->writers_lock); cifs_inode->writers = 0;
After commit 2c7d399e551c ("smb: client: reuse file lease key in compound operations") the client started reusing lease keys for rename, unlink and set path size operations to prevent it from breaking its own leases and thus causing unnecessary lease breaks to same connection. The implementation relies on positive dentries and cifsInodeInfo::lease_granted to decide whether reusing lease keys for the compound requests. cifsInodeInfo::lease_granted was introduced by commit 0ab95c2510b6 ("Defer close only when lease is enabled.") to indicate whether lease caching is granted for a specific file, but that can only happen until file is open, so cifsInodeInfo::lease_granted was left uninitialised in ->alloc_inode and then client started sending random lease keys for files that hadn't any leases. This fixes the following test case against samba: mount.cifs //srv/share /mnt/1 -o ...,nosharesock mount.cifs //srv/share /mnt/2 -o ...,nosharesock touch /mnt/1/foo; tail -f /mnt/1/foo & pid=$! mv /mnt/2/foo /mnt/2/bar # fails with -EIO kill $pid Fixes: 0ab95c2510b6 ("Defer close only when lease is enabled.") Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.com> --- fs/smb/client/cifsfs.c | 1 + 1 file changed, 1 insertion(+)