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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Kia Ora Steve,

Just wanting to know if there is anything more Kevin or I can usefully
do to nudge this along, or do you have this in hand?

It would be very good to get this consistent.

Thanks,

Andrew Bartlett

On Thu, 2024-06-27 at 17:04 +0200, Kevin Ottens wrote:
> 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@xxxxxxxxxxx
> > > > 
> > > > 
> > > > 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@xxxxxxxxx
> > > > 
> > > > 
> > > > > 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@xxxxxxxxxx
> > > > > > 
> > > > > > 
> > > > > > > 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@xxxxxxxxxx
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +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
> 
> 
> 
-- 
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





[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux