Re: Different behavior of POSIX file locks depending on cache mode

[Kevin Ottens <kevin.ottens@enioka.com>]

Hello,

Is there anyone working on a patch for this since you narrowed it down a bit? 
We can probably test the changes if that's the case.

Regards.

On Tuesday 25 June 2024 09:41:48 CEST Kevin Ottens wrote:
> Hello,
> 
> On Tuesday 25 June 2024 08:00:11 CEST Andrew Bartlett wrote:
> > Thanks so much Steve.
> 
> Thanks indeed!
> 
> > Kevin can give more info, but no, my understanding is that this was
> > "always" problematic, the reason it came up now is LibreOffice changed
> > how it handled file saves, rather than a kernel change.
> 
> I confirm. It's been here for a long time AFAIK, but Libreoffice recently
> enabled some code path by default (it was previously disabled by default).
> 
> > Even if the fix is to be in the direction that 'breaks' LibreOffice,
> > that is OK, I have given Kevin a suggestion on using locks ranges
> > towards the end of the file as a way to regain the desired 'advisory'
> > semantics, but we wanted to be working on solid ground with consistent,
> > reliable semantics that don't depend on cache modes.
> 
> Yes, the consistency is important. Once we have a solid and reliable
> behavior we can always get things adjusted higher in the stack.
> 
> Regards.
> 
> > On Sun, 2024-06-23 at 23:54 -0500, Steve French wrote:
> > > This was interesting to dig through (and netfs caching changes made
> > > it
> > > a little harder to trace the original code) but it looks fixable. See
> > > cifs_find_fid_lock_conflict() in fs/smb/client/file.c.   It does not
> > > look new though - so let me know if you noticed that the behavior in
> > > earlier kernels was different for the default case (smb3.1.1 mounts
> > > with caching enabled).
> > > 
> > > The problem seems to be that locking is enforced only in some write
> > > paths, but these places where we do write vs. byte range locking
> > > checks (although at first glance may seem logical) obviously do not
> > > follow POSIX semantics which would allow a write to a locked range
> > > (even if POSIX behavior is counter-intuitive and different from
> > > Windows semantics).  Two obvious things to fix that I see so far:
> > > 
> > > 1) It was harder than expected to trace since looks like we don't
> > > have
> > > good dynamic (or static for that matter) tracepoints for the write
> > > and
> > > write error paths (although netfs fortunately has a few) - so
> > > obviously should add a few tracepoints to make it easier to narrow
> > > this kind of thing down in the future
> > > 2) We need to make changes to how we check lock conflicts.  See
> > > cifs_writev() and its call to cifs_find_lock_conflict(). It looks
> > > like
> > > this is the original commit that may have caused the problem:
> > > commit 85160e03a79e0d7f9082e61f6a784abc6f402701
> > > Author: Pavel Shilovsky <
> > > piastry@etersoft.ru
> > > 
> > > Date:   Sat Oct 22 15:33:29 2011 +0400
> > > 
> > >     CIFS: Implement caching mechanism for mandatory brlocks
> > >     
> > >     If we have an oplock and negotiate mandatory locking style we
> > > 
> > > handle
> > > 
> > >     all brlock requests on the client.
> > > 
> > > On Sun, Jun 9, 2024 at 11:41 PM Andrew Bartlett <
> > > abartlet@samba.org
> > > 
> > > > wrote:
> > > > (resend due spam rules on list)
> > > > 
> > > > Kia Ora Steve,
> > > > 
> > > > I'm working with Kevin on this, and I set up a clean environment
> > > > with
> > > > the latest software to make sure this is all still an issue on
> > > > current
> > > > software:
> > > > 
> > > > I was hoping to include the old SMB1 unix extensions in this test
> > > > also,
> > > > but these seem unsupported in current kernels.  When did they go
> > > > away?
> > > > 
> > > > Anyway, here is the data.  It certainly looks like an issue with
> > > > the
> > > > SMB3 client, as only the client changes with the cache=none
> > > > 
> > > > Server is Samba 4.20.1 from Debian Sid.  Kernel is
> > > > Linux debian-sid-cifs-client 6.7.9-amd64 #1 SMP PREEMPT_DYNAMIC
> > > > Debian
> > > > 6.7.9-2 (2024-03-13) x86_64 GNU/Linux
> > > > 
> > > > With SMB1 but not unix extensions (seems unsupported):
> > > > 
> > > > root@debian-sid-cifs-client:~# mount.cifs
> > > > //192.168.122.234/testuser
> > > > mnt -o user=testuser,pass=pass,vers=1.0
> > > > root@debian-sid-cifs-client:~# cd mnt/
> > > > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > > > Testing with foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: x count: 0
> > > > Third file descriptor 5
> > > > Wrote to third fd: 1
> > > > 
> > > > root@debian-sid-cifs-client:~# mount.cifs
> > > > //192.168.122.234/testuser
> > > > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,posix
> > > > root@debian-sid-cifs-client:~# cd mnt/
> > > > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > > > Testing with foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: x count: -1
> > > > Third file descriptor 5
> > > > Wrote to third fd: -1
> > > > root@debian-sid-cifs-client:~# mount.cifs
> > > > //192.168.122.234/testuser
> > > > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,unix
> > > > 
> > > > root@debian-sid-cifs-client:~# mount.cifs
> > > > //192.168.122.234/testuser
> > > > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,unix,nobrl
> > > > root@debian-sid-cifs-client:~# cd mnt/
> > > > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > > > Testing with foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: o count: 1
> > > > Third file descriptor 5
> > > > Wrote to third fd: 1
> > > > 
> > > > And with cache=none
> > > > 
> > > > root@debian-sid-cifs-client:~# mount.cifs
> > > > //192.168.122.234/testuser
> > > > mnt -o user=testuser,pass=penguin12#,vers=3.1.1,posix,cache=none
> > > > root@debian-sid-cifs-client:~# cd mnt/
> > > > root@debian-sid-cifs-client:~/mnt# ../lock_test foo
> > > > Testing with foo
> > > > Got new file descriptor 3
> > > > Lock set: 1
> > > > Second file descriptor 4
> > > > Read from second fd: o count: 1
> > > > Third file descriptor 5
> > > > Wrote to third fd: 1
> > > > 
> > > > On Thu, 2024-05-23 at 11:12 -0500, Steve French wrote:
> > > > > What is the behavior with "nobrl" mount option? and what is the
> > > > > behavior when running with the POSIX extensions enabled (e.g. to
> > > > > current Samba or ksmbd adding "posix" to the mount options)
> > > > > 
> > > > > On Thu, May 23, 2024 at 11:08 AM Kevin Ottens <
> > > > > kevin.ottens@enioka.com
> > > > > 
> > > > > > wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > I've been hunting down a bug exhibited by Libreoffice regarding
> > > > > > POSIX file
> > > > > > locks in conjunction with CIFS mounts. In short: just before
> > > > > > saving, it
> > > > > > reopens a file on which it already holds a file lock (via
> > > > > > another
> > > > > > file
> > > > > > descriptor in the same process) in order to read from it to
> > > > > > create
> > > > > > a backup
> > > > > > copy... but the read call fails.
> > > > > > 
> > > > > > I've been in discussion with Andrew Bartlett for a little while
> > > > > > regarding this
> > > > > > issue and, after exploring several venues, he advised me to
> > > > > > send an
> > > > > > email to
> > > > > > this list in order to get more opinions about it.
> > > > > > 
> > > > > > The latest discovery we did was that the cache option on the
> > > > > > mountpoint seems
> > > > > > to impact the behavior of the POSIX file locks. I made a
> > > > > > minimal
> > > > > > test
> > > > > > application (attached to this email) which basically does the
> > > > > > 
> > > > > > following:
> > > > > >  * open a file for read/write
> > > > > >  * set a POSIX write lock on the whole file
> > > > > >  * open the file a second time and try to read from it
> > > > > >  * open the file a third time and try to write to it
> > > > > > 
> > > > > > It assumes there is already some text in the file. Also, as it
> > > > > > goes
> > > > > > it outputs
> > > > > > information about the calls.
> > > > > > 
> > > > > > The output I get is the following with cache=strict on the
> > > > > > mount:
> > > > > > ---
> > > > > > Testing with /mnt/foo
> > > > > > Got new file descriptor 3
> > > > > > Lock set: 1
> > > > > > Second file descriptor 4
> > > > > > Read from second fd: x count: -1
> > > > > > Third file descriptor 5
> > > > > > Wrote to third fd: -1
> > > > > > ---
> > > > > > 
> > > > > > If I'm using cache=none:
> > > > > > ---
> > > > > > Testing with /mnt/foo
> > > > > > Got new file descriptor 3
> > > > > > Lock set: 1
> > > > > > Second file descriptor 4
> > > > > > Read from second fd: b count: 1
> > > > > > Third file descriptor 5
> > > > > > Wrote to third fd: 1
> > > > > > ---
> > > > > > 
> > > > > > That's the surprising behavior which prompted the email on this
> > > > > > list. Is it
> > > > > > somehow intended that the cache option would impact the
> > > > > > semantic of
> > > > > > the file
> > > > > > locks? At least it caught me by surprise and I wouldn't expect
> > > > > > such
> > > > > > a
> > > > > > difference in behavior.
> > > > > > 
> > > > > > Now, since the POSIX locks are process wide, I would have
> > > > > > expected
> > > > > > to have the
> > > > > > output I'm getting for the "cache=none" case to be also the one
> > > > > > I'm
> > > > > > getting
> > > > > > for the "cache=strict" case.
> > > > > > 
> > > > > > I'm looking forward to feedback on this one. I really wonder if
> > > > > > we
> > > > > > missed
> > > > > > something obvious or if there is some kind of bug in the cifs
> > > > > > driver.
> > > > > > 
> > > > > > Regards.
> > > > > > --
> > > > > > Kévin Ottens
> > > > > > kevin.ottens@enioka.com
> > > > > > 
> > > > > > 
> > > > > > +33 7 57 08 95 13
> > > > 
> > > > --
> > > > 
> > > > Andrew Bartlett (he/him)
> > > > https://samba.org/~abartlet/
> > > > 
> > > > Samba Team Member (since 2001)
> > > > https://samba.org
> > > > 
> > > > Samba Team Lead
> > > > https://catalyst.net.nz/services/samba
> > > > 
> > > > Catalyst.Net Ltd
> > > > 
> > > > Proudly developing Samba for Catalyst.Net Ltd - a Catalyst IT group
> > > > company
> > > > 
> > > > Samba Development and Support:
> > > > https://catalyst.net.nz/services/samba
> > > > 
> > > > 
> > > > Catalyst IT - Expert Open Source Solutions
> > > > 
> > > > --
> > > > Andrew Bartlett (he/him)
> > > > https://samba.org/~abartlet/
> > > > 
> > > > Samba Team Member (since 2001)
> > > > https://samba.org
> > > > 
> > > > Samba Team Lead
> > > > https://catalyst.net.nz/services/samba
> > > > 
> > > > Catalyst.Net Ltd
> > > > 
> > > > Proudly developing Samba for Catalyst.Net Ltd - a Catalyst IT group
> > > > company
> > > > 
> > > > Samba Development and Support:
> > > > https://catalyst.net.nz/services/samba
> > > > 
> > > > 
> > > > Catalyst IT - Expert Open Source Solutions


-- 
Kévin Ottens
kevin.ottens@enioka.com
+33 7 57 08 95 13