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 -- Kévin Ottens kevin.ottens@xxxxxxxxxx +33 7 57 08 95 13
