Re: [PATCH] gc: support temporarily preserving garbage

Jeff King [off-list ref] writes:

On Wed, Dec 03, 2014 at 01:21:03PM -0800, Brodie Rao wrote:

quoted

quoted

I think it is also not sufficient. This patch seems to cover only
objects. But we assume that we can atomically rename() new versions of
files into place whenever we like without disrupting existing readers.
This is the case for ref updates (and packed-refs), as well as the index
file.  The destination end of the rename is an unlink() in disguise, and
would be susceptible to the same problems.

I'm not aware of renaming over files happening anywhere in gc-related
code. Do you think that's something that would need to be addressed in
the rest of the code base before going forward with this garbage
directory approach? If so, do you have any suggestions on how to
tackle that problem?

As an example, if you run "git pack-refs --all --prune" (which is run by
"git gc"), it will create a new pack-refs file and rename it into place.
Another git program (say, "git for-each-ref") might be reading the file
at the same time. If you run pack-refs and for-each-ref simultaneously
in tight loops on your problematic NFS setup, what happens?

I have no idea if it breaks or not. I don't have such a misbehaving
system, and I don't know how rename() is implemented on it. But if it
_is_ a problem of the same variety, then I don't see much point in
making an invasive fix to address half of the problem areas, but not the
other half (i.e., if we are still left with a broken git in this setup,
was the invasive fix worth the cost?).

If it is a problem (and again, I am just guessing), I'd imagine you
would need a similar setup to what you proposed for unlink(): before
renaming "packed-refs.lock" into "packed-refs", hard-link it into your
"trash" area. And you'd probably want to intercept rename() there, to
catch all places where we use this technique.

Also we need to take it into account that it is not an issue unique
to Git that the server side may expire these .nfsXXXXX entries left
by an NFS client ("silly rename") to keep files that have been
removed or renamed away alive.  Aren't there a knob on the NFS
server end to control how long these are kept unexpired to avoid
stale filehandle errors, so that not just Git but all applications
running on NFS client machines will not be hurt by it?

Working it around at the application program level for each and
every application that runs on a machine that can NFS mount
filesystems from elsewhere may be simply madness, no?

On Thu, Dec 04, 2014 at 10:04:29AM -0800, Junio C Hamano wrote:

quoted

If it is a problem (and again, I am just guessing), I'd imagine you
would need a similar setup to what you proposed for unlink(): before
renaming "packed-refs.lock" into "packed-refs", hard-link it into your
"trash" area. And you'd probably want to intercept rename() there, to
catch all places where we use this technique.

Also we need to take it into account that it is not an issue unique
to Git that the server side may expire these .nfsXXXXX entries left
by an NFS client ("silly rename") to keep files that have been
removed or renamed away alive.  Aren't there a knob on the NFS
server end to control how long these are kept unexpired to avoid
stale filehandle errors, so that not just Git but all applications
running on NFS client machines will not be hurt by it?

Working it around at the application program level for each and
every application that runs on a machine that can NFS mount
filesystems from elsewhere may be simply madness, no?

Thanks. That is the vague feeling I have about the series, but without
having actually _seen_ the problem or knowing to what extent it affects
git, I didn't want to say anything so definite (e.g., I am only guessing
that rename() is a problem, but from the original description, it sounds
quite plausible that it is).

If this can be fixed outside of git, I very much agree that is the best
path forward.

-Peff