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. 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. 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? > > 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? > > -- Regards, Shyam
