Re: [PATCH 0/3] Another approach to large transactions

Stefan Beller [off-list ref] writes:

The problem comes from guessing the number of fds we're allowed to use.
At first I thought it was a fundamental issue with the code being broken, but
it turns out we just need a larger offset as we apparently have 9 files open
already, before the transaction even starts.
I did not expect the number to be that high, which is why I came up with the
arbitrary number of 8 (3 for stdin/out/err, maybe packed refs and reflog so I
guessed, 8 would do fine).

I am not sure if the 9 is a constant or if it scales to some unknown
property yet.
So to make the series work, all we need is:

- int remaining_fds = get_max_fd_limit() - 8;
+ int remaining_fds = get_max_fd_limit() - 9;

I am going to try to understand where the 9 comes from and resend the patches.

I have a suspicion that the above is an indication that the approach
is fundamentally not sound.  9 may be OK in your test repository,
but that may fail in a repository with different resource usage
patterns.

On the core management side, xmalloc() and friends retry upon
failure, after attempting to free the resource.  I wonder if your
codepath can do something similar to that, perhaps?

On the other hand, it may be that this "let's keep it open as long
as possible, as creat-close-open-write-close is more expensive" may
not be worth the complexity.  I wonder if it might not be a bad idea
to start with a simpler rule, e.g. "use creat-write-close for ref
updates outside transactions, and creat-close-open-write-close for
inside transactions, as that is likely to be multi-ref updates" or
something stupid and simple like that?

Michael?

On Mon, Apr 20, 2015 at 3:51 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

The problem comes from guessing the number of fds we're allowed to use.
At first I thought it was a fundamental issue with the code being broken, but
it turns out we just need a larger offset as we apparently have 9 files open
already, before the transaction even starts.
I did not expect the number to be that high, which is why I came up with the
arbitrary number of 8 (3 for stdin/out/err, maybe packed refs and reflog so I
guessed, 8 would do fine).

I am not sure if the 9 is a constant or if it scales to some unknown
property yet.
So to make the series work, all we need is:

- int remaining_fds = get_max_fd_limit() - 8;
+ int remaining_fds = get_max_fd_limit() - 9;

I am going to try to understand where the 9 comes from and resend the patches.

I have a suspicion that the above is an indication that the approach
is fundamentally not sound.  9 may be OK in your test repository,
but that may fail in a repository with different resource usage
patterns.

You put my concerns in a better wording.

On the core management side, xmalloc() and friends retry upon
failure, after attempting to free the resource.  I wonder if your
codepath can do something similar to that, perhaps?

But then we'd need to think about which fds can be 'garbage collected'.
The lock files certainly can be closed and reopened. The first 3 fd not so.
So we'd need to maintain a data structure of file descriptors good/bad
for this reclaiming.

On the other hand, it may be that this "let's keep it open as long
as possible, as creat-close-open-write-close is more expensive" may
not be worth the complexity.  I wonder if it might not be a bad idea
to start with a simpler rule, e.g. "use creat-write-close for ref
updates outside transactions, and creat-close-open-write-close for
inside transactions, as that is likely to be multi-ref updates" or
something stupid and simple like that?

I thought about any ref about goes through transaction nowadays.
Having the current patches the first n locks are creat-write-close,
while the remaining locks have the  creat-close-open-write-close
pattern, so it slows only the large transactions.

My plan is to strace all open calls and check if the aforementioned
9 open files are just a constant.

Michael?

On Mon, Apr 20, 2015 at 4:07 PM, Stefan Beller [off-list ref] wrote:

On Mon, Apr 20, 2015 at 3:51 PM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

The problem comes from guessing the number of fds we're allowed to use.
At first I thought it was a fundamental issue with the code being broken, but
it turns out we just need a larger offset as we apparently have 9 files open
already, before the transaction even starts.
I did not expect the number to be that high, which is why I came up with the
arbitrary number of 8 (3 for stdin/out/err, maybe packed refs and reflog so I
guessed, 8 would do fine).

I am not sure if the 9 is a constant or if it scales to some unknown
property yet.
So to make the series work, all we need is:

- int remaining_fds = get_max_fd_limit() - 8;
+ int remaining_fds = get_max_fd_limit() - 9;

I am going to try to understand where the 9 comes from and resend the patches.

