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

Bug #98836 reported by Martin Pool
252
This bug affects 29 people
Affects Status Importance Assigned to Milestone
Bazaar
Confirmed
High
Unassigned
Breezy
Triaged
Low
Unassigned

Bug Description

As of bzr 1.12 (and many previous versions) the dirstate working tree format uses an OS lock on the dirstate file. This was done so that we could safely make in-place updates to the dirstate file. Bzr also (and its tied to the OS lock usage) uses an edit-inplace approach to modifying the file.

However, this causes several problems:

 * While the dirstate file is locked, it cannot be read: eg by info (bug 174055) or by diff
 * OS locks don't work well on all platforms
 * They are particularly problematic on network filesystems, which often don't have working file locking either inherently or because of a configuration problem, eg smb (bug 31006), AFP (bug 114528), nfs (bug 108605)
 * OS lock behaviour varies between platforms therefore is harder to test and debug, eg bug 305006
 * On some platforms, OS locks are implicitly shared across a process and this makes them harder to test and/or hides bugs
 * OS locks can't be broken and don't show who is holding the lock
 * OS locks are not supported by Jython
 * When the disk is full or bzr crashes the dirstate file can be shorter than it should be.

All these bugs are collected into this one bug, as few of them can be fixed without fixing the OS lock issue, and fixing the OS lock issue will fix them all.

If the use of diff and stat while a commit editor is open is fixed in a different way - e.g. by a separate stat cache, then we can just modify this description to only list the remaining issues.

These bugs are somewhat distinct aspects so shouldn't be marked as dupes, but they can probably best be fixed together.

Totally fixing this requires changing the format not to rely on file locking, which requires a format that is safe if it is being read and written simultaneously. That format can't assume any particular behaviour if an attempt is made to rename a file while it's being read, because that can either fail, cause an error for the reader, or follow the rename, depending on the platform.

Some partial fixes may also be possible.

Related branches

Revision history for this message
John A Meinel (jameinel) wrote :

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.

Changed in bzr:
status: Unconfirmed → Confirmed
Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 98836] Re: can't do readonly operations during a commit in a dirstate tree

> 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

Martin Pool (mbp)
Changed in bzr:
importance: Undecided → High
Revision history for this message
Andrew Cowie (afcowie) wrote : Can't do diff from editor during a commit

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

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 98836] Can't do diff from editor during a commit

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>.

Revision history for this message
Andrew Cowie (afcowie) wrote : Re: can't do readonly operations during a commit in a dirstate tree

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

Martin Pool (mbp)
description: updated
description: updated
Revision history for this message
Karl Fogel (kfogel) wrote : Re: [MASTER] dirstate file write locks exclude readers and limit portability

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.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 98836] Re: [MASTER] dirstate file write locks exclude readers and limit portability

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

-Rob

Revision history for this message
Karl Fogel (kfogel) wrote : Re: [MASTER] dirstate file write locks exclude readers and limit portability

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

Revision history for this message
Martin Pool (mbp) 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.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 98836] Re: [MASTER] dirstate file write locks exclude readers and limit portability

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

Martin Pool (mbp)
description: updated
Martin Pool (mbp)
summary: - [MASTER] dirstate file write locks exclude readers and limit portability
+ [MASTER] "OS locks must die" - dirstate file write locks exclude readers
+ and limit portability
tags: added: dirstate2
Changed in bzr:
status: Confirmed → In Progress
assignee: nobody → Robert Collins (lifeless)
Revision history for this message
Robert Collins (lifeless) wrote :

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.

Changed in bzr:
assignee: Robert Collins (lifeless) → nobody
description: updated
Changed in bzr:
status: In Progress → Confirmed
Revision history for this message
John Gilmore (gnu-gilmore) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
John Gilmore (gnu-gilmore) wrote :
Download full text (3.5 KiB)

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...

Read more...

Revision history for this message
Winston Wolff (winstonw) wrote : Re: [Bug 98836] Re: [MASTER] "OS locks must die" - dirstate file write locks exclude readers and limit portability
Download full text (4.1 KiB)

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
> ...

Read more...

Revision history for this message
rasmith (rasmith) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 98836] Re: [MASTER] "OS locks must die" - dirstate file write locks exclude readers and limit portability

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

Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Michael B. Trausch (mtrausch) wrote : Re: [Bug 98836] Re: [MASTER] "OS locks must die" - dirstate file write locks exclude readers and limit portability

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

Revision history for this message
gpk (gpk-kochanski) wrote :

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 .

Paul Sladen (sladen)
tags: added: udd
Revision history for this message
Paul Sladen (sladen) 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)
  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).

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
David I (david-ingamells) wrote :

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)

Revision history for this message
David I (david-ingamells) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

@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.

Revision history for this message
Eabin (eabin) wrote :

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

Revision history for this message
Samuel Åslund (samuel-6n) wrote :

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.)

Jelmer Vernooij (jelmer)
Changed in brz:
status: New → Triaged
importance: Undecided → Low
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.