checksum: keep the link alive while hashing a file (fixes #839)#1031
checksum: keep the link alive while hashing a file (fixes #839)#1031dr-who wants to merge 1 commit into
Conversation
…t#839) With --checksum the sender hashes every regular file while it builds the file list (flist.c calls file_checksum() from make_file()), and nothing is sent to the peer for as long as that takes. On a large file the far end therefore sees a silent link and aborts once --timeout elapses: rsync -a --checksum --timeout=3 rsync://host/mod/ dst/ [Receiver] io timeout after 4 seconds -- exiting rsync error: timeout in data send/receive (code 30) A single 2 GiB file reproduces it here (~4.3s to hash). The same transfer without --checksum, or with a --timeout longer than the hash, is fine, so it is purely the unbroken silence while hashing. Note it is driven by the time to hash one file, not by how many there are: 500,000 small files take 4.6s to hash in total and do *not* trip it, because list entries keep streaming out and the link never goes quiet. rsync already has the right primitive for this. maybe_send_keepalive() sends a MSG_NOOP and self-throttles on allowed_lull, which set_io_timeout() computes as half of --timeout -- exactly the "send something every half the threshold" interval this needs, at whatever timeout the user picked. The generator, sender and receiver already call it during their long stretches; the file-list hashing path simply never did. Tick it as we hash. file_checksum() had six copies of the same chunk loop (one per digest), so rather than add the call six times -- and leave a seventh digest free to forget it -- the loop is now a single CHECKSUM_FEED() macro carrying the tick. It takes the update as an expression because the digests have differing signatures; a function-pointer iterator would need a wrapper per digest and could not express the pre-27 MD4 tail, so each caller keeps its own trailing partial chunk handling unchanged. The tick is throttled by bytes hashed rather than fired per piece: the MD4 loop runs in CSUM_CHUNK (64) byte pieces, so a per-piece tick would consult the clock 16.7 million times per GiB -- 512x more often than the other digests. That measured +2.6% on an MD4 pass here (1.95s -> 2.00s for 1 GiB, matching the 2.9ns vDSO time() call), and would be far worse where time() is a real syscall. Consulting it once per CHUNK_SIZE of hashing puts every digest on the same footing and the cost back in the noise (1.94s). Verified digest-identical to the unpatched build in both directions (master sender vs patched receiver and vice versa) over files at the chunk boundaries -- 0, 1, 63, 64, 65, 32767, 32768, 32769, 65537 and multi-chunk sizes -- for every checksum-choice this build offers, with a flipped byte as a positive control. generate_and_send_sums() was checked for the same pattern and does not need it: it writes each block sum as it computes, so that link never goes quiet.
192c461 to
123576e
Compare
|
Follow-up: the keep-alive tick is now throttled by bytes hashed rather than fired once per piece. The first version called it from every loop iteration, which was fine for the digests that work in Measured on a 1 GiB file with
That +2.6% matches the arithmetic exactly —
Re-verified after the change: digest-identical to master in both directions across the chunk-boundary sizes, still fixes the hang at |
1 participant
Alternative to #840 for issue #839 — credit to @tenox7 for the report and for identifying the keepalive as the mechanism. I reproduced the bug, then found #840's version doesn't fix the reported case; details below.
Reproducing it
With
--checksumthe sender hashes every regular file while building the file list (flist.c callsfile_checksum()frommake_file()), and sends nothing meanwhile. A single 2 GiB file (~4.3s to hash):Controls: the same transfer without
--checksum→ fine;--checksum --timeout=30→ fine;--checksumwith no timeout → fine. So it is purely the unbroken silence while hashing.It's driven by per-file hash time, not file count. 500,000 small files take 4.6s to hash in total and do not trip it, because list entries keep streaming and the link never goes quiet. One big file does.
Two problems with #840
1. The hardcoded 5-second interval can't fix the reported case. I applied #840 and re-ran the reproducer:
--timeout=3--timeout=6--timeout=12With
--timeout=3the peer has already given up before #840's first keepalive at 5s.2. The
am_daemongate is too narrow. The same hang happens over a remote shell:This change
rsync already has the right primitive:
maybe_send_keepalive()sends aMSG_NOOPand self-throttles onallowed_lull, whichset_io_timeout()computes as half of--timeout— exactly the interval this needs, at any timeout value. The generator, sender and receiver already call it during their long stretches (generator.c:296,sender.c:116,receiver.c:390); the file-list hashing path simply never did.file_checksum()had six copies of the same chunk loop, one per digest — which is why #840 had to patch each. Rather than add the call six times and leave a seventh digest free to forget it, the loop is now a singleCHECKSUM_FEED()macro carrying the tick.It takes the update as an expression deliberately: the digests have differing signatures (
EVP_DigestUpdate,XXH64_update,md5_update,mdfour_update), so a function-pointer iterator would need a wrapper per digest, and it could not express the pre-27 MD4 case, which usesCSUM_CHUNKand a different tail condition. Each caller keeps its own trailing-partial-chunk handling unchanged.Verification
--timeout=1upward.generate_and_send_sums()for the same pattern: it does not need it, because it writes each block sum as it computes, so that link never goes quiet.On a regression test: I deliberately haven't added one. Exercising this needs a multi-GB file and a wall-clock race against
--timeout, which would be slow and timing-flaky across the CI platforms. Happy to add one if you'd like it, but it seemed worse than documenting the measured reproduction here.