On 12/8/2023 5:58 AM, Shyam Prasad N wrote:
On Wed, Dec 6, 2023 at 7:12 PM Tom Talpey <tom@xxxxxxxxxx> wrote:
On 12/6/2023 12:31 AM, Shyam Prasad N wrote:
On Wed, Dec 6, 2023 at 1:02 AM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
meetakshisetiyaoss@xxxxxxxxx writes:
From: Meetakshi Setiya <msetiya@xxxxxxxxxxxxx>
Lock contention during unlink operation causes cifs lease break ack
worker thread to block and delay sending lease break acks to server.
This case occurs when multiple threads perform unlink, write and lease
break acks on the same file. Thhis patch fixes the problem by reusing
the existing lease keys for rename, unlink and set path size compound
operations so that the client does not break its own lease.
Signed-off-by: Meetakshi Setiya <msetiya@xxxxxxxxxxxxx>
---
fs/smb/client/cifsglob.h | 6 ++---
fs/smb/client/cifsproto.h | 8 +++----
fs/smb/client/cifssmb.c | 6 ++---
fs/smb/client/inode.c | 12 +++++-----
fs/smb/client/smb2inode.c | 49 +++++++++++++++++++++++++--------------
fs/smb/client/smb2proto.h | 8 +++----
6 files changed, 51 insertions(+), 38 deletions(-)
NAK. This patch broke some xfstests.
Consider this reproducer:
$ cat repro.sh
#!/bin/sh
umount /mnt/1 &>/dev/null
mount.cifs //srv/share /mnt/1 -o ...,vers=3.1.1
rm /mnt/1/* &>/dev/null
pushd /mnt/1 >/dev/null
touch foo
ln -v foo bar
rm -v bar
popd >/dev/null
umount /mnt/1 &>/dev/null
$ ./repro.sh
'bar' => 'foo'
rm: cannot remove 'bar': Invalid argument
This is what going on
- client creates 'foo' with RHW lease granted.
- client creates hardlink file 'bar'.
At this point, we have two positive dentries (foo & bar) which share
same inode.
- The client then attempts to remove 'bar' by re-using lease key from
'foo' through compound request CREATE(DELETE)+CLOSE, which fails with
STATUS_INVALID_PARAMETER.
That's interesting. I'm assuming that the STATUS_INVALID_PARAMETER is
due to the server not recognizing the lease id for the file bar.
I'm not sure that this is a client bug.
If the server supports hard links, then should it not be aware that
foo and bar are the same files? AFAIK, file lease is associated with a
file, and not the dentry.
Lease keys are tied to the _filename_, including the full path. They
are basically guid's, and are used as lookup keys in the lease list,
from which the lease itself is the resulting value. The value is then
inspected for a match to the filename for which it was created.
They are not about the _handle_, which is what is apparently being
assumed here.
MS-SMB2 section 3.3.5.9.8 says this, which the server in question is
correctly enforcing [emphasis added]:
The server MUST attempt to locate a Lease by performing a lookup in the LeaseTable.LeaseList
using the LeaseKey in the SMB2_CREATE_REQUEST_LEASE as the lookup key. If a lease is found,
Lease.FileDeleteOnClose is FALSE, and *Lease.Filename does not match the file name* for the
incoming request, the request MUST be failed with STATUS_INVALID_PARAMETER
IOW, hard links are, from a protocol leasing standpoint, not the
same "file".
This is interesting. I would assume that leases are a mechanism that
assure client that it is free to cache the file locally, and that the
server would inform the client when that is not safe anymore.
MS-SMB2 has this (non-normative) statement in section 3.2.4.3:
If the client accesses a file through multiple paths, such as using
different server names or share names or parent directory names, it
will create multiple File elements, and therefore multiple
File.LeaseKeys for the same remote file. This loses the performance
benefits of sharing cache state across all Opens of the same file and
can cause additional lease breaks to be generated, as actions by a
client through one path will affect caching by that client through
other paths. However, the impact is a matter of performance; cache
correctness is preserved. If the client accesses the same path
across multiple opens, the client will use the same File element and
therefore the same File.LeaseKey is used.
So, this next statement makes an incorrect assumption:
Hard links are in-fact pointing to the same file. So I would've
assumed that the server would have treated both links to have the same
lease.
Not at all - the lease key is chosen by the *client*, not the server,
and the key can only look up one actual lease. So the protocol enforces
it, meaning the server must verify the path.
So either the server should share leases between hard links.
If not, at least an existing RWH lease on link1 would be broken by RWH
lease requested by another client on link2. At least for the Windows
file server, that does not happen either.
Isn't this a bug from the correctness stand point?
Can you provide traces on that? If the protocol document is incorrect,
or the Windows implementation does not conform to it, that's bad.
Tom.
Tom.
Meetakshi, can you please follow the repro steps provided by Paulo
(against Windows file server) and check why the invalid parameter
error is being returned?
