btrfs and git-reflog

I was just playing with git 1.5.3.8 and btrfs 0.11, and I noticed
something odd.

If I prepare a very simple repository:

  $ mkdir foo
  $ cd foo
  $ git init
  Initialized empty Git repository in .git/
  $ echo hi > blort
  $ git add .
  $ git commit -m create
  Created initial commit 4ae9415: create
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 blort

and then attempt to expire the reflogs

  $ git-reflog --expire --all

on ext3, git-reflog completes its work and exits immediately;

and on btrfs, it gets stuck in some sort of loop that causes it to
allocate more and more memory until I kill it or it pushes the
machine into OOM.

Kernel is 2.6.24 or so on x86-64.

-- 
Paul Collins
Wellington, New Zealand

Dag vijandelijk luchtschip de huismeester is dood

On Friday 25 January 2008, Paul Collins wrote:

I was just playing with git 1.5.3.8 and btrfs 0.11, and I noticed
something odd.

If I prepare a very simple repository:

  $ mkdir foo
  $ cd foo
  $ git init
  Initialized empty Git repository in .git/
  $ echo hi > blort
  $ git add .
  $ git commit -m create
  Created initial commit 4ae9415: create
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 blort

and then attempt to expire the reflogs

  $ git-reflog --expire --all

on ext3, git-reflog completes its work and exits immediately;

Strange, but I can reproduce here.  I'll take a look, thanks for this report.

-chris

On Friday 25 January 2008, Paul Collins wrote:

I was just playing with git 1.5.3.8 and btrfs 0.11, and I noticed
something odd.

If I prepare a very simple repository:

  $ mkdir foo
  $ cd foo
  $ git init
  Initialized empty Git repository in .git/
  $ echo hi > blort
  $ git add .
  $ git commit -m create
  Created initial commit 4ae9415: create
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 blort

and then attempt to expire the reflogs

  $ git-reflog expire --all

on ext3, git-reflog completes its work and exits immediately;

and on btrfs, it gets stuck in some sort of loop that causes it to
allocate more and more memory until I kill it or it pushes the
machine into OOM.

It works something like this:

readdir(.git/logs/refs/heads)
# this returns .git/logs/refs/heads/master
# <do some work>

open(.git/logs/refs/heads/master.lock, O_CREAT);
# <do more work>, write to master.lock

rename(master.lock, master)

readdir(.git/logs/refs/heads)

readdir again returns .git/logs/refs/heads/master, which is arguably
correct.  It is a new file that just happens to have a name
that git already saw.  So, git loops over this file infinitely because
it doesn't realize it has already processed it.

This happens because btrfs doesn't return the hash of the
file name as the offset to readdir.  It returns the inode number,
and since master is a new file, btrfs considers it a non-duplicate
entry.

The btrfs patch below changes my readdir code to force the
directory f_pos field to the max offset allowed when we've
seen all the directory entries.  This prevents the readdir
call from looping forever in the face of newly added files.

But, git might want to add some checks to see if it has
already processed things.

diff -r 21e9b461f802 inode.c

--- a/inode.c	Thu Jan 24 16:13:14 2008 -0500
+++ b/inode.c	Fri Jan 25 10:28:49 2008 -0500

@@ -1430,7 +1431,7 @@ read_dir_items:
 			di = (struct btrfs_dir_item *)((char *)di + di_len);
 		}
 	}
-	filp->f_pos++;
+	filp->f_pos = INT_LIMIT(typeof(filp->f_pos));
 nopos:
 	ret = 0;
 err:

On Fri, 25 Jan 2008, Chris Mason wrote:

The btrfs patch below changes my readdir code to force the
directory f_pos field to the max offset allowed when we've
seen all the directory entries.  This prevents the readdir
call from looping forever in the face of newly added files.

I think such a change may be sensible if we also taught the Linux VFS 
layer about it (so that we could avoid the costly parts of readdir() 
getting a semaphore etc the next call), but in this particular format it's 
just an ugly hack for what is a git bug.

But, git might want to add some checks to see if it has
already processed things.

Yes indeed. This is clearly git mis-using "readdir()", and should be 
fixed. It could happen on other filesystems, and very much including ones 
where there is no option of just "fixing" the filesystem (including 
Linux: not everybody can upgrade their kernels just because git makes 
some broken assumptions).

		Linus

Linus Torvalds [off-list ref] writes:

Yes indeed. This is clearly git mis-using "readdir()", and should be 
fixed. It could happen on other filesystems, and very much including ones 
where there is no option of just "fixing" the filesystem (including 
Linux: not everybody can upgrade their kernels just because git makes 
some broken assumptions).

I agree that this is a broken assumption on git's part.  When we
have this loop:

	while (ent = readdir()) {
        	... do something in that directory ...
	}

we should expect readdir() may return new entries and cope with
it, as "If a file is removed from or added to the directory
after the most recent call to opendir() or rewinddir(), whether
a subsequent call to readdir() returns an entry for that file is
unspecified." [*1*]

The modification may not be done by us, but by "git push" from
elsewhere to update an existing ref, which can run while we are
inside this loop.

For this particular case, I think keeping track of what we have
already dealt with and skipping when we see duplicates is a good
enough solution.  Removal of a loose ref is protected by taking
a lock on the packed-refs file, so we shouldn't have to worry
about races between pack-refs and a push to delete a ref (IOW,
it won't be an issue that readdir(3) may report a file that is
removd by simultaneous git-push).

However, I am not sure if the way delete_ref() takes locks (one
for the ref itself and another for packed-refs file) is
deadlock-free.  That may need an independent fix if there is; I
haven't closely looked at this.

I suspect we might want to change the current ".lock" suffix to
something that would make the lock filenames an invalid ref
(e.g., "..lock", or "~"), so that people can use "foo.lock" as a
branch name.  That is also outside of the scope of readdir(3)
abuse fix, though.


[Reference]

*1* http://www.opengroup.org/onlinepubs/000095399/functions/readdir.html