# [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock 7 messages from 2015-04-14 to 2015-04-15. Participants: Stefan Beller, Junio C Hamano, Michael Haggerty. Thread: https://gitlist.dev/t/39079 ## Stefan Beller, 2015-04-14 22:25 Subject: [PATCH 0/3] Large transactions in git Message-ID: <1429050308-9617-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1429050308-9617-1-git-send-email-sbeller%40google.com ``` On Git Merge Wilhelm Bierbaum from Twitter made clear that we'd have problems with large transactions in git. As I have been working on that series a few months ago and it still bugs me, I thought about reviving the series. However the series got stale a few months ago because we were not sure how to do it right. The first few patches were deemed uncontroversial though, which I am resending now. The first patch contains tests telling us what needs fixing. The second patch renames the precondition for other tests, as ULIMIT is a bit generic. The third patch removes the `lock_fd` field from struct ref_lock which was duplicate information. That said, this series is not fixing the actual issue, but I'd guess having these patches is step forward actually. Thanks, Stefan Stefan Beller (3): update-ref: test handling large transactions properly t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE refs.c: remove lock_fd from struct ref_lock refs.c | 16 ++++++---------- t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++ t/t7004-tag.sh | 4 ++-- 3 files changed, 36 insertions(+), 12 deletions(-) -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-04-14 22:25 Subject: [PATCH 1/3] update-ref: test handling large transactions properly Message-ID: <1429050308-9617-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1429050308-9617-2-git-send-email-sbeller%40google.com In-Reply-To: <1429050308-9617-1-git-send-email-sbeller@google.com> ``` Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7b4707b..47d2fe9 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' ' test_must_fail git rev-parse --verify -q $c ' +run_with_limited_open_files () { + (ulimit -n 32 && "$@") +} + +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' ' +( + for i in $(test_seq 33) + do + echo "create refs/heads/$i HEAD" + done >large_input && + run_with_limited_open_files git update-ref --stdin large_input && + run_with_limited_open_files git update-ref --stdin URL: https://gitlist.dev/e/1429050308-9617-3-git-send-email-sbeller%40google.com In-Reply-To: <1429050308-9617-1-git-send-email-sbeller@google.com> ``` During creation of the patch series our discussion we could have a more descriptive name for the prerequisite for the test so it stays unique when other limits of ulimit are introduced. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- t/t7004-tag.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 796e9f7..06b8e0d 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1463,10 +1463,10 @@ run_with_limited_stack () { (ulimit -s 128 && "$@") } -test_lazy_prereq ULIMIT 'run_with_limited_stack true' +test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true' # we require ulimit, this excludes Windows -test_expect_success ULIMIT '--contains works in a deep repo' ' +test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' ' >expect && i=1 && while test $i -lt 8000 -- 2.3.0.81.gc37f363 ``` ## Stefan Beller, 2015-04-14 22:25 Subject: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock Message-ID: <1429050308-9617-4-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1429050308-9617-4-git-send-email-sbeller%40google.com In-Reply-To: <1429050308-9617-1-git-send-email-sbeller@google.com> ``` The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove it. You may argue this introduces more coupling as we need to know more about the internals of the lock file mechanism, but this will be solved in a later patch. No functional changes intended. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index 14e52ca..4066752 100644 --- a/refs.c +++ b/refs.c @@ -11,7 +11,6 @@ struct ref_lock { char *orig_ref_name; struct lock_file *lk; unsigned char old_sha1[20]; - int lock_fd; int force_write; }; @@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, int attempts_remaining = 3; lock = xcalloc(1, sizeof(struct ref_lock)); - lock->lock_fd = -1; if (mustexist) resolve_flags |= RESOLVE_REF_READING; @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, goto error_return; } - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); - if (lock->lock_fd < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { + last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* * Maybe somebody just deleted one of the @@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock) { if (close_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock) { if (commit_lock_file(lock->lk)) return -1; - lock->lock_fd = -1; return 0; } @@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock, errno = EINVAL; return -1; } - if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 || + 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) { int save_errno = errno; error("Couldn't write %s", lock->lk->filename.buf); @@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1, status |= error("couldn't write %s: %s", log_file, strerror(errno)); } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && - (write_in_full(lock->lock_fd, + (write_in_full(lock->lk->fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || - write_str_in_full(lock->lock_fd, "\n") != 1 || + write_str_in_full(lock->lk->fd, "\n") != 1 || close_ref(lock) < 0)) { status |= error("couldn't write %s", lock->lk->filename.buf); -- 2.3.0.81.gc37f363 ``` ## Junio C Hamano, 2015-04-14 23:12 Subject: Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock Message-ID: URL: https://gitlist.dev/e/xmqq8udu1f0c.fsf%40gitster.dls.corp.google.com In-Reply-To: <1429050308-9617-4-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove > it. You may argue this introduces more coupling as we need to know more > about the internals of the lock file mechanism, but this will be solved in > a later patch. > > No functional changes intended. It is somewhat strange to hear "in a later patch" in [PATCH 3/3] of a 3-patch series ;-), but I think this makes sense. Whenever we take a ref-lock, and we are going to actually write something into the filesystem, we would go thru the lock_file API, so we can depend on lk to have its own file descriptor field. > > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > refs.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/refs.c b/refs.c > index 14e52ca..4066752 100644 > --- a/refs.c > +++ b/refs.c > @@ -11,7 +11,6 @@ struct ref_lock { > char *orig_ref_name; > struct lock_file *lk; > unsigned char old_sha1[20]; > - int lock_fd; > int force_write; > }; > > @@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, > int attempts_remaining = 3; > > lock = xcalloc(1, sizeof(struct ref_lock)); > - lock->lock_fd = -1; > > if (mustexist) > resolve_flags |= RESOLVE_REF_READING; > @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, > goto error_return; > } > > - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); > - if (lock->lock_fd < 0) { > + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { > + last_errno = errno; > if (errno == ENOENT && --attempts_remaining > 0) > /* > * Maybe somebody just deleted one of the > @@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock) > { > if (close_lock_file(lock->lk)) > return -1; > - lock->lock_fd = -1; > return 0; > } > > @@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock) > { > if (commit_lock_file(lock->lk)) > return -1; > - lock->lock_fd = -1; > return 0; > } > > @@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock, > errno = EINVAL; > return -1; > } > - if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || > - write_in_full(lock->lock_fd, &term, 1) != 1 || > + 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) { > int save_errno = errno; > error("Couldn't write %s", lock->lk->filename.buf); > @@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned char *sha1, > status |= error("couldn't write %s: %s", log_file, > strerror(errno)); > } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && > - (write_in_full(lock->lock_fd, > + (write_in_full(lock->lk->fd, > sha1_to_hex(cb.last_kept_sha1), 40) != 40 || > - write_str_in_full(lock->lock_fd, "\n") != 1 || > + write_str_in_full(lock->lk->fd, "\n") != 1 || > close_ref(lock) < 0)) { > status |= error("couldn't write %s", > lock->lk->filename.buf); ``` ## Michael Haggerty, 2015-04-15 08:49 Subject: Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock Message-ID: <552E2608.7020104@alum.mit.edu> URL: https://gitlist.dev/e/552E2608.7020104%40alum.mit.edu In-Reply-To: <1429050308-9617-4-git-send-email-sbeller@google.com> ``` On 04/15/2015 12:25 AM, Stefan Beller wrote: > The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove > it. You may argue this introduces more coupling as we need to know more > about the internals of the lock file mechanism, but this will be solved in > a later patch. > > No functional changes intended. This whole series LGTM; however, I suggest that this patch be split up. See below. > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > refs.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/refs.c b/refs.c > index 14e52ca..4066752 100644 > --- a/refs.c > +++ b/refs.c > [...] > @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, > goto error_return; > } > > - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); > - if (lock->lock_fd < 0) { > + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { > + last_errno = errno; > if (errno == ENOENT && --attempts_remaining > 0) > /* > * Maybe somebody just deleted one of the > [...] Here you add the line "last_errno = errno". It is a good change, but it is not part of removing ref_lock::lock_fd. I suggest that you move this change to a separate commit. You might also consider moving the new line to the "else" clause, because it's really about preserving errno around the call to error() and preparing for "goto error_return". With or without this split, this patch is Reviewed-by: Michael Haggerty Michael -- Michael Haggerty mhagger@alum.mit.edu ``` ## Junio C Hamano, 2015-04-15 18:35 Subject: Re: [PATCH 3/3] refs.c: remove lock_fd from struct ref_lock Message-ID: URL: https://gitlist.dev/e/xmqqbnipkzol.fsf%40gitster.dls.corp.google.com In-Reply-To: <552E2608.7020104@alum.mit.edu> ``` Michael Haggerty writes: > This whole series LGTM; however, I suggest that this patch be split up. > See below. > >> Signed-off-by: Stefan Beller >> Signed-off-by: Junio C Hamano >> --- >> refs.c | 16 ++++++---------- >> 1 file changed, 6 insertions(+), 10 deletions(-) >> >> diff --git a/refs.c b/refs.c >> index 14e52ca..4066752 100644 >> --- a/refs.c >> +++ b/refs.c >> [...] >> @@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, >> goto error_return; >> } >> >> - lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); >> - if (lock->lock_fd < 0) { >> + if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { >> + last_errno = errno; >> if (errno == ENOENT && --attempts_remaining > 0) >> /* >> * Maybe somebody just deleted one of the >> [...] > > Here you add the line "last_errno = errno". It is a good change, but it > is not part of removing ref_lock::lock_fd. I think this patch came from an ancient codebase before 06839515 (lock_ref_sha1_basic: do not die on locking errors, 2014-11-19), which added the "last_errno = errno", and was not rebased to match more recent codebase. I am planning to apply these on top of v2.4.0-rc, so there will be no new "save to last_errno" in the end. Thanks. ```