[BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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.

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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);

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[Torsten Bögershausen]

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);
>
>
>
>

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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);

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[Phillip Wood]

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

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[Phillip Wood]

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

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

>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().

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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;

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[phillip.wood123]

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;
> 
> 

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[Patrick Steinhardt]

[-- 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 --]

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[phillip.wood123]

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

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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. 

Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[Patrick Steinhardt]

[-- 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 --]

RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64

[rsbecker]

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.