[MASTER] "OS locks must die" - dirstate file write locks exclude readers and limit portability

The error is at least improved with my patch to change "LockError" => "LockContention", which I've merged into both bzr.dev, and bzr-0.15.

I'm not sure what to do about the commit editor and such.

Things like this also happen if you do "bzr diff | less" with a large diff in one window, and "bzr add" in another.

To this point, read-locks have been a no-op, so it hasn't been a problem.

> Things like this also happen if you do "bzr diff | less" with a large
> diff in one window, and "bzr add" in another.

Well, it is a bit more justifiable that diff should wait if another
command is actually modifying the tree. In this case it's not really
conceptually necessary to exclude readers while we're waiting for a
message.

--
Martin

A common use case is trying to run :!bzr diff filename from your favourite editor while committing so you can see what exactly you changed as you're documenting it in the commit message, but it fails saying things are locked. Quite jarring.

AfC

On Sat, 2007-12-01 at 05:26 +0000, Andrew Cowie wrote:
> A common use case is trying to run :!bzr diff filename from your
> favourite editor while committing so you can see what exactly you
> changed as you're documenting it in the commit message, but it fails
> saying things are locked. Quite jarring.

Not to reduce the validity of your point, but are you aware of
'bzr commit --show-diff' ?
--Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

People are using this behaviour as a justification why they prefer Git. I don't pretend to claim that this is simple to fix, but when I pointed out Rob's "work around" I was laughed out of the room. Perhaps this could be addressed.

AfC

For the doing-things-while-commit-editor-is-in-use case, a relatively clean fix is:

1. dirstate-lock the tree, determine the changed files and directories, and calculate a checksum for the overall change (for all I know, bzr does so at this stage already; if not, it certainly could)

2. Release the lock, but remember the checksum, and bring up the commit editor.

3. When the user is done editing, resume the commit, but use the checksum to confirm that the change to be committed is exactly the same one remembered from step 1. If it's not the same, then fail with an error:

  "Someone (maybe you?) changed the tree while you were writing the
  commit message. Your commit message has been saved in foo.txt.
  Please try committing again, after confirming that the exact
  changes you intend to commit are still present."

I agree that ideally, read-only operations would always be possible, and that the overall dirstate lock problem should be solved. But this one particular symptom apparently bites a lot of users, and can be fixed another way pretty easily.

Lets *either* keep this in the bugtracker, or on the list. Tracking N
instances of the same thread will be frustrating.

-Rob

This is probably the right place to jump into the ML thread, as it contains the outline of a real solution: https://lists.ubuntu.com/archives/bazaar/2009q1/055201.html

The suggestion at the end of that thread as of today is from Robert that we should have an indirection file giving the name of the dirstate to use, and this should be a minimal change. I previously thought I'd seen a problem in that approach, but I can't think of it at the moment.

On Tue, 2009-03-24 at 23:36 +0000, Martin Pool wrote:
> The suggestion at the end of that thread as of today is from Robert that
> we should have an indirection file giving the name of the dirstate to
> use, and this should be a minimal change. I previously thought I'd seen
> a problem in that approach, but I can't think of it at the moment.

Concretely:
 - we add a indirection file
 - clients that see a missing file spin on it (as its being updated at
the time)
 - updates to the indirection file are write-and-rename, with a known
backup file name so that an incomplete replacement can be recovered
from.
 - readers can read the file the got pointed at indefinitely.
 - on close, readers check if the file is still pointed at, and if not
   try to rm it, squashing errors (as another reader may be open)
 - normal writers (commit, merge etc) that have a lockdir lock can just
   write a new file and update the indirection file
 - special writers (status etc) would write a new file, and then:
   - write a new indirection tempfile,
   - move the old one away
   - check the old one has the same pointer it had when they started
     - if not, put it back
     - if it does, put their version in place

we may still get stale dirstate files accumulating, so check should rm
ones that look implausibly old.

-Rob

I'm not really working on this either, as much as I'd love to fix it. Theres at least a passable prototype in my branch, or you could take an approach of removing the need for change detection to write dirstates for efficency as another way out of the problem.

It's insane that it's taken 5 years since the first "bzr doesn't work on NFS" bug report (in the mailing list - it comes up first in a web search for "No more locks" bzr) and STILL the developers are yammering about whether and how to fix it. Howabout "#define flock(...) noop"; that will fix the problem for 99% of the users. Then you can have an unfixed bug report for the other 1% of users who somehow scramble their files by running two commands at once.

