Thanks so much Steve. 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. 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. Thanks, Andrew Bartlett 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