I have a suspicion that the above is an indication that the approach
is fundamentally not sound.  9 may be OK in your test repository,
but that may fail in a repository with different resource usage
patterns.

You put my concerns in a better wording.

quoted

On the core management side, xmalloc() and friends retry upon
failure, after attempting to free the resource.  I wonder if your
codepath can do something similar to that, perhaps?

But then we'd need to think about which fds can be 'garbage collected'.
The lock files certainly can be closed and reopened. The first 3 fd not so.
So we'd need to maintain a data structure of file descriptors good/bad
for this reclaiming.

quoted

On the other hand, it may be that this "let's keep it open as long
as possible, as creat-close-open-write-close is more expensive" may
not be worth the complexity.  I wonder if it might not be a bad idea
to start with a simpler rule, e.g. "use creat-write-close for ref
updates outside transactions, and creat-close-open-write-close for
inside transactions, as that is likely to be multi-ref updates" or
something stupid and simple like that?

I thought about any ref about goes through transaction nowadays.
Having the current patches the first n locks are creat-write-close,
while the remaining locks have the  creat-close-open-write-close
pattern, so it slows only the large transactions.

My plan is to strace all open calls and check if the aforementioned
9 open files are just a constant.

When running the test locally, i.e. not in the test suite, but typing
the commands
myself into the shell, Git is fine with having just 5 file descriptors left.
The additional 4 required fds come from beign run inside the test suite.

When strace-ing git, I cannot see any possible other fds which would require
having some left over space required. So I'd propose we'd just take a reasonable
number not too small for various test setups like 32 and then go with the
proposed patches.

I'll just resend the patches to have a new basis for discussion.

quoted

Michael?

This is another attempt on enabling large transactions
(large in terms of open file descriptors). We keep track of how many
lock files are opened by the ref_transaction_commit function.
When more than a reasonable amount of files is open, we close
the file descriptors to make sure the transaction can continue.

Another idea I had during implementing this was to move this file
closing into the lock file API, such that only a certain amount of
lock files can be open at any given point in time and we'd be 'garbage
collecting' open fds when necessary in any relevant call to the lock
file API. This would have brought the advantage of having such
functionality available in other users of the lock file API as well.
The downside however is the over complication, you really need to always
check for (lock->fd != -1) all the time, which may slow down other parts
of the code, which did not ask for such a feature.

Signed-off-by: Stefan Beller <redacted>
---

This replaces the latest patch on origin/sb/remove-fd-from-ref-lock
The test suite passes now

 refs.c                | 13 +++++++++++++
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 4f495bd..1e8cb16 100644
--- a/refs.c
+++ b/refs.c

@@ -3041,6 +3041,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = EINVAL;
 		return -1;
 	}
+	if (lock->lk->fd == -1)
+		reopen_lock_file(lock->lk);
 	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {

@@ -3719,6 +3721,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 {
 	int ret = 0, i;
 	int n = transaction->nr;
+	/*
+	 * We may want to open many files in a large transaction, so come up with
+	 * a reasonable maximum, keep some spares for stdin/out and other open
+	 * files.
+	 */
+	int remaining_fds = get_max_fd_limit() - 32;
 	struct ref_update **updates = transaction->updates;
 	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 	struct string_list_item *ref_to_delete;

@@ -3762,6 +3770,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				    update->refname);
 			goto cleanup;
 		}
+		if (remaining_fds > 0) {
+			remaining_fds--;
+		} else {
+			close_lock_file(update->lock->lk);
+		}
 	}
 
 	/* Perform updates first so live commits remain referenced */

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7a69f1a..636d3a1 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -1071,7 +1071,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do

@@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
 )
 '
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do

-- 
2.4.0.rc2.5.g4c2045b.dirty

On 04/21/2015 12:51 AM, Junio C Hamano wrote:

Stefan Beller [off-list ref] writes:

quoted

The problem comes from guessing the number of fds we're allowed to use.
At first I thought it was a fundamental issue with the code being broken, but
it turns out we just need a larger offset as we apparently have 9 files open
already, before the transaction even starts.
I did not expect the number to be that high, which is why I came up with the
arbitrary number of 8 (3 for stdin/out/err, maybe packed refs and reflog so I
guessed, 8 would do fine).

I am not sure if the 9 is a constant or if it scales to some unknown
property yet.
So to make the series work, all we need is:

- int remaining_fds = get_max_fd_limit() - 8;
+ int remaining_fds = get_max_fd_limit() - 9;

I am going to try to understand where the 9 comes from and resend the patches.

I have a suspicion that the above is an indication that the approach
is fundamentally not sound.  9 may be OK in your test repository,
but that may fail in a repository with different resource usage
patterns.

On the core management side, xmalloc() and friends retry upon
failure, after attempting to free the resource.  I wonder if your
codepath can do something similar to that, perhaps?

On the other hand, it may be that this "let's keep it open as long
as possible, as creat-close-open-write-close is more expensive" may
not be worth the complexity.  I wonder if it might not be a bad idea
to start with a simpler rule, e.g. "use creat-write-close for ref
updates outside transactions, and creat-close-open-write-close for
inside transactions, as that is likely to be multi-ref updates" or
something stupid and simple like that?

Michael?

Given that the release is so close, I think we should use the simplest
thing that could work, which I think is Stefan's original
N*(creat-close),N*(open-write-close),N*rename patch. I don't think there
are many code paths that might build up a big transaction anyway (I
guess only "git update-ref --stdin" and "git push --atomic"?) Neither of
these has been around very long, so I don't think the small performance
hit will bother anybody.

The correct solution is clearly N*(creat-write-close),N*rename, but that
is too complicated for this release. So let's get the bug fixed for the
release and try to get the better fix in the next release.

It would be possible to optimize the N=1 case (I guess it's by far the
most common case) really stupidly using something like

        if (n > 1)
                close_lock_file(update->lock->lk);

but I doubt even that's worth it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

On Mon, Apr 20, 2015 at 05:31:11PM -0700, Stefan Beller wrote:

When running the test locally, i.e. not in the test suite, but typing
the commands
myself into the shell, Git is fine with having just 5 file descriptors left.
The additional 4 required fds come from beign run inside the test suite.

When strace-ing git, I cannot see any possible other fds which would require
having some left over space required. So I'd propose we'd just take a reasonable
number not too small for various test setups like 32 and then go with the
proposed patches.

FWIW, we already use a magic value of "25 extra" in open_packed_git_1. I
don't know if that means the number has been proven in practice, or if
it is simply that nobody actually exercises the pack_max_fds code. I
suspect it is the latter, especially since d131b7a (sha1_file.c: Don't
retain open fds on small packs, 2011-03-02).

-Peff

On Tue, Apr 21, 2015 at 4:21 PM, Jeff King [off-list ref] wrote:

On Mon, Apr 20, 2015 at 05:31:11PM -0700, Stefan Beller wrote:

quoted

When running the test locally, i.e. not in the test suite, but typing
the commands
myself into the shell, Git is fine with having just 5 file descriptors left.
The additional 4 required fds come from beign run inside the test suite.

When strace-ing git, I cannot see any possible other fds which would require
having some left over space required. So I'd propose we'd just take a reasonable
number not too small for various test setups like 32 and then go with the
proposed patches.

FWIW, we already use a magic value of "25 extra" in open_packed_git_1. I
don't know if that means the number has been proven in practice, or if
it is simply that nobody actually exercises the pack_max_fds code. I
suspect it is the latter, especially since d131b7a (sha1_file.c: Don't
retain open fds on small packs, 2011-03-02).

25 is equally sound as I could not find any hard calculation on that
number in the
history or code. I will change it to 25 in the next version of the patch.

Thanks!
Stefan

On Wed, Apr 22, 2015 at 12:14:08PM -0700, Stefan Beller wrote:

quoted

FWIW, we already use a magic value of "25 extra" in open_packed_git_1. I
don't know if that means the number has been proven in practice, or if
it is simply that nobody actually exercises the pack_max_fds code. I
suspect it is the latter, especially since d131b7a (sha1_file.c: Don't
retain open fds on small packs, 2011-03-02).

25 is equally sound as I could not find any hard calculation on that
number in the
history or code. I will change it to 25 in the next version of the patch.

FWIW, I think 32 is just fine, too, and the patch doesn't need re-rolled
because of this. I mostly wanted to point out that yes, indeed, we use
this "eh, a few dozen is probably enough" strategy elsewhere. Which
maybe, sort-of validates it. :)

-Peff