In case you didn't figure it out, even when there's just ONE GUY using bzr, running ONE COMMAND AT A TIME, they still get screwed over by this bug, for years!

And "bzr branch http:...." first downloads megabytes of stuff, THEN fails. If you move that whole directory tree to a non-NFS filesystem (assuming you have one), then it still fails and you have to go back and re-download all the megabytes you already downloaded. I'm amazed anyone still uses bzr, when there are so many better alternatives.

John, bzr works fine on NFS unless your NFS installation is broken, as is explained perfectly clearly in bug 108605. This bug is saying it would be nice to work around misconfigured NFS. There's also a 'nolock' NFS option if you want it. Don't be a troll.

NFS works fine with BZR unless your BZR installation is broken. It's always "that other guy's problem". But real users in real network configurations who don't happen to give bzr what it demands in terms of file locks have been complaining about this for five years. Bzr doesn't gracefully fall back; it dies and leaves an ugly corpse behind. Patches have been posted for this but never applied. And yes, my NFS server is running lockd, but it doesn't actually work with my Ubuntu client's NFS implementation. Somehow, my network filesystem all works, flawlessly, for years, but not for bzr.

This bug has fourteen duplicates. In many, you tried to patiently explain that it was *their* problem and that if they just teased their network configuration long enough, no bug would actually need to be fixed. Several of those bug reports had other users tack on comments saying, "Me too - I'm having the same trouble". As far as I could tell, few of them ever actually got their problem resolved -- they just went away unsatisfied. In one of them, #137387, you went so far as to analyze a tcpdump of the underlying NFS traffic and detected a possible bug in file locking in NFSv4, then wrote "I'm not sure what to do with this bug. I'm close to saying it's a server bug or quirk and bzr is not doing anything wrong." From other comments, I think it related to subtle semantics: can you upgrade a read lock to a write lock by having the same program make a second lock on the same file, or does the second lock produce some kind of later error? It's much harder to "do the obvious thing" in that case, from the wrong end of a network connection and without any idea of which user process an NFS request is coming from. Instead, a truncate that tries to drop a byte locked by the read lock might well return an error, protecting the file contents for the reader. Even if you're right and they're wrong, you didn't solve that user's problem. The sysadmins involved had been fighting much more serious NFS bugs for years; they were hoping to find version control software that didn't tickle subtle file locking semantics questions. They patched out the read lock and it fixed the problem, but you didn't accept the patch. You didn't even take their fix for leaving a lockfile lying around when a filesystem lock fails (requiring a bzr break-lock to recover). In another report, #114528, two users reported switching their project to subversion because it worked on their AFP network and bzr didn't. Still no response.

Rather than telling me and every other bug reporter to reconfigure our filesystems and LANs and patch up our kernels, please consider this a wake-up call. File locking is giving your users more trouble than it cures. The existing code doesn't work cleanly in a wide variety of actual installations, despite the specs saying that it should. Even when it works, it takes too much sysadmin effort, and when it fails, it invariably hurts somebody who never actually needed any file locking, somebody who'll never do bzr operations in parallel. Rewriting the whole file format seems to be taking a lot of time and isn't working yet. Maybe the old file format will work e...

I quit using BZR a long time ago because this problem blocked me. I use Git now. Works great.

Winston Wolff
Stratolab - Games for Learning
tel: (646) 827-2242
web: www.stratolab.com

On Aug 4, 2010, at 11:04 PM, John Gilmore wrote:

