* [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
@ 2024-02-25 18:44 rsbecker
2024-02-25 19:08 ` rsbecker
0 siblings, 1 reply; 15+ messages in thread
From: rsbecker @ 2024-02-25 18:44 UTC (permalink / raw)
To: git
This appears to be a new issue introduced at 2.44.0. It only occurs on
NonStop ia64 but not on x86. I am not sure why this is happening although
1Mb exceeds the single I/O size on this machine.
expecting success of 7704.9 '--max-cruft-size with pruning':
git init max-cruft-size-prune &&
(
cd max-cruft-size-prune &&
test_commit base &&
foo="$(generate_random_blob foo $((1024*1024)))" &&
bar="$(generate_random_blob bar $((1024*1024)))" &&
baz="$(generate_random_blob baz $((1024*1024)))" &&
test-tool chmtime -10000 "$objdir/$(test_oid_to_path
"$foo")" &&
git repack -d --cruft --max-cruft-size=1M &&
# backdate the mtimes of all cruft packs to validate
# that they were rewritten as a result of pruning
ls $packdir/pack-*.mtimes | sort >cruft.before &&
for cruft in $(cat cruft.before)
do
mtime="$(test-tool chmtime --get -10000 "$cruft")"
&&
echo $cruft $mtime >>mtimes || return 1
done &&
# repack (and prune) with a --max-cruft-size to ensure
# that we appropriately split the resulting set of packs
git repack -d --cruft --max-cruft-size=1M \
--cruft-expiration=10.seconds.ago &&
ls $packdir/pack-*.mtimes | sort >cruft.after &&
for cruft in $(cat cruft.after)
do
old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" &&
new_mtime="$(test-tool chmtime --get $cruft)" &&
test $old_mtime -lt $new_mtime || return 1
done &&
test_line_count = 3 cruft.before &&
test_line_count = 2 cruft.after &&
test_must_fail git cat-file -e $foo &&
git cat-file -e $bar &&
git cat-file -e $baz
)
Initialized empty Git repository in
/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/t/trash
directory.t7704-repack-cruft/max-cruft-size-prune/.git/
[master (root-commit) d1ff1c9] base
Author: A U Thor mailto:author@example.com
1 file changed, 1 insertion(+)
create mode 100644 base.t
Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Enumerating cruft objects: 6, done.
Counting objects: 100% (3/3), done.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0)
ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or
directory
test_line_count: line count for cruft.after != 2
not ok 9 - --max-cruft-size with pruning
#
# git init max-cruft-size-prune &&
# (
# cd max-cruft-size-prune &&
#
# test_commit base &&
# foo="$(generate_random_blob foo $((1024*1024)))" &&
# bar="$(generate_random_blob bar $((1024*1024)))" &&
# baz="$(generate_random_blob baz $((1024*1024)))" &&
#
# test-tool chmtime -10000 "$objdir/$(test_oid_to_path
"$foo")" &&
#
# git repack -d --cruft --max-cruft-size=1M &&
#
# # backdate the mtimes of all cruft packs to validate
# # that they were rewritten as a result of pruning
# ls $packdir/pack-*.mtimes | sort >cruft.before &&
# for cruft in $(cat cruft.before)
# do
# mtime="$(test-tool chmtime --get -10000
"$cruft")" &&
# echo $cruft $mtime >>mtimes || return 1
# done &&
#
# # repack (and prune) with a --max-cruft-size to
ensure
# # that we appropriately split the resulting set of
packs
# git repack -d --cruft --max-cruft-size=1M \
# --cruft-expiration=10.seconds.ago &&
# ls $packdir/pack-*.mtimes | sort >cruft.after &&
#
# for cruft in $(cat cruft.after)
# do
# old_mtime="$(grep $cruft mtimes | cut -d" "
-f2)" &&
# new_mtime="$(test-tool chmtime --get
$cruft)" &&
# test $old_mtime -lt $new_mtime || return 1
# done &&
#
# test_line_count = 3 cruft.before &&
# test_line_count = 2 cruft.after &&
# test_must_fail git cat-file -e $foo &&
# git cat-file -e $bar &&
# git cat-file -e $baz
# )
#
1..9
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-25 18:44 [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 rsbecker @ 2024-02-25 19:08 ` rsbecker 2024-02-25 19:19 ` Torsten Bögershausen 0 siblings, 1 reply; 15+ messages in thread From: rsbecker @ 2024-02-25 19:08 UTC (permalink / raw) To: rsbecker, git On Sunday, February 25, 2024 1:45 PM, I wrote: >To: git@vger.kernel.org >Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 > >This appears to be a new issue introduced at 2.44.0. It only occurs on NonStop ia64 >but not on x86. I am not sure why this is happening although 1Mb exceeds the >single I/O size on this machine. > >expecting success of 7704.9 '--max-cruft-size with pruning': > git init max-cruft-size-prune && > ( > cd max-cruft-size-prune && > > test_commit base && > foo="$(generate_random_blob foo $((1024*1024)))" && > bar="$(generate_random_blob bar $((1024*1024)))" && > baz="$(generate_random_blob baz $((1024*1024)))" && > > test-tool chmtime -10000 "$objdir/$(test_oid_to_path "$foo")" && > > git repack -d --cruft --max-cruft-size=1M && > > # backdate the mtimes of all cruft packs to validate > # that they were rewritten as a result of pruning > ls $packdir/pack-*.mtimes | sort >cruft.before && > for cruft in $(cat cruft.before) > do > mtime="$(test-tool chmtime --get -10000 "$cruft")" >&& > echo $cruft $mtime >>mtimes || return 1 > done && > > # repack (and prune) with a --max-cruft-size to ensure > # that we appropriately split the resulting set of packs > git repack -d --cruft --max-cruft-size=1M \ > --cruft-expiration=10.seconds.ago && > ls $packdir/pack-*.mtimes | sort >cruft.after && > > for cruft in $(cat cruft.after) > do > old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" && > new_mtime="$(test-tool chmtime --get $cruft)" && > test $old_mtime -lt $new_mtime || return 1 > done && > > test_line_count = 3 cruft.before && > test_line_count = 2 cruft.after && > test_must_fail git cat-file -e $foo && > git cat-file -e $bar && > git cat-file -e $baz > ) > >Initialized empty Git repository in >/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/t/trash >directory.t7704-repack-cruft/max-cruft-size-prune/.git/ >[master (root-commit) d1ff1c9] base > Author: A U Thor mailto:author@example.com > 1 file changed, 1 insertion(+) > create mode 100644 base.t >Enumerating objects: 3, done. >Counting objects: 100% (3/3), done. >Writing objects: 100% (3/3), done. >Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating cruft >objects: 6, done. >Counting objects: 100% (3/3), done. >Compressing objects: 100% (3/3), done. >Writing objects: 100% (3/3), done. >Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating objects: 3, >done. >Counting objects: 100% (3/3), done. >Writing objects: 100% (3/3), done. >Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0) >ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or directory >test_line_count: line count for cruft.after != 2 not ok 9 - --max-cruft-size with >pruning # ># git init max-cruft-size-prune && ># ( ># cd max-cruft-size-prune && ># ># test_commit base && ># foo="$(generate_random_blob foo $((1024*1024)))" && ># bar="$(generate_random_blob bar $((1024*1024)))" && ># baz="$(generate_random_blob baz $((1024*1024)))" && ># ># test-tool chmtime -10000 "$objdir/$(test_oid_to_path >"$foo")" && ># ># git repack -d --cruft --max-cruft-size=1M && ># ># # backdate the mtimes of all cruft packs to validate ># # that they were rewritten as a result of pruning ># ls $packdir/pack-*.mtimes | sort >cruft.before && ># for cruft in $(cat cruft.before) ># do ># mtime="$(test-tool chmtime --get -10000 >"$cruft")" && ># echo $cruft $mtime >>mtimes || return 1 ># done && ># ># # repack (and prune) with a --max-cruft-size to >ensure ># # that we appropriately split the resulting set of >packs ># git repack -d --cruft --max-cruft-size=1M \ ># --cruft-expiration=10.seconds.ago && ># ls $packdir/pack-*.mtimes | sort >cruft.after && ># ># for cruft in $(cat cruft.after) ># do ># old_mtime="$(grep $cruft mtimes | cut -d" " >-f2)" && ># new_mtime="$(test-tool chmtime --get >$cruft)" && ># test $old_mtime -lt $new_mtime || return 1 ># done && ># ># test_line_count = 3 cruft.before && ># test_line_count = 2 cruft.after && ># test_must_fail git cat-file -e $foo && ># git cat-file -e $bar && ># git cat-file -e $baz ># ) ># >1..9 I did find the following calls to write(), one of which might be involved. write() should not be used directly unless the count is clearly very small. Xwrite() should be used instead. There are other calls but those are either small or not on platform. reftable/writer.c: int n = w->write(w->write_arg, zeroed, w->pending_padding); reftable/writer.c: n = w->write(w->write_arg, data, len); run-command.c: len = write(io->fd, io->u.out.buf, t/helper/test-path-utils.c: if (write(1, buffer, count) < 0) t/helper/test-windows-named-pipe.c: write(1, buf, nbr); t/helper/test-windows-named-pipe.c: write(1, buf, nbr); trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-25 19:08 ` rsbecker @ 2024-02-25 19:19 ` Torsten Bögershausen 2024-02-25 20:36 ` rsbecker 0 siblings, 1 reply; 15+ messages in thread From: Torsten Bögershausen @ 2024-02-25 19:19 UTC (permalink / raw) To: rsbecker; +Cc: git On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: > On Sunday, February 25, 2024 1:45 PM, I wrote: > >To: git@vger.kernel.org > >Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 > > > >This appears to be a new issue introduced at 2.44.0. It only occurs on > NonStop ia64 > >1..9 [snip] > > I did find the following calls to write(), one of which might be involved. > write() should not be used directly unless the count is clearly very small. > Xwrite() should be used instead. There are other calls but those are either > small or not on platform. (Probably a typ0: Xwrite() -> xwrite() But I think that this should be used: write_in_full() > > reftable/writer.c: int n = w->write(w->write_arg, zeroed, > w->pending_padding); > reftable/writer.c: n = w->write(w->write_arg, data, len); > run-command.c: len = write(io->fd, io->u.out.buf, > t/helper/test-path-utils.c: if (write(1, buffer, count) > < 0) > t/helper/test-windows-named-pipe.c: write(1, buf, nbr); > t/helper/test-windows-named-pipe.c: write(1, buf, nbr); > trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len); > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-25 19:19 ` Torsten Bögershausen @ 2024-02-25 20:36 ` rsbecker 2024-02-26 15:32 ` Phillip Wood 0 siblings, 1 reply; 15+ messages in thread From: rsbecker @ 2024-02-25 20:36 UTC (permalink / raw) To: 'Torsten Bögershausen'; +Cc: git On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >> On Sunday, February 25, 2024 1:45 PM, I wrote: >> >To: git@vger.kernel.org >> >Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 >> > >> >This appears to be a new issue introduced at 2.44.0. It only occurs >> >on >> NonStop ia64 >> >1..9 >[snip] >> >> I did find the following calls to write(), one of which might be involved. >> write() should not be used directly unless the count is clearly very small. >> Xwrite() should be used instead. There are other calls but those are >> either small or not on platform. > >(Probably a typ0: Xwrite() -> xwrite() > >But I think that this should be used: >write_in_full() My mailer autocorrected, yes, xwrite. write_in_full() would be safe, although a bit redundant since xwrite() does similar things and is used by write_in_full(). The question is which call is bad? The cruft stuff is relatively new and I don't know the code. >> reftable/writer.c: int n = w->write(w->write_arg, zeroed, >> w->pending_padding); >> reftable/writer.c: n = w->write(w->write_arg, data, len); >> run-command.c: len = write(io->fd, io->u.out.buf, >> t/helper/test-path-utils.c: if (write(1, buffer, count) >> < 0) >> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-25 20:36 ` rsbecker @ 2024-02-26 15:32 ` Phillip Wood 2024-02-26 15:52 ` rsbecker ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Phillip Wood @ 2024-02-26 15:32 UTC (permalink / raw) To: rsbecker, 'Torsten Bögershausen'; +Cc: git, Patrick Steinhardt Hi Randal [cc'ing Patrick for the reftable writer] On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: > On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>> To: git@vger.kernel.org >> But I think that this should be used: >> write_in_full() > > My mailer autocorrected, yes, xwrite. write_in_full() would be safe, > although a bit redundant since xwrite() does similar things and is used by > write_in_full(). Note that unlike write_in_full(), xwrite() does not guarantee to write the whole buffer passed to it. In general unless a caller is writing a single byte or writing less than PIPE_BUF bytes to a pipe it should use write_in_full(). > The question is which call is bad? The cruft stuff is > relatively new and I don't know the code. > >>> reftable/writer.c: int n = w->write(w->write_arg, zeroed, >>> w->pending_padding); >>> reftable/writer.c: n = w->write(w->write_arg, data, len); Neither of these appear to check for short writes and reftable_fd_write() is a thin wrapper around write(). Maybe reftable_fd_write() should be using write_in_full()? >>> run-command.c: len = write(io->fd, io->u.out.buf, This call to write() looks correct as it is in the io pump loop. >>> t/helper/test-path-utils.c: if (write(1, buffer, > count) >>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); In principle these all look like they are prone to short writes. >>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len); This caller explicitly says it prefers short writes over retrying Best Wishes Phillip ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 15:32 ` Phillip Wood @ 2024-02-26 15:52 ` rsbecker 2024-02-26 16:00 ` Phillip Wood 2024-02-27 8:45 ` Patrick Steinhardt 2 siblings, 0 replies; 15+ messages in thread From: rsbecker @ 2024-02-26 15:52 UTC (permalink / raw) To: phillip.wood, 'Torsten Bögershausen' Cc: git, 'Patrick Steinhardt' On Monday, February 26, 2024 10:32 AM, Philip Wood wrote: >On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: >> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>>> To: git@vger.kernel.org >>> But I think that this should be used: >>> write_in_full() >> >> My mailer autocorrected, yes, xwrite. write_in_full() would be safe, >> although a bit redundant since xwrite() does similar things and is >> used by write_in_full(). > >Note that unlike write_in_full(), xwrite() does not guarantee to write the whole >buffer passed to it. In general unless a caller is writing a single byte or writing less >than PIPE_BUF bytes to a pipe it should use write_in_full(). > >> The question is which call is bad? The cruft stuff is relatively new >> and I don't know the code. >> >>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed, >>>> w->pending_padding); >>>> reftable/writer.c: n = w->write(w->write_arg, data, len); > >Neither of these appear to check for short writes and >reftable_fd_write() is a thin wrapper around write(). Maybe >reftable_fd_write() should be using write_in_full()? > >>>> run-command.c: len = write(io->fd, io->u.out.buf, > >This call to write() looks correct as it is in the io pump loop. > >>>> t/helper/test-path-utils.c: if (write(1, buffer, >> count) >>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); > >In principle these all look like they are prone to short writes. > >>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len); > >This caller explicitly says it prefers short writes over retrying The real issue is that t7704.9 fails as follows: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating objects: 3, done. Counting objects: 100% (3/3), done. Writing objects: 100% (3/3), done. Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0) ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or directory test_line_count: line count for cruft.after != 2 not ok 9 - --max-cruft-size with pruning # So something is not writing the mtimes file correctly. That's what I am trying to track down. The write issue is a possible cause but not necessarily the root cause. --Randall ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 15:32 ` Phillip Wood 2024-02-26 15:52 ` rsbecker @ 2024-02-26 16:00 ` Phillip Wood 2024-02-26 18:03 ` rsbecker 2024-02-26 19:02 ` rsbecker 2024-02-27 8:45 ` Patrick Steinhardt 2 siblings, 2 replies; 15+ messages in thread From: Phillip Wood @ 2024-02-26 16:00 UTC (permalink / raw) To: rsbecker, 'Torsten Bögershausen'; +Cc: git, Patrick Steinhardt On 26/02/2024 15:32, Phillip Wood wrote: > Hi Randal > > [cc'ing Patrick for the reftable writer] > > On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: >> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>>> To: git@vger.kernel.org >>> But I think that this should be used: >>> write_in_full() >> >> My mailer autocorrected, yes, xwrite. write_in_full() would be safe, >> although a bit redundant since xwrite() does similar things and is >> used by >> write_in_full(). > > Note that unlike write_in_full(), xwrite() does not guarantee to write > the whole buffer passed to it. In general unless a caller is writing a > single byte or writing less than PIPE_BUF bytes to a pipe it should use > write_in_full(). > >> The question is which call is bad? The cruft stuff is >> relatively new and I don't know the code. I should have been clearer that I do not think any of these calls are the likely problem for the cruft pack code. I do think the reftable writers are worth looking at though for git in general. For the cruft pack problem you might want to look for suspect xwrite() calls where the caller does not handle a short write correctly for example under builtin/ we have builtin/index-pack.c: err = xwrite(1, input_buffer + input_offset, input_len); builtin/receive-pack.c: xwrite(2, msg, sz); builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); builtin/repack.c: xwrite(cmd->in, "\n", 1); builtin/unpack-objects.c: int ret = xwrite(1, buffer + offset, len); Best Wishes Phillip >>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed, >>>> w->pending_padding); >>>> reftable/writer.c: n = w->write(w->write_arg, data, len); > > Neither of these appear to check for short writes and > reftable_fd_write() is a thin wrapper around write(). Maybe > reftable_fd_write() should be using write_in_full()? > >>>> run-command.c: len = write(io->fd, io->u.out.buf, > > This call to write() looks correct as it is in the io pump loop. > >>>> t/helper/test-path-utils.c: if (write(1, buffer, >> count) >>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, >>>> buf, nbr); >>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); > > In principle these all look like they are prone to short writes. > >>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, >>>> buf_line->len); > > This caller explicitly says it prefers short writes over retrying > > Best Wishes > > Phillip ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 16:00 ` Phillip Wood @ 2024-02-26 18:03 ` rsbecker 2024-02-26 19:02 ` rsbecker 1 sibling, 0 replies; 15+ messages in thread From: rsbecker @ 2024-02-26 18:03 UTC (permalink / raw) To: phillip.wood, 'Torsten Bögershausen' Cc: git, 'Patrick Steinhardt' >From: Phillip Wood <phillip.wood123@gmail.com> On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote: >To: rsbecker@nexbridge.com; 'Torsten Bögershausen' <tboegi@web.de> >Cc: git@vger.kernel.org; Patrick Steinhardt <ps@pks.im> >Subject: Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 > >On 26/02/2024 15:32, Phillip Wood wrote: >> Hi Randal >> >> [cc'ing Patrick for the reftable writer] >> >> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: >>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>>>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>>>> To: git@vger.kernel.org >>>> But I think that this should be used: >>>> write_in_full() >>> >>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe, >>> although a bit redundant since xwrite() does similar things and is >>> used by write_in_full(). >> >> Note that unlike write_in_full(), xwrite() does not guarantee to write >> the whole buffer passed to it. In general unless a caller is writing a >> single byte or writing less than PIPE_BUF bytes to a pipe it should >> use write_in_full(). >> >>> The question is which call is bad? The cruft stuff is relatively new >>> and I don't know the code. > >I should have been clearer that I do not think any of these calls are the likely >problem for the cruft pack code. I do think the reftable writers are worth looking at >though for git in general. > >For the cruft pack problem you might want to look for suspect xwrite() calls where >the caller does not handle a short write correctly for example under builtin/ we have > >builtin/index-pack.c: err = xwrite(1, input_buffer + >input_offset, input_len); >builtin/receive-pack.c: xwrite(2, msg, sz); >builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid), >the_hash_algo->hexsz); >builtin/repack.c: xwrite(cmd->in, "\n", 1); >builtin/unpack-objects.c: int ret = xwrite(1, buffer + >offset, len); > >Best Wishes > >Phillip > >>>>> reftable/writer.c: int n = w->write(w->write_arg, >>>>> zeroed, >>>>> w->pending_padding); >>>>> reftable/writer.c: n = w->write(w->write_arg, data, len); >> >> Neither of these appear to check for short writes and >> reftable_fd_write() is a thin wrapper around write(). Maybe >> reftable_fd_write() should be using write_in_full()? >> >>>>> run-command.c: len = write(io->fd, io->u.out.buf, >> >> This call to write() looks correct as it is in the io pump loop. >> >>>>> t/helper/test-path-utils.c: if (write(1, >>>>> buffer, >>> count) >>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, >>>>> buf, nbr); >>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >> >> In principle these all look like they are prone to short writes. >> >>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, >>>>> buf_line->len); >> >> This caller explicitly says it prefers short writes over retrying I'm getting caught a bit behind the curve. After rebuilding from master, I'm now getting: + test 1708960150 -lt 1708970156 + test_line_count = 3 cruft.before + test_line_count = 2 cruft.after test_line_count: line count for cruft.after != 2 This is looking more like a different problem than xwrite(). ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 16:00 ` Phillip Wood 2024-02-26 18:03 ` rsbecker @ 2024-02-26 19:02 ` rsbecker 2024-02-26 19:45 ` phillip.wood123 1 sibling, 1 reply; 15+ messages in thread From: rsbecker @ 2024-02-26 19:02 UTC (permalink / raw) To: phillip.wood, 'Torsten Bögershausen' Cc: git, 'Patrick Steinhardt' On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote: >On 26/02/2024 15:32, Phillip Wood wrote: >> Hi Randal >> >> [cc'ing Patrick for the reftable writer] >> >> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: >>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>>>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>>>> To: git@vger.kernel.org >>>> But I think that this should be used: >>>> write_in_full() >>> >>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe, >>> although a bit redundant since xwrite() does similar things and is >>> used by write_in_full(). >> >> Note that unlike write_in_full(), xwrite() does not guarantee to write >> the whole buffer passed to it. In general unless a caller is writing a >> single byte or writing less than PIPE_BUF bytes to a pipe it should >> use write_in_full(). >> >>> The question is which call is bad? The cruft stuff is relatively new >>> and I don't know the code. > >I should have been clearer that I do not think any of these calls are the likely >problem for the cruft pack code. I do think the reftable writers are worth looking at >though for git in general. > >For the cruft pack problem you might want to look for suspect xwrite() calls where >the caller does not handle a short write correctly for example under builtin/ we have > >builtin/index-pack.c: err = xwrite(1, input_buffer + >input_offset, input_len); >builtin/receive-pack.c: xwrite(2, msg, sz); >builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid), >the_hash_algo->hexsz); >builtin/repack.c: xwrite(cmd->in, "\n", 1); >builtin/unpack-objects.c: int ret = xwrite(1, buffer + >offset, len); > >Best Wishes > >Phillip > >>>>> reftable/writer.c: int n = w->write(w->write_arg, >>>>> zeroed, >>>>> w->pending_padding); >>>>> reftable/writer.c: n = w->write(w->write_arg, data, len); >> >> Neither of these appear to check for short writes and >> reftable_fd_write() is a thin wrapper around write(). Maybe >> reftable_fd_write() should be using write_in_full()? >> >>>>> run-command.c: len = write(io->fd, io->u.out.buf, >> >> This call to write() looks correct as it is in the io pump loop. >> >>>>> t/helper/test-path-utils.c: if (write(1, >>>>> buffer, >>> count) >>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, >>>>> buf, nbr); >>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >> >> In principle these all look like they are prone to short writes. >> >>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, >>>>> buf_line->len); >> >> This caller explicitly says it prefers short writes over retrying Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up? diff --git a/builtin/index-pack.c b/builtin/index-pack.c index a3a37bd215..f80b8d101a 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, * the last part of the input buffer to stdout. */ while (input_len) { - err = xwrite(1, input_buffer + input_offset, input_len); + err = write_in_full(1, input_buffer + input_offset, input_len); if (err <= 0) break; input_len -= err; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index db65607485..4277c63d08 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params) if (use_sideband) send_sideband(1, 2, msg, sz, use_sideband); else - xwrite(2, msg, sz); + write_in_full(2, msg, sz); } __attribute__((format (printf, 1, 2))) diff --git a/builtin/repack.c b/builtin/repack.c index ede36328a3..4916870992 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -314,8 +314,8 @@ static int write_oid(const struct object_id *oid, die(_("could not start pack-objects to repack promisor objects")); } - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); - xwrite(cmd->in, "\n", 1); + write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); + write_in_full(cmd->in, "\n", 1); return 0; } diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index e0a701f2b3..6935c4574e 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED) /* Write the last part of the buffer to stdout */ while (len) { - int ret = xwrite(1, buffer + offset, len); + int ret = write_in_full(1, buffer + offset, len); if (ret <= 0) break; len -= ret; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 19:02 ` rsbecker @ 2024-02-26 19:45 ` phillip.wood123 0 siblings, 0 replies; 15+ messages in thread From: phillip.wood123 @ 2024-02-26 19:45 UTC (permalink / raw) To: rsbecker, phillip.wood, 'Torsten Bögershausen' Cc: git, 'Patrick Steinhardt' On 26/02/2024 19:02, rsbecker@nexbridge.com wrote: > On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote: >> On 26/02/2024 15:32, Phillip Wood wrote: >>> Hi Randal >>> >>> [cc'ing Patrick for the reftable writer] >>> >>> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote: >>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote: >>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote: >>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote: >>>>>>> To: git@vger.kernel.org >>>>> But I think that this should be used: >>>>> write_in_full() >>>> >>>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe, >>>> although a bit redundant since xwrite() does similar things and is >>>> used by write_in_full(). >>> >>> Note that unlike write_in_full(), xwrite() does not guarantee to write >>> the whole buffer passed to it. In general unless a caller is writing a >>> single byte or writing less than PIPE_BUF bytes to a pipe it should >>> use write_in_full(). >>> >>>> The question is which call is bad? The cruft stuff is relatively new >>>> and I don't know the code. >> >> I should have been clearer that I do not think any of these calls are the likely >> problem for the cruft pack code. I do think the reftable writers are worth looking at >> though for git in general. >> >> For the cruft pack problem you might want to look for suspect xwrite() calls where >> the caller does not handle a short write correctly for example under builtin/ we have >> >> builtin/index-pack.c: err = xwrite(1, input_buffer + >> input_offset, input_len); >> builtin/receive-pack.c: xwrite(2, msg, sz); >> builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid), >> the_hash_algo->hexsz); >> builtin/repack.c: xwrite(cmd->in, "\n", 1); >> builtin/unpack-objects.c: int ret = xwrite(1, buffer + >> offset, len); >> >> Best Wishes >> >> Phillip >> >>>>>> reftable/writer.c: int n = w->write(w->write_arg, >>>>>> zeroed, >>>>>> w->pending_padding); >>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len); >>> >>> Neither of these appear to check for short writes and >>> reftable_fd_write() is a thin wrapper around write(). Maybe >>> reftable_fd_write() should be using write_in_full()? >>> >>>>>> run-command.c: len = write(io->fd, io->u.out.buf, >>> >>> This call to write() looks correct as it is in the io pump loop. >>> >>>>>> t/helper/test-path-utils.c: if (write(1, >>>>>> buffer, >>>> count) >>>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1, >>>>>> buf, nbr); >>>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr); >>> >>> In principle these all look like they are prone to short writes. >>> >>>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, >>>>>> buf_line->len); >>> >>> This caller explicitly says it prefers short writes over retrying > > Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up? I'm glad that worked for you. If you are able to write it up that would be great. There are some dodgy looking xwrite() calls outside builtin/ as well. It is probably worth checking the return value of write_in_full() when you do the conversion for the sites where we ignore errors at the moment. Best Wishes Phillip > diff --git a/builtin/index-pack.c b/builtin/index-pack.c > index a3a37bd215..f80b8d101a 100644 > --- a/builtin/index-pack.c > +++ b/builtin/index-pack.c > @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, > * the last part of the input buffer to stdout. > */ > while (input_len) { > - err = xwrite(1, input_buffer + input_offset, input_len); > + err = write_in_full(1, input_buffer + input_offset, input_len); > if (err <= 0) > break; > input_len -= err; > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index db65607485..4277c63d08 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params) > if (use_sideband) > send_sideband(1, 2, msg, sz, use_sideband); > else > - xwrite(2, msg, sz); > + write_in_full(2, msg, sz); > } > > __attribute__((format (printf, 1, 2))) > diff --git a/builtin/repack.c b/builtin/repack.c > index ede36328a3..4916870992 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -314,8 +314,8 @@ static int write_oid(const struct object_id *oid, > die(_("could not start pack-objects to repack promisor objects")); > } > > - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); > - xwrite(cmd->in, "\n", 1); > + write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); > + write_in_full(cmd->in, "\n", 1); > return 0; > } > > diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c > index e0a701f2b3..6935c4574e 100644 > --- a/builtin/unpack-objects.c > +++ b/builtin/unpack-objects.c > @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED) > > /* Write the last part of the buffer to stdout */ > while (len) { > - int ret = xwrite(1, buffer + offset, len); > + int ret = write_in_full(1, buffer + offset, len); > if (ret <= 0) > break; > len -= ret; > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-26 15:32 ` Phillip Wood 2024-02-26 15:52 ` rsbecker 2024-02-26 16:00 ` Phillip Wood @ 2024-02-27 8:45 ` Patrick Steinhardt 2024-02-27 10:43 ` phillip.wood123 2024-02-27 14:10 ` rsbecker 2 siblings, 2 replies; 15+ messages in thread From: Patrick Steinhardt @ 2024-02-27 8:45 UTC (permalink / raw) To: phillip.wood; +Cc: rsbecker, 'Torsten Bögershausen', git [-- Attachment #1: Type: text/plain, Size: 866 bytes --] On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote: > Hi Randal > > [cc'ing Patrick for the reftable writer] > > > The question is which call is bad? The cruft stuff is > > relatively new and I don't know the code. > > > > > > reftable/writer.c: int n = w->write(w->write_arg, zeroed, > > > > w->pending_padding); > > > > reftable/writer.c: n = w->write(w->write_arg, data, len); > > Neither of these appear to check for short writes and reftable_fd_write() is > a thin wrapper around write(). Maybe reftable_fd_write() should be using > write_in_full()? It already does starting with 85a8c899ce (reftable: handle interrupted writes, 2023-12-11): ``` static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) { int *fdp = (int *)arg; return write_in_full(*fdp, data, sz); } ``` Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-27 8:45 ` Patrick Steinhardt @ 2024-02-27 10:43 ` phillip.wood123 2024-02-27 14:10 ` rsbecker 1 sibling, 0 replies; 15+ messages in thread From: phillip.wood123 @ 2024-02-27 10:43 UTC (permalink / raw) To: Patrick Steinhardt, phillip.wood Cc: rsbecker, 'Torsten Bögershausen', git Hi Patrick On 27/02/2024 08:45, Patrick Steinhardt wrote: > On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote: >>>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed, >>>>> w->pending_padding); >>>>> reftable/writer.c: n = w->write(w->write_arg, data, len); >> >> Neither of these appear to check for short writes and reftable_fd_write() is >> a thin wrapper around write(). Maybe reftable_fd_write() should be using >> write_in_full()? > > It already does starting with 85a8c899ce (reftable: handle interrupted > writes, 2023-12-11): > > ``` > static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) > { > int *fdp = (int *)arg; > return write_in_full(*fdp, data, sz); > } > ``` Oh, the branch I had checkout out was older than I realized, sorry for the confusion. Best Wishes Phillip ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-27 8:45 ` Patrick Steinhardt 2024-02-27 10:43 ` phillip.wood123 @ 2024-02-27 14:10 ` rsbecker 2024-02-27 14:22 ` Patrick Steinhardt 1 sibling, 1 reply; 15+ messages in thread From: rsbecker @ 2024-02-27 14:10 UTC (permalink / raw) To: 'Patrick Steinhardt', phillip.wood Cc: 'Torsten Bögershausen', git On Tuesday, February 27, 2024 3:46 AM, Patrick Steinhardt wrote: >On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote: >> Hi Randal >> >> [cc'ing Patrick for the reftable writer] >> >> > The question is which call is bad? The cruft stuff is relatively new >> > and I don't know the code. >> > >> > > > reftable/writer.c: int n = w->write(w->write_arg, zeroed, >> > > > w->pending_padding); >> > > > reftable/writer.c: n = w->write(w->write_arg, data, len); >> >> Neither of these appear to check for short writes and >> reftable_fd_write() is a thin wrapper around write(). Maybe >> reftable_fd_write() should be using write_in_full()? > >It already does starting with 85a8c899ce (reftable: handle interrupted writes, 2023-12-11): > >``` >static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) { > int *fdp = (int *)arg; > return write_in_full(*fdp, data, sz); >} Unfortunately, this fix is included in what I am testing but does not impact the issue I am seeing one way or another, but thank you. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-27 14:10 ` rsbecker @ 2024-02-27 14:22 ` Patrick Steinhardt 2024-02-27 14:28 ` rsbecker 0 siblings, 1 reply; 15+ messages in thread From: Patrick Steinhardt @ 2024-02-27 14:22 UTC (permalink / raw) To: rsbecker; +Cc: phillip.wood, 'Torsten Bögershausen', git [-- Attachment #1: Type: text/plain, Size: 1489 bytes --] On Tue, Feb 27, 2024 at 09:10:55AM -0500, rsbecker@nexbridge.com wrote: > On Tuesday, February 27, 2024 3:46 AM, Patrick Steinhardt wrote: > >On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote: > >> Hi Randal > >> > >> [cc'ing Patrick for the reftable writer] > >> > >> > The question is which call is bad? The cruft stuff is relatively new > >> > and I don't know the code. > >> > > >> > > > reftable/writer.c: int n = w->write(w->write_arg, > zeroed, > >> > > > w->pending_padding); > >> > > > reftable/writer.c: n = w->write(w->write_arg, data, len); > >> > >> Neither of these appear to check for short writes and > >> reftable_fd_write() is a thin wrapper around write(). Maybe > >> reftable_fd_write() should be using write_in_full()? > > > >It already does starting with 85a8c899ce (reftable: handle interrupted > writes, 2023-12-11): > > > >``` > >static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) { > > int *fdp = (int *)arg; > > return write_in_full(*fdp, data, sz); > >} > > Unfortunately, this fix is included in what I am testing but does not impact > the issue I am seeing one way or another, but thank you. I didn't expect it to :) The mentioned commit only fixes things with the reftable backend, which is not tested by default. I assume that you didn't run tests with GIT_TEST_DEFAULT_REF_FORMAT=reftable, and thus t7704 wouldn't use the reftable code in the first place. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 2024-02-27 14:22 ` Patrick Steinhardt @ 2024-02-27 14:28 ` rsbecker 0 siblings, 0 replies; 15+ messages in thread From: rsbecker @ 2024-02-27 14:28 UTC (permalink / raw) To: 'Patrick Steinhardt' Cc: phillip.wood, 'Torsten Bögershausen', git On Tuesday, February 27, 2024 9:22 AM, Patrick Steinhardt wrote: >On Tue, Feb 27, 2024 at 09:10:55AM -0500, rsbecker@nexbridge.com wrote: >> On Tuesday, February 27, 2024 3:46 AM, Patrick Steinhardt wrote: >> >On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote: >> >> Hi Randal >> >> >> >> [cc'ing Patrick for the reftable writer] >> >> >> >> > The question is which call is bad? The cruft stuff is relatively >> >> > new and I don't know the code. >> >> > >> >> > > > reftable/writer.c: int n = w->write(w->write_arg, >> zeroed, >> >> > > > w->pending_padding); >> >> > > > reftable/writer.c: n = w->write(w->write_arg, data, len); >> >> >> >> Neither of these appear to check for short writes and >> >> reftable_fd_write() is a thin wrapper around write(). Maybe >> >> reftable_fd_write() should be using write_in_full()? >> > >> >It already does starting with 85a8c899ce (reftable: handle >> >interrupted >> writes, 2023-12-11): >> > >> >``` >> >static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz) { >> > int *fdp = (int *)arg; >> > return write_in_full(*fdp, data, sz); } >> >> Unfortunately, this fix is included in what I am testing but does not >> impact the issue I am seeing one way or another, but thank you. > >I didn't expect it to :) The mentioned commit only fixes things with the reftable backend, which is not tested by default. I assume that >you didn't run tests with GIT_TEST_DEFAULT_REF_FORMAT=reftable, and thus >t7704 wouldn't use the reftable code in the first place. That is correct, I did not. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-02-27 14:28 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-25 18:44 [BUG] 2.44.0 t7704.9 Fails on NonStop ia64 rsbecker 2024-02-25 19:08 ` rsbecker 2024-02-25 19:19 ` Torsten Bögershausen 2024-02-25 20:36 ` rsbecker 2024-02-26 15:32 ` Phillip Wood 2024-02-26 15:52 ` rsbecker 2024-02-26 16:00 ` Phillip Wood 2024-02-26 18:03 ` rsbecker 2024-02-26 19:02 ` rsbecker 2024-02-26 19:45 ` phillip.wood123 2024-02-27 8:45 ` Patrick Steinhardt 2024-02-27 10:43 ` phillip.wood123 2024-02-27 14:10 ` rsbecker 2024-02-27 14:22 ` Patrick Steinhardt 2024-02-27 14:28 ` rsbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).