Re: [PATCHv3 6/6] refs.c: enable large transactions
On Fri, Jan 23, 2015 at 4:38 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano gits...@pobox.com wrote:
yeah that's the goal. Though as we're in one transaction, as soon
as we have an early exit, the transaction will abort.
An early exit I am talking about is this:
static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
struct object *o;
if (!lock) {
errno = EINVAL;
return -1;
}
if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
return 0;
It returns 0 and then the transaction side has this call in a loop:
if (!is_null_sha1(update-new_sha1)) {
if (write_ref_sha1(update-lock, update-new_sha1,
update-msg)) {
strbuf_addf(err, Cannot write to the ref
lock '%s'.,
update-refname);
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
}
And just after this code path there is the new
+ /* Do not keep all lock files open at the same time. */
+ close_ref(update-lock);
So in case we go the zero return path of write_ref_sha1 the loop looks like
/* Acquire all locks while verifying old values */
for (all updates) {
write_ref_sha1(...)
close_ref(...)
}
In case we do go the non zero return path, it is
if (write_ref_sha1(update-lock, update-new_sha1, ..)
goto cleanup;
...
cleanup:
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
I do not see the problem in the code itself, but rather in understanding
the code. I will send a follow up patch which makes it easier to follow
by removing the early exit with no problem away.
If so, shouldn't the write function at least close the file
descriptor even when it knows that the $ref.lock already has the
correct object name? The call to close_ref() is never made when the
early-return codepath is taken as far as I can see.
The goto cleanup; will take care of unlocking (and closing fds of) all refs
Yes, if write_ref_sha1() returns with non-zero signaling an error,
then the goto will trigger.
But if it short-cuts and returns zero, that goto will not be
reached.
Yes, if the goto is not reached we just drop down to
close_ref(update-lock); which should take care of
the open fd.
Thanks,
Stefan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
Stefan Beller sbel...@google.com writes: I do not see the problem in the code itself, but rather in understanding the code. I will send a follow up patch which makes it easier to follow by removing the early exit with no problem away. Taken as a whole the code may function correctly but the division of roles of individual functions seems screwed up. write_ref_sha1() sometimes unlocks, and sometimes leaves the unlocking to the caller, and the caller cannot even tell if it is expected to do the unlocking for it from the return value because both cases return 0 (success). I am not sure if it is sensible to call that correct but hard to understand. I'd rather see us admit that its behaviour is screwey and needs fixing for better code health longer term. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 6/6] refs.c: enable large transactions
By closing the file descriptors after creating the lock file we are not
limiting the size of the transaction by the number of available file
descriptors.
When closing the file descriptors early, we also need to write the values
in early, if we don't want to reopen the files.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
version3:
* Do not reopen the files after closing them. Make sure we have
written all necessary information before closing the file.
Doing it that way enabled us to drop the patch [PATCH 4/6]
refs.c: Have a write_in_full_to_lock_file wrapping write_in_full
refs.c| 15 ---
t/t1400-update-ref.sh | 4 ++--
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 1bfc84b..2594b23 100644
--- a/refs.c
+++ b/refs.c
@@ -3752,6 +3752,17 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-refname);
goto cleanup;
}
+ if (!is_null_sha1(update-new_sha1)) {
+ if (write_ref_sha1(update-lock, update-new_sha1,
+ update-msg)) {
+ strbuf_addf(err, Cannot write to the ref lock
'%s'.,
+ update-refname);
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ }
+ /* Do not keep all lock files open at the same time. */
+ close_ref(update-lock);
}
/* Perform updates first so live commits remain referenced */
@@ -3759,9 +3770,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- if (write_ref_sha1(update-lock, update-new_sha1,
- update-msg)
- || commit_ref(update-lock, update-new_sha1)) {
+ if (commit_ref(update-lock, update-new_sha1)) {
strbuf_addf(err, Cannot update the ref '%s'.,
update-refname);
ret = TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,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
@@ -990,7 +990,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.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
Stefan Beller sbel...@google.com writes: By closing the file descriptors after creating the lock file we are not limiting the size of the transaction by the number of available file descriptors. When closing the file descriptors early, we also need to write the values in early, if we don't want to reopen the files. I am not sure if an early return in write_ref_sha1() is entirely correct. The unlock step was split out of write and commit functions in the previous step because you eventually want to be able to close the file descriptor that is open to $ref.lock early, while still keeping the $ref.lock file around to avoid others competing with you, so that you can limit the number of open file descriptors, no? If so, shouldn't the write function at least close the file descriptor even when it knows that the $ref.lock already has the correct object name? The call to close_ref() is never made when the early-return codepath is taken as far as I can see. Puzzled... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: By closing the file descriptors after creating the lock file we are not limiting the size of the transaction by the number of available file descriptors. When closing the file descriptors early, we also need to write the values in early, if we don't want to reopen the files. I am not sure if an early return in write_ref_sha1() is entirely correct. The unlock step was split out of write and commit functions in the previous step because you eventually want to be able to close the file descriptor that is open to $ref.lock early, while still keeping the $ref.lock file around to avoid others competing with you, so that you can limit the number of open file descriptors, no? yeah that's the goal. Though as we're in one transaction, as soon as we have an early exit, the transaction will abort. If so, shouldn't the write function at least close the file descriptor even when it knows that the $ref.lock already has the correct object name? The call to close_ref() is never made when the early-return codepath is taken as far as I can see. The goto cleanup; will take care of unlocking (and closing fds of) all refs -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
Stefan Beller sbel...@google.com writes:
On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano gits...@pobox.com wrote:
yeah that's the goal. Though as we're in one transaction, as soon
as we have an early exit, the transaction will abort.
An early exit I am talking about is this:
static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
struct object *o;
if (!lock) {
errno = EINVAL;
return -1;
}
if (!lock-force_write !hashcmp(lock-old_sha1, sha1))
return 0;
It returns 0 and then the transaction side has this call in a loop:
if (!is_null_sha1(update-new_sha1)) {
if (write_ref_sha1(update-lock, update-new_sha1,
update-msg)) {
strbuf_addf(err, Cannot write to the ref lock
'%s'.,
update-refname);
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
}
If so, shouldn't the write function at least close the file
descriptor even when it knows that the $ref.lock already has the
correct object name? The call to close_ref() is never made when the
early-return codepath is taken as far as I can see.
The goto cleanup; will take care of unlocking (and closing fds of) all refs
Yes, if write_ref_sha1() returns with non-zero signaling an error,
then the goto will trigger.
But if it short-cuts and returns zero, that goto will not be
reached.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