> NFS works fine with BZR unless your BZR installation is broken. It's
> always "that other guy's problem". But real users in real network
> configurations who don't happen to give bzr what it demands in terms of
> file locks have been complaining about this for five years. Bzr doesn't
> gracefully fall back; it dies and leaves an ugly corpse behind. Patches
> have been posted for this but never applied. And yes, my NFS server is
> running lockd, but it doesn't actually work with my Ubuntu client's NFS
> implementation. Somehow, my network filesystem all works, flawlessly,
> for years, but not for bzr.
>
> This bug has fourteen duplicates. In many, you tried to patiently
> explain that it was *their* problem and that if they just teased their
> network configuration long enough, no bug would actually need to be
> fixed. Several of those bug reports had other users tack on comments
> saying, "Me too - I'm having the same trouble". As far as I could tell,
> few of them ever actually got their problem resolved -- they just went
> away unsatisfied. In one of them, #137387, you went so far as to
> analyze a tcpdump of the underlying NFS traffic and detected a possible
> bug in file locking in NFSv4, then wrote "I'm not sure what to do with
> this bug. I'm close to saying it's a server bug or quirk and bzr is not
> doing anything wrong." From other comments, I think it related to
> subtle semantics: can you upgrade a read lock to a write lock by having
> the same program make a second lock on the same file, or does the second
> lock produce some kind of later error? It's much harder to "do the
> obvious thing" in that case, from the wrong end of a network connection
> and without any idea of which user process an NFS request is coming
> from. Instead, a truncate that tries to drop a byte locked by the read
> lock might well return an error, protecting the file contents for the
> reader. Even if you're right and they're wrong, you didn't solve that
> user's problem. The sysadmins involved had been fighting much more
> serious NFS bugs for years; they were hoping to find version control
> software that didn't tickle subtle file locking semantics questions.
> They patched out the read lock and it fixed the problem, but you didn't
> accept the patch. You didn't even take their fix for leaving a lockfile
> lying around when a filesystem lock fails (requiring a bzr break-lock to
> recover). In another report, #114528, two users reported switching
> their project to subversion because it worked on their AFP network and
> bzr didn't. Still no response.
>
> Rather than telling me and every other bug reporter to reconfigure our
> filesystems and LANs and patch up our kernels, please consider this a
> wake-up call. File locking is giving your users more trouble than it
> cures. The existing code doesn't work cleanly in a wide variety of
> actual installations, despite the specs saying that it should. Even
> ...

Same here. BZR just crashes on our Samba shares, while Git happily does whatever I ask it to do. I really tried to use BZR and everything was fine until I needed to put a repository on our network. FTP/SFTP/FTPS is not allowed on our network and we are not going to be able to get the shares reconfigured, so the only option for us was Git.

Hi John,

I realize it's very frustrating if you hit this, especially if the
problem only occurs with bzr. Determining whether it is a local
configuration problem or a bug specific to one NFS client or server is
necessary but not very helpful to users who are being bitten.
Basically every substantial project I've worked on that does any disk
storage has needed to work around NFS bugs or quirks of one kind or
another, so I really should have known better and vetoed a format that
relied on file locking. I'm sorry I didn't.

If someone puts up a patch that solves this without introducing worse
bugs I will very happily take it. I guess many people may feel it's a
reasonable tradeoff but just taking chances on losing their data seems
like a poor risk.

(On the other hand, since I think we do have a lockdir lock, perhaps
doing without the OS lock won't risk more than a transient error.
I'll have a look at it.)

--
Martin

We rewrite the dirstate file in-place, which will be rather messy
without the os lock.

I will mention that I have put up a branch that works without oslocks;
it needs a little polish is all and should solve the problem
permanently.

On Fri, 2010-08-06 at 00:46 +0000, Martin Pool wrote:
> If someone puts up a patch that solves this without introducing worse
> bugs I will very happily take it. I guess many people may feel it's a
> reasonable tradeoff but just taking chances on losing their data seems
> like a poor risk.
>
> (On the other hand, since I think we do have a lockdir lock, perhaps
> doing without the OS lock won't risk more than a transient error.
> I'll have a look at it.)

I should think that there would be a way for bzr to do all of its
locking internally to the repository, instead of using operating system
locks which are more-or-less different on every operating system. Sure,
they're abstracted a bit, but the exact semantics are different
depending on where you're running and where your data is. I think that
probably one of the major problems with NFS is just that.

Perhaps bzr could learn how to fall back to an internal lock format,
such that even in the face of a broken operating system or a broken
filesystem, bzr can continue operations safely, if maybe more slowly.

 --- Mike

I just spent all day finding the BZR on NFS4 bug again.
See https://bugs.launchpad.net/bugs/665082 and
https://bugs.launchpad.net/bugs/480444 .

AFAICT, there are various levels of "fixing" this required, at the simpler end being:

  1. Don't bother querying the lock for read-only operations (just fail on exception in the case of genuinely inconsistent data)
  2. For commit, only hold the lock during the actual update phase (after the use was closed the editor and saved the tempfile) (comment #6).

to the far larger end of things:

  3. Changing the fundamental update mechanism/process to be lock-free and persistently self-coherent (comment #10).

On Wed, Nov 10, 2010 at 1:29 PM, Paul Sladen <email address hidden> wrote:
> AFAICT, there are various levels of "fixing" this required, at the
> simpler end being:
>
>  1. Don't bother querying the lock for read-only operations (just fail on exception in the case of genuinely inconsistent data)

This would break bzr when updates occured.

>  2. For commit, only hold the lock during the actual update phase (after the use was closed the editor and saved the tempfile) (comment #6).

This would break when a reader exists (cannot boot a reader out)
leading to a half-done commit in a very confusing fashion.

> to the far larger end of things:
>
>  3. Changing the fundamental update mechanism/process to be lock-free
> and persistently self-coherent (comment #10).

Prototype code is written.

-Rob

I get the following during a bzr merge on an Ubuntu 08.04 system with an nfs4 mounted filesystem:

open("/data/id/CmsRoot/work/DHive/integrate_task_14101_0/.bzr/checkout/dirstate", O_RDWR|O_LARGEFILE) = 5
fstat64(5, {st_mode=S_IFREG|0644, st_size=32598, ...}) = 0
fcntl64(5, F_SETLK64, {type=F_WRLCK, whence=SEEK_SET, start=0, len=0}, 0xbf978570) = 0
fstat64(5, {st_mode=S_IFREG|0644, st_size=32598, ...}) = 0
mmap2(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6dd8000
_llseek(5, 0, [0], SEEK_SET) = 0
write(5, "#bazaar dirstate flat format 3\nc"..., 37837) = 37837
ftruncate64(5, 37837) = 0
_llseek(5, 0, [0], SEEK_SET) = 0
read(5, "#bazaar dirstate flat format 3\nc"..., 37837) = 32598
_llseek(5, 5239, [37837], SEEK_CUR) = 0
fcntl64(5, F_SETLKW64, {type=F_UNLCK, whence=SEEK_SET, start=0, len=0}, 0xbf978ae0) = 0
close(5) = -1 EIO (Input/output error)

Sorry forgot to say that was generated by running strace bzr merge ...

with version 2.3.0, there was no traceback and bzr just said

bzr: ERROR: [Errno 5] Input/output error

@David I, this probably means that you (or your sysadmins) have not
enabled nfs locking on this filesystem. If you do that it will work
ok.

this might be helpful for others stumbling across this bug: in kernel 2.6.34 there was a NFS4 server bug, locking up the client:

https://bugzilla.kernel.org/show_bug.cgi?id=16277

These locks are not enough to keep the local bazaar repository in shape any way!

******************************************************************************
$ bzr ci src/ksn/Makefile
Committing to: <repo_path>
aborting commit write group: DirstateCorrupt(The dirstate file (DirState(u'<repo_path>/.bzr/checkout/dirstate')) appears to be corrupt: Bad parse, we expected to end on \n, not: 45 to_and_nct-20120307150343-sa5f7rw4ganyg7xx-21: (StaticTuple('tests/etc', 'Spec_Plus_Nums', 'spec_plus_nums-20120307150343-sa5f7rw4ganyg7xx-20'), [StaticTuple('l', '<other_path>/forPYuxwAAAAJAD+DXAAAKH/', 0L, 0, '0'), StaticTuple('n', '<email address hidden>', 0L, 0, 'TO_And_NCT')]))
bzr: ERROR: The dirstate file (DirState(u'<repo_path>r/.bzr/checkout/dirstate')) appears to be corrupt: Bad parse, we expected to end on \n, not: 45 to_and_nct-20120307150343-sa5f7rw4ganyg7xx-21: (StaticTuple('tests/etc', 'Spec_Plus_Nums', 'spec_plus_nums-20120307150343-sa5f7rw4ganyg7xx-20'), [StaticTuple('l', '<other_path>/forPYuxwAAAAJAD+DXAAAKH/', 0L, 0, '0'), StaticTuple('n', '<email address hidden>', 0L, 0, 'TO_And_NCT')])
******************************************************************************

Yes, we have some kind of problem in our NFS setup.
Yes, I interrupted a "status" command right before the above happend.

No, I _do_not_ expect a _read_ command to be able to trash my repository.

Could you possibly stop updating files in place and write the new information to a backup file and move it to the right name after it is complete?
(That's what I/we always do to avoid these kind of problems. Move or better yet hard-links use to be atomic in the file-system.)