[PATCH] perf: work around the tested repo having an index.lock

[PATCH] perf: work around the tested repo having an index.lock

[Ævar Arnfjörð Bjarmason]

When the tested repo has an index.lock file it should be removed. This
file may be present if e.g. git-status previously crashed in that
repo, and it will make a lot of git commands fail. Let's try harder
and remove the lock.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/perf/perf-lib.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index b6fc880395..b50211b259 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -108,7 +108,14 @@ test_perf_create_repo_from () {
 		cd "$repo" &&
 		"$MODERN_GIT" init -q &&
 		test_perf_do_repo_symlink_config_ &&
-		mv .git/hooks .git/hooks-disabled 2>/dev/null
+		mv .git/hooks .git/hooks-disabled 2>/dev/null &&
+		if test -f .git/index.lock
+		then
+			# We may be copying a repo that can't run "git
+			# status" due to a locked index. Since we have
+			# a copy it's fine to remove the lock.
+			rm .git/index.lock
+		fi
 	) || error "failed to copy repository '$source' to '$repo'"
 }
 
-- 
2.13.0.506.g27d5fe0cd

Re: [PATCH] perf: work around the tested repo having an index.lock

[Jeff King]

On Fri, Jun 02, 2017 at 10:33:30AM +0000, Ævar Arnfjörð Bjarmason wrote:

> When the tested repo has an index.lock file it should be removed. This
> file may be present if e.g. git-status previously crashed in that
> repo, and it will make a lot of git commands fail. Let's try harder
> and remove the lock.

If your git-status is crashing, you probably have bigger problems (and
need to clean up the original, too).

But I think a more compelling case is that there may be an ongoing
operation in the original repo (e.g., say you are in the middle of
writing a commit message) when we do a blind copy of the filesystem
contents. You might racily pick up a lockfile.

Should we find and delete all *.lock files in the copied directory? That
would get ref locks, etc. Half-formed object files are OK. Technically
if you want to get an uncorrupted repository you'd also want to copy
refs before objects (in case somebody makes a new object and updates a
ref while you're copying).

I don't know how careful it's worth being. I don't really _object_ to
this patch exactly, but it does seem like it's picking up one random
case (that presumably you hit) and ignoring all of the related cases.

-Peff

Re: [PATCH] perf: work around the tested repo having an index.lock

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 2, 2017 at 8:45 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 02, 2017 at 10:33:30AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> When the tested repo has an index.lock file it should be removed. This
>> file may be present if e.g. git-status previously crashed in that
>> repo, and it will make a lot of git commands fail. Let's try harder
>> and remove the lock.
>
> If your git-status is crashing, you probably have bigger problems (and
> need to clean up the original, too).
>
> But I think a more compelling case is that there may be an ongoing
> operation in the original repo (e.g., say you are in the middle of
> writing a commit message) when we do a blind copy of the filesystem
> contents. You might racily pick up a lockfile.
>
> Should we find and delete all *.lock files in the copied directory? That
> would get ref locks, etc. Half-formed object files are OK. Technically
> if you want to get an uncorrupted repository you'd also want to copy
> refs before objects (in case somebody makes a new object and updates a
> ref while you're copying).
>
> I don't know how careful it's worth being. I don't really _object_ to
> this patch exactly, but it does seem like it's picking up one random
> case (that presumably you hit) and ignoring all of the related cases.

It's not a perfect solution, but it seemed not to cause any harm, and
would allow us to just do what you mean. I can't think of a case where
we'd care to preserve the index.lock in our perf copy, of course the
test may fail due to various other reasons, but at least it won't be
due to this reason.

This is just something I happened to run into locally because I had a
stale index.lock, but FWIW at work I have a git updater running on
tens of thousands of machines that has to handle various edge cases
(e.g. the machine ran out of disk space in the middle of an update, or
something was kill -9'd).

The leftover index.lock is quite common, the second most common "index
is hosed" error is "fatal: index file smaller than expected", but
solving that is more invasive, you need to rm the index and reset
--hard.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I think a more compelling case is that there may be an ongoing
> operation in the original repo (e.g., say you are in the middle of
> writing a commit message) when we do a blind copy of the filesystem
> contents. You might racily pick up a lockfile.
>
> Should we find and delete all *.lock files in the copied directory? That
> would get ref locks, etc. Half-formed object files are OK. Technically
> if you want to get an uncorrupted repository you'd also want to copy
> refs before objects (in case somebody makes a new object and updates a
> ref while you're copying).
>
> I don't know how careful it's worth being. I don't really _object_ to
> this patch exactly, but it does seem like it's picking up one random
> case (that presumably you hit) and ignoring all of the related cases.

My feeling exactly.  Diagnosing and failing upfront saying "well you
made a copy but it is not suitable for testing" sounds more sensible
at lesat to me.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 3, 2017 at 1:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> But I think a more compelling case is that there may be an ongoing
>> operation in the original repo (e.g., say you are in the middle of
>> writing a commit message) when we do a blind copy of the filesystem
>> contents. You might racily pick up a lockfile.
>>
>> Should we find and delete all *.lock files in the copied directory? That
>> would get ref locks, etc. Half-formed object files are OK. Technically
>> if you want to get an uncorrupted repository you'd also want to copy
>> refs before objects (in case somebody makes a new object and updates a
>> ref while you're copying).
>>
>> I don't know how careful it's worth being. I don't really _object_ to
>> this patch exactly, but it does seem like it's picking up one random
>> case (that presumably you hit) and ignoring all of the related cases.
>
> My feeling exactly.  Diagnosing and failing upfront saying "well you
> made a copy but it is not suitable for testing" sounds more sensible
> at lesat to me.

This change makes the repo suitable for testing when it wasn't before.

Yes, there are cases where there are other issues than index.lock
preventing testing the repo, but I don't see why there shouldn't be a
partial solution that solves a very common case in lieu of a perfect
solution.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> My feeling exactly.  Diagnosing and failing upfront saying "well you
>> made a copy but it is not suitable for testing" sounds more sensible
>> at lesat to me.
>
> This change makes the repo suitable for testing when it wasn't before.

Perhaps "not suitable" was a bit too vague.

The copy you made is not in a consistent state that is good for
testing.  This change may declare that it is now in a consistent
state, but removal of a single *.lock file does not make it so.  We
do not know what other transient inconsistency the resulting copy
has; it is inherent to git-unaware copy---that is why we discouraged
and removed rsync transport after all.

> Yes, there are cases where there are other issues than index.lock
> preventing testing the repo, but I don't see why there shouldn't be a
> partial solution that solves a very common case in lieu of a perfect
> solution.

As long as the partial solution makes sure that the case it
addressed was the only breakage, I'd be happy to see that it leaves
other kinds of inconsistencies "too rare to bother fixing".  I however
feel dirty if the punting is "we won't even bother diagnosing and
assume that *.lock is the only thing we care about".

Perhaps run "fsck" and "status" immediately after copying to make
sure they succeed, or something like that?

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps run "fsck" and "status" immediately after copying to make
> sure they succeed, or something like that?

Hmph, this is me half-being silly, as this "copying an existing one"
is meant for testing Git with a large repository, and having to run
fsck may add meaningful overhead in addition to the actual copying
in the set-up procedure.  I do not offhand think of a good solution.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Christian Couder]

On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> My feeling exactly.  Diagnosing and failing upfront saying "well you
>>> made a copy but it is not suitable for testing" sounds more sensible
>>> at lesat to me.
>>
>> This change makes the repo suitable for testing when it wasn't before.
>
> Perhaps "not suitable" was a bit too vague.
>
> The copy you made is not in a consistent state that is good for
> testing.  This change may declare that it is now in a consistent
> state, but removal of a single *.lock file does not make it so.  We
> do not know what other transient inconsistency the resulting copy
> has; it is inherent to git-unaware copy---that is why we discouraged
> and removed rsync transport after all.

If we don't like git-unaware copies, maybe we should go back to the
reasons why we are making one here.
In 342e9ef2d9 (Introduce a performance testing framework, 2012-02-17),
Thomas wrote:

    3. Creating test repos from scratch in every test is extremely
       time-consuming, and shipping or downloading such large/weird repos
       is out of the question.

       We leave this decision to the user.  Two different sizes of test
       repos can be configured, and the scripts just copy one or more of
       those (using hardlinks for the object store).  By default it tries
       to use the build tree's git.git repository.

       This is fairly fast and versatile.  Using a copy instead of a clone
       preserves many properties that the user may want to test for, such
       as lots of loose objects, unpacked refs, etc.

Is a local clone really much slower these days? Wouldn't it is use
hard links too?
By the way the many properties that are preserved might not be worth
preserving as they could make results depend a lot on the current
state of the original repo.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 4, 2017 at 9:37 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> My feeling exactly.  Diagnosing and failing upfront saying "well you
>>>> made a copy but it is not suitable for testing" sounds more sensible
>>>> at lesat to me.
>>>
>>> This change makes the repo suitable for testing when it wasn't before.
>>
>> Perhaps "not suitable" was a bit too vague.
>>
>> The copy you made is not in a consistent state that is good for
>> testing.  This change may declare that it is now in a consistent
>> state, but removal of a single *.lock file does not make it so.  We
>> do not know what other transient inconsistency the resulting copy
>> has; it is inherent to git-unaware copy---that is why we discouraged
>> and removed rsync transport after all.
>
> If we don't like git-unaware copies, maybe we should go back to the
> reasons why we are making one here.
> In 342e9ef2d9 (Introduce a performance testing framework, 2012-02-17),
> Thomas wrote:
>
>     3. Creating test repos from scratch in every test is extremely
>        time-consuming, and shipping or downloading such large/weird repos
>        is out of the question.
>
>        We leave this decision to the user.  Two different sizes of test
>        repos can be configured, and the scripts just copy one or more of
>        those (using hardlinks for the object store).  By default it tries
>        to use the build tree's git.git repository.
>
>        This is fairly fast and versatile.  Using a copy instead of a clone
>        preserves many properties that the user may want to test for, such
>        as lots of loose objects, unpacked refs, etc.
>
> Is a local clone really much slower these days? Wouldn't it is use
> hard links too?
> By the way the many properties that are preserved might not be worth
> preserving as they could make results depend a lot on the current
> state of the original repo.

AFAICT from some quick testing it'll hardlink the objects/ dir, so
e.g. you preserve the loose objects. Making the results depend on the
state of the original repo is a feature, but perhaps it should be opt
in. It's very useful to be able to take a repo that's accrued e.g. a
month's worth of refs & loose objects and test that v.s. a fresh
clone.

But there are other things that ever a hardlink local clone doesn't
preserve which might be worth preserving...

Re: [PATCH] perf: work around the tested repo having an index.lock

[Jeff King]

On Sun, Jun 04, 2017 at 09:55:15AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Is a local clone really much slower these days? Wouldn't it is use
> > hard links too?
> > By the way the many properties that are preserved might not be worth
> > preserving as they could make results depend a lot on the current
> > state of the original repo.
> 
> AFAICT from some quick testing it'll hardlink the objects/ dir, so
> e.g. you preserve the loose objects. Making the results depend on the
> state of the original repo is a feature, but perhaps it should be opt
> in. It's very useful to be able to take a repo that's accrued e.g. a
> month's worth of refs & loose objects and test that v.s. a fresh
> clone.
> 
> But there are other things that ever a hardlink local clone doesn't
> preserve which might be worth preserving...

Yes. Reflogs are one example. They aren't copied at all as part of a
clone, but they impact pruning and repacking.

-Peff

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>>> My feeling exactly.  Diagnosing and failing upfront saying "well you
>>>> made a copy but it is not suitable for testing" sounds more sensible
>>>> at lesat to me.
>>>
>>> This change makes the repo suitable for testing when it wasn't before.
>>
>> Perhaps "not suitable" was a bit too vague.
>>
>> The copy you made is not in a consistent state that is good for
>> testing.  This change may declare that it is now in a consistent
>> state, but removal of a single *.lock file does not make it so.  We
>> do not know what other transient inconsistency the resulting copy
>> has; it is inherent to git-unaware copy---that is why we discouraged
>> and removed rsync transport after all.
>
> If we don't like git-unaware copies, maybe we should go back to the
> reasons why we are making one here.

We do need git-unaware bit-for-bit copy for testing, because you may
want to see the effect of unreachable objects, for example.  

It's just that git-unaware copies, because it cannot be an atomic
snapshot, can introduce inconsistencies the original repository did
not have, rendering the result ineffective.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Christian Couder]

On Mon, Jun 5, 2017 at 4:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Sun, Jun 4, 2017 at 2:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>>> My feeling exactly.  Diagnosing and failing upfront saying "well you
>>>>> made a copy but it is not suitable for testing" sounds more sensible
>>>>> at lesat to me.
>>>>
>>>> This change makes the repo suitable for testing when it wasn't before.
>>>
>>> Perhaps "not suitable" was a bit too vague.
>>>
>>> The copy you made is not in a consistent state that is good for
>>> testing.  This change may declare that it is now in a consistent
>>> state, but removal of a single *.lock file does not make it so.  We
>>> do not know what other transient inconsistency the resulting copy
>>> has; it is inherent to git-unaware copy---that is why we discouraged
>>> and removed rsync transport after all.
>>
>> If we don't like git-unaware copies, maybe we should go back to the
>> reasons why we are making one here.
>
> We do need git-unaware bit-for-bit copy for testing, because you may
> want to see the effect of unreachable objects, for example.

I think there might be different kind of people interested in performance tests.

Users with existing repositories might want to see how the different
Git versions perform on their real life repos.
Developers might want to test Git on different repos with different
characteristics.

For example some developers might want to test on repos with and
without a lot of unreachable objects, to make sure that the latest
changes they made improve perf in both cases. While some users might
only be interested in testing on their actual repositories to see how
the latest Git versions improve things (or not) in practice.

In this example the needs of developers would perhaps be better suited
if they could control the amount of unreachable objects in the tests,
while the needs of the users would be better suited if the tests just
used actual repos as is.

So I wonder what changes would be needed to the perf framework and the
perf tests to accomodate both of these kinds of needs.



> It's just that git-unaware copies, because it cannot be an atomic
> snapshot, can introduce inconsistencies the original repository did
> not have, rendering the result ineffective.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Jeff King]

On Sun, Jun 04, 2017 at 09:00:28AM +0900, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> >> My feeling exactly.  Diagnosing and failing upfront saying "well you
> >> made a copy but it is not suitable for testing" sounds more sensible
> >> at lesat to me.
> >
> > This change makes the repo suitable for testing when it wasn't before.
> 
> Perhaps "not suitable" was a bit too vague.
> 
> The copy you made is not in a consistent state that is good for
> testing.  This change may declare that it is now in a consistent
> state, but removal of a single *.lock file does not make it so.  We
> do not know what other transient inconsistency the resulting copy
> has; it is inherent to git-unaware copy---that is why we discouraged
> and removed rsync transport after all.

Right. What I was getting at in my original message was that this is the
tip of the iceberg if we are worried about inconsistent states. And the
right solution is probably to say "you are on your own for making sure
the repo you point to is in a sane state". Because there are so many
cases to catch, and they're so rare, it's not worth trying to catch them
all.

I don't really mind addressing this one case that much. I'm not sure
that we want to accrue a pile of band-aids here that causes a
maintenance burden and doesn't really solve the problem completely. One
way to do that is to say no to the first band-aid. But we could also
apply it and see what happens. At the very worst it's a few extra lines
of code, and we can start to get worried on the second or third
band-aid.

-Peff

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't really mind addressing this one case that much. I'm not sure
> that we want to accrue a pile of band-aids here that causes a
> maintenance burden and doesn't really solve the problem completely. One
> way to do that is to say no to the first band-aid. 

Yup, that was where my objection came from.

> But we could also
> apply it and see what happens. At the very worst it's a few extra lines
> of code, and we can start to get worried on the second or third
> band-aid.

That's OK as well.  But we should resolve now that we will rip all
of them out once we start seeing the second or third band-aid.  I
really do not want to see the "accring a pile of band aids" in our
future.

Re: [PATCH] perf: work around the tested repo having an index.lock

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I think a more compelling case is that there may be an ongoing
> operation in the original repo (e.g., say you are in the middle of
> writing a commit message) when we do a blind copy of the filesystem
> contents. You might racily pick up a lockfile.
>
> Should we find and delete all *.lock files in the copied directory? That
> would get ref locks, etc. Half-formed object files are OK. Technically
> if you want to get an uncorrupted repository you'd also want to copy
> refs before objects (in case somebody makes a new object and updates a
> ref while you're copying).

Or "git branch -m A B" is in progress.

I think it all depends on what your "threat" model is ;-).  Do we
assume that many users are "time-sharing" a box and a repository?
If not, i.e. if you are the sole user of a box and a repository on
it, such a concurrent access to make the result of git-unaware copy
problematic will not be in index.lock (after all you are now doing
the perf thing, not editing a commit log message in the repository
used for testing Git), but will be in ref locks (somebody else
pushing into the repository you are *not* currently using from
sideways).

Re: [PATCH] perf: work around the tested repo having an index.lock

[Jeff King]

On Sun, Jun 04, 2017 at 11:04:50AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But I think a more compelling case is that there may be an ongoing
> > operation in the original repo (e.g., say you are in the middle of
> > writing a commit message) when we do a blind copy of the filesystem
> > contents. You might racily pick up a lockfile.
> >
> > Should we find and delete all *.lock files in the copied directory? That
> > would get ref locks, etc. Half-formed object files are OK. Technically
> > if you want to get an uncorrupted repository you'd also want to copy
> > refs before objects (in case somebody makes a new object and updates a
> > ref while you're copying).
> 
> Or "git branch -m A B" is in progress.
> 
> I think it all depends on what your "threat" model is ;-).  Do we
> assume that many users are "time-sharing" a box and a repository?
> If not, i.e. if you are the sole user of a box and a repository on
> it, such a concurrent access to make the result of git-unaware copy
> problematic will not be in index.lock (after all you are now doing
> the perf thing, not editing a commit log message in the repository
> used for testing Git), but will be in ref locks (somebody else
> pushing into the repository you are *not* currently using from
> sideways).

I was specifically thinking of the case where you run "git commit -a",
it locks the index, and then while you are writing the commit message
you need to collect some more perf results. The default perf repo is the
current repository itself, so running any perf script will copy the
index.lock and probably cause the script to misbehave.

That doesn't seem like an implausible sequence of events (frankly, I'm
surprised I haven't hit it myself, as I often run perf scripts while
writing commit messages. I think I've been saved by generally using a
separate linux.git clone as my test repo).

So it may be reasonable to say that the index.lock is special. We hold
it for a long time (compared to reflocks, which do a quick compare-and-swap).
And then we can throw up our hands for any other races. People who run
"git prune" on their test repository running the perf scripts get what
they deserve. :)

-Peff