Opened 11 years ago

Closed 10 years ago

Last modified 6 years ago

#2955 closed Enhancement (fixed)

verify pieces only when necessary, or when the user requests it.

Reported by: charles Owned by: charles
Priority: Normal Milestone: 2.20
Component: libtransmission Version: 1.91
Severity: Normal Keywords:
Cc: colrol@…, giovannibajo@…

Description (last modified by charles)

NAMING

This is called "lazy verify", "just-in-time verify", or "JIT verify", and was suggested by alus.

SUMMARY

There are three cases where pre-JIT Transmission verifies an entire torrent all at once: (1) when a user asks it to, (2) when a torrent is newly added, and (3) when Transmission notices some kind of a change in the files, typically a changed timestamp.

This ticket doesn't alter (1) or (2) at all. Those aren't causing any trouble.

However, (3) bugs lots of people. Let's say you move a torrent's files around in the OS' file manager, then fire up Transmission and tell it where to find them. Transmission notices that the file's timestamps have changed, so it reverifies them. That's a pain if you moved three or four Linux DVD iso files and now wait on 16 GiB worth of checksum tests.

Under "just-in-time" verification, Transmission will make a note of the changed timestamps, but assume that you know what you're doing and not do a full pass.

The first time Transmission accesses a piece associated with a changed timestamp, then it will check that piece before using it. (For example, before uploading it to a peer.) Given enough time the whole torrent will still be checked, but since it's done in such small chunks, it's largely invisible to the user.

If a piece fails its checksum test, the torrent is stopped and an error message is shown asking the user to Verify Local Data.

RELATED PROBLEMS

One extremely common way to trigger (3) is to forget to plug in an external drive that has a torrent's data on it. That's mostly covered by JIT verify, so for trac bookkeeping purposes I've included it in this ticket.

In addition to JIT verify, the unplugged-HD problem needs a "did you leave a drive unplugged?" type message and to stop torrents when their files disappear.

Attachments (2)

lazyVerify.patch (27.0 KB) - added by Longinus00 10 years ago.
Update to ensure clean patching
lazyVerify-timestamp.diff (33.7 KB) - added by charles 10 years ago.

Download all attachments as: .zip

Change History (57)

comment:1 Changed 11 years ago by Rolcol

  • Cc colrol@… added

comment:2 Changed 11 years ago by Longinus00

I've been playing around with this and have a rough implementation but I'm curious if utorrent checks every piece requested or if they only check files which they suspect have been tampered with.

comment:3 Changed 11 years ago by charles

I don't know what uTorrent does, but IMO once we get one piece that doesn't match our expectations, we should verify the entire torrent to be safe.

comment:4 Changed 11 years ago by Longinus00

I agree with you on that point, I was just wondering if utorrent checks every piece it reads or only on torrents it thinks have been modified in some way. For instance, in transmission, should every piece read by tr_ioread be checked or only pieces of torrents which have failed the mtime check when loading the resume file?

comment:5 Changed 11 years ago by charles

I don't think utorrent would be a guide for this -- shortly after the passage I quoted at the top, alus said he hadn't gotten around to implementing this in utorrent yet.

Still, I see your point. If we verify unconditionally, some unncessary IO and CPU is going to happen. It's probably better to use this logic: "if we're being asked to upload a piece, and its hashcheck mtime doesn't match our current mtime, then check the piece now, and if it fails, stop the torrent and reverify the whole thing."

comment:6 Changed 11 years ago by Longinus00

I had the same thinking, but when I started to play around with this I was reminded of andrefree's situation.

The most common scenario involving reverifys seems to be people who, for one reason or another, need to reverify all their torrents. Currently, the code to save the resume file will purposely write out a non matching mtime if the torrent has any unchecked pieces so they'd forever be hit with a performance penalty for that torrent. While that logic could be changed, it seems dangerous to consider files okay without testing them. I thought up two potential solutions to this problem that involved minimal user interaction.

  1. Because resume.c zeros out the "ispiecechecked" bitfield when mtimes don't match, each time you read a piece and check its hash, update the bitfield. On reads first test the bitfield before checking the hash. This is, unfortunately, unlikely to ever completely check a torrent but it might help cut the load on torrents which are seeding very quickly.
  2. Periodically scan the torrent list and verify the ones which failed the mtime test at load so that their "ispiecechecked" bitfields can be filled in. In order to not be too aggravating, it should only verify a torrent if no other verification is going on as the hash checks should protect us from bad pieces in the other torrents.

Combining these approaches is what I've played with so far, but there's still other issues. I don't know how it would be best to let the user know why their torrents were being verified (this is the main reason why I was hoping for more information about how utorrent was doing this). Maybe a suffix/prefix in the torrent display would be sufficient? Some sort of popup? Also, what happens should a user want to remove a torrent which failed the check from transmission and hasn't been checked yet? Should transmission offer to verify before removal? How would this (all the feedback) best be implemented in the case of the daemon and the rpc servers in the other clients?

comment:8 Changed 11 years ago by krx

  • Cc krx@… added

comment:9 Changed 11 years ago by Longinus00

I update my patch to the latest trunk and made tor->checkedPieces persistent. This lets us not use the invalid mtime method to inform future transmission instances about untrustworthy data. Another consequence of this is that the not enabling the auto-verify routine only incurs a slight penalty and so is merely optional.

There should still be some way of letting the user know that transmission is unsure of a torrent's contents because some people might think transmission is downloading bad files when in reality the corruption came from elsewhere.

comment:10 Changed 11 years ago by charles

  • Milestone changed from 2.00 to Sometime

comment:11 Changed 11 years ago by charles

  • Milestone changed from Sometime to 2.10

comment:12 follow-up: Changed 11 years ago by Astara

This wouldn't fix the problem I ran into (#3224) that was closed as a duplicate of another bug (#2336) this patch was thought to address.

In my situation, I transmission may come up and my torrents may have moved due to drives being plugged in or not, at the time of boot. Transmission comes up and if the torrent-save directory isn't present, it casts hundreds of completed torrents as "not present" -- 0% present. It should suspend with error until corrected -- but because of this, when I remount the drive, all those torrents are now in 0% and "unverified" states and the only way to get them to seeing again is a verify of each torrent -- a 4-5 hour process. The modify value wouldn't have changed -- they should all be the same when I remount the disk as when they were unmounted in the prior boot.

One thing that I was looking at for a workaround for some of these problems might be an 'override' command "F"orce" -- force a torrent back into verified and 100% downloaded state, and let operations proceed from there. It's a bit of a kludge, but due to removable disks and the long time it takes to verify torrents, it might not be unreasonable for the user to be able to specify a way to tell transmission that 1 or more torrents were really "ok", and it should just accept the user's word as to their state -- as it is the user's system -- they 'should' know...:-).

Given how long verifications take I'm not sure hiding verifications piecemeal on each transfer would be good for throughput. If a torrent *needs* to be verified -- mark it as such, and maybe do as you suggest -- start transfer and verify the needed pieces, but after the first piece starts, begin verify other pieces in background, one at a time, so that if any pieces became needed before they had been verified, they could be pushed to the front of the verification queue. Not the most trivial algorithm.

Alternatively, start off a low-priority background task to verify the whole torrent 'en mass' with no 'breaks' read in the whole torrent megabytes at a time to increase I/O efficiency. Meanwhile, continue as you suggest and verify pieces as needed while the background is finishing -- with any foreground task having higher cpu and disk priority -- so it can steal cycles from the background task when urgency demands.

Eh?

comment:13 in reply to: ↑ 12 Changed 11 years ago by Longinus00

Replying to Astara:

This wouldn't fix the problem I ran into (#3224) that was closed as a duplicate of another bug (#2336) this patch was thought to address.

In my situation, I transmission may come up and my torrents may have moved due to drives being plugged in or not, at the time of boot. Transmission comes up and if the torrent-save directory isn't present, it casts hundreds of completed torrents as "not present" -- 0% present. It should suspend with error until corrected -- but because of this, when I remount the drive, all those torrents are now in 0% and "unverified" states and the only way to get them to seeing again is a verify of each torrent -- a 4-5 hour process. The modify value wouldn't have changed -- they should all be the same when I remount the disk as when they were unmounted in the prior boot.

If you read through my patch or try it out you'll find that it should fix your problem.

comment:14 Changed 11 years ago by Longinus00

  • Owner changed from charles to Longinus00

comment:15 Changed 10 years ago by charles

  • Milestone changed from 2.10 to Sometime

Bumping to post-2.10 since that release is coming soon.

comment:16 Changed 10 years ago by charles

  • Milestone changed from Sometime to 2.20

comment:17 Changed 10 years ago by charles

Tickets #1017 and #3008 have been closed as subsets of this ticket.

comment:18 Changed 10 years ago by krx

  • Cc krx@… removed

Changed 10 years ago by Longinus00

Update to ensure clean patching

comment:19 Changed 10 years ago by lvella

I've applyed the patch against r11438, and it solved my problem of incomplete downloads being completely reverified after a system crash.

But I am wondering if the approach taken is the best for this unclean shutdown situation. Maybe there could be a periodical fsync of the files, followed by an fsync alternative .resume files, that would contain what was for sure downloaded and synced with the disk. After a crash, the same way completed downloads are for sure unchanged, the alternative .resume would give a figure of the surely complete and correct chunks. Only the divergence between the "offical" .resume and the checkpointed resume would have to be checked.

The idea may be a generalization of the approach taken here: each chunk would be in either of three states for Transmission: surely correct, possibly correct or missing. Before lazyVerify, no torrent would start before every chunk was either "surely correct" or "missing". After lazyVerify, torrents may start with pieces that are "possibly correct", and on demand being asserted as "surely correct". I am suggesting that "surely correct" information to be stored in non volatile medium, and this "sure" be asserted by periodical fsyncs of correctly hash verified chunks.

Last edited 10 years ago by lvella (previous) (diff)

comment:20 Changed 10 years ago by charles

Ticket #3653 has been closed as a duplicate of this ticket.

comment:21 follow-up: Changed 10 years ago by charles

Longinus00: in tr_cacheReadCheckPiece(), wouldn't we want to unconditionally check a piece if it's unchecked, instead of only when failedState==NONE? It seems like otherwise we could upload unchecked blocks to peers.

comment:22 Changed 10 years ago by charles

lazyVerify-2010-12-04-001.diff:

  • Add a per-piece time_t telling when we last tested the piece. This will get saved between sessions in the .resume file. This replaces mtimes in the resume file and replaces the tr_torrent.checkedPieces bitfield.
  • The only times we verify an entire torrent from beginning to end are (1) when adding a new torrent or (2) when the user presses "Verify Local Data"
  • Don't allow a torrent to be started or verified if the local data existed but has disappeared. Instead, call tr_torrentSetLocalError() telling the user to plug their external drives in and/or call "Set Location", or to remove + readd the torrent if they *really* want to redownload. By avoiding the call to tr_verifyAdd(), we avoid the problem of Verify clobbering tr_torrent.completion's fields when an external drive's unplugged.
  • If we're downloading a piece and it becomes complete, test it.
  • If we're uploading a piece, walk through each of the piece's files. If the piece has never been tested, test the piece. If we think the file is complete (so we're done downloading it) and the piece's testedTime is less than the file's mtime, test the piece. If the test fails, stop the torrent with a "hashcheck failed! Please Verify Local Data" error message.
Last edited 10 years ago by charles (previous) (diff)

comment:23 Changed 10 years ago by charles

new in lazyVerify-2010-12-005:

  • reorder tr_piece fields s.t. we save a byte on 64bit machines
  • tighten up the human-readable error messages

comment:24 Changed 10 years ago by charles

Ticket #3791 has been closed as a duplicate of this ticket.

comment:25 in reply to: ↑ 21 ; follow-ups: Changed 10 years ago by Longinus00

Replying to charles:

Longinus00: in tr_cacheReadCheckPiece(), wouldn't we want to unconditionally check a piece if it's unchecked, instead of only when failedState==NONE? It seems like otherwise we could upload unchecked blocks to peers.

The reason is because the only time tr_cacheReadCheckPiece gets called is for completed pieces which all get verified before being marked as "complete".

I seem to recall a discussion with you in IRC a ways back regarding saving a timestamp for every piece and how I felt it was unnecessary and didn't have any advantages over saving the checked status of every piece.

comment:26 in reply to: ↑ 25 Changed 10 years ago by charles

Replying to Longinus00:

I seem to recall a discussion with you in IRC a ways back regarding saving a timestamp for every piece and how I felt it was unnecessary and didn't have any advantages over saving the checked status of every piece.

I remember that, too. The main issue was with avoiding unnecessary rechecks when downloading, since a file's mtime would be continually changing. In these recent patches this is addressed addressed by tr_torrentPieceNeedsCheck() which controls whether or not a piece needs to be rechecked.

Last edited 10 years ago by charles (previous) (diff)

comment:27 Changed 10 years ago by charles

lazyVerify-2010-12-06.diff: simplify the Verify code.

comment:28 Changed 10 years ago by x190

Quote:

call tr_torrentSetLocalError() telling the user to plug their external drives in and/or call "Set Location", or to remove + readd the torrent if they *really* want to redownload.

We used to have this in Mac Client. We were given a drop down dialogue with a message stating "Unable to locate local data, please select download location." You could then select the proper location via a browser.

Changed 10 years ago by charles

comment:29 in reply to: ↑ 25 Changed 10 years ago by charles

Longinus00, there are a couple of other differences (besides the timestamp) between the two proposals that I think are worth discussion.

  1. The first implementation adds an autoverify mechanism along with RPC and C API mechanisms for it. The second implementation does not have this mechanism, and I'm not sure I see its purpose.
  1. By refusing to verify or start torrents whose contents have disappeared, the second implementation sidesteps several issues. The special cases for some failedState values go away. in fact, the entire failedState enum and its tr_stat field go away. Much of verify.c's code becomes unnecessary, and it resolves the issue of verify.c not clobbering the progress bitfield in these situations. The first patch solves this last point too, but by adding more code between verify.c and resume.c.

Would you be interested in discussing these, or in modifying the first proposed implementation to follow these points?

comment:30 Changed 10 years ago by charles

  • Owner changed from Longinus00 to charles
  • Status changed from new to assigned

Okay well I give up. I've been trying for weeks to get you engaged again on Transmission but it feels like you've already made up your mind and are only phoning in complaints.

comment:31 Changed 10 years ago by giovannibajo

  • Cc giovannibajo@… added

comment:32 Changed 10 years ago by charles

Added to trunk in r11506

comment:33 Changed 10 years ago by x190

Last edited 10 years ago by x190 (previous) (diff)

comment:34 Changed 10 years ago by charles

r11529: don't force a reverify after moving a torrent's contents on disk.

comment:35 Changed 10 years ago by charles

r11565: remove some of these noisy debug messages.

comment:36 follow-ups: Changed 10 years ago by x190

Transmission should stop and return the "uh, oh" message rather than re-creating a missing folder/file for which it had previously downloaded data. I'm referring to a sub-folder of a multi-folder torrent. As of 11565, Transmission recreates the sub-directory and continues on as if it still had all the data.

Message should say something like "A file or sub-folder [include specific name if possible] is missing! Return the data to the original location or deselect the item in the Inspector."

What needs to happen in the case that the user wishes to re-download the data??? Perhaps, hitting the resume button after the error has been thrown should clear previous bitmapping for that data?? OTY :).

See the last part of the following forum post for more details.

https://forum.transmissionbt.com/viewtopic.php?f=2&t=10983&p=51290#p51290

Last edited 10 years ago by x190 (previous) (diff)

comment:37 Changed 10 years ago by x190

Follow-up to above experiment: remove only some of the data.

Transfer completes and goes to seeding mode without [LAZY] realizing it is missing data. Only a user initiated "Verify Local Data" finds that data is missing. Suggested fix: Do not recreate data files/folders that were previously there.

comment:41 in reply to: ↑ 36 ; follow-up: Changed 10 years ago by charles

x190: how do I trigger the subfolder issue?

comment:42 in reply to: ↑ 41 Changed 10 years ago by x190

Replying to charles:

x190: how do I trigger the subfolder issue?

Download a torrent containing multiple files or folders to ~90% (this number should be more or less arbitrary). Remove one of these files/folders. [LAZY] will never notice and Transmission will simply recreate the file/folder that was removed when it needs to write data received. The completion level is not reset but continues on from the previous indicated level. This means that the torrent can and will complete to 100% and seed even tho' you removed 90% of at least one file. I of course do not know if any peers asked for those missing pieces during the completion process, but that should be immaterial to the issue anyway. If you want to recreate exactly what I did, return the file you removed when completion shows 95% indicated (let it overwrite), so that, at completion the file is only missing 5%. None of that should matter. The error ("Uh, oh") occurs the instant Transmission feels the need to recreate a file or folder for which it had previously downloaded and mapped data.

EDIT: I tested with a 68 GB torrent of which only about 14 GB was selected. Again, shouldn't matter.

2.13 stable doesn't get it right either but in a different kind of way. Please see Trac Ticket #3852 for this issue (2.13 stable).

It occurs to me we need to find a way for Transmission to recognize that data has been removed (the point of file/folder re-creation (11565) would be the absolute final "DO NOT ENTER" zone).

EDIT:

All torrents were active during testing.

charles,

Two scenarios that should be tested with 11565.

#1 Remove all data while torrent is active. #2 Unmount external drive while torrent is active.

As with above, one would hope to see the torrent stop and give the "Uh, oh" message.

Last edited 10 years ago by x190 (previous) (diff)

comment:43 in reply to: ↑ 36 ; follow-up: Changed 10 years ago by charles

I've confirmed that the behavior in comment:36 exists in 2.13 as well as trunk, so it's not really related to #2955. IMO we ought to move this discussion to #3852.

comment:44 in reply to: ↑ 43 Changed 10 years ago by x190

Replying to charles:

I've confirmed that the behavior in comment:36 exists in 2.13 as well as trunk, so it's not really related to #2955. IMO we ought to move this discussion to #3852.

I did notice one difference between 2.13 and trunk (11565). 2.13 did not recreate the missing file unless and until I was able to delete the file/folder and empty trash, as apparently, T was able to keep writing to it otherwise.

In any case, we need to find a way to (A)stop the torrent when data goes missing and (B) decide what error message to display, which, I guess, is where [LAZY] would become involved.

comment:45 Changed 10 years ago by charles

I did notice one difference between 2.13 and trunk (11565). 2.13 did not recreate the missing file unless and until I was able to delete the file/folder and empty trash, as apparently, T was able to keep writing to it otherwise.

That's just by chance of when an open file is flushed out of Transmission's cache of open files. If you were to do the same test enough times, odds are that trunk and 2.13 both create or not create the file about as often as each other.

(B) decide what error message to display, which, I guess, is where [LAZY] would become involved.

Maybe I'm being too anal with the trac tickets, but I don't see where #2955 enters into this. :)

comment:46 Changed 10 years ago by charles

Ticket #3855 has been closed as a duplicate of #1017 and a subset of this ticket.

comment:47 Changed 10 years ago by charles

  • Description modified (diff)
  • Summary changed from lazy torrent verification to instead of automatically verifying entire torrents, only verify pieces when they're needed

comment:48 Changed 10 years ago by charles

  • Description modified (diff)

comment:49 Changed 10 years ago by charles

  • Description modified (diff)

comment:50 Changed 10 years ago by charles

  • Summary changed from instead of automatically verifying entire torrents, only verify pieces when they're needed to verify pieces only when necessary, or when the user requests it.

comment:51 Changed 10 years ago by charles

  • Description modified (diff)

comment:52 Changed 10 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

This has been in the nightlies for a month now without any bug reports. Marking as fixed.

comment:53 Changed 10 years ago by jordan

r11763:

(trunk libT) #2955 "verify pieces only when necessary, or when the user requests it." -- add better support for old .resume files

Super-poussin says some readynas users are reporting high CPU oloads in 2.20 beta 1. My guess is this is due to pieces being reverified. Before now, the .resume files kept timestamps per-file, and 2.20 keeps timestamps per-piece. The problem is that 2.20 beta 1 didn't support reading the older per-file timetstamps from .resume files, so users loading up 2.20 beta 1 may find Transmission thinks none of the pieces in the torrents have been verified.

The fix is to have 2.20 beta 2 read the old per-file timestamps, so upgrading from 2.1x to 2.20 will go smoothly. That's what this commit does.

Unfortunately, the readynas users who have already been bitten by this will continue to be bitten until they reverify their files. 2.20 beta 1, which thinks all those pieces were never verified, has probably overwritten the .resume files from 2.1x... :(

comment:54 Changed 10 years ago by ijuxda

I have not fully examined lazyVerify-timestamp.diff and the subsequent related commits in this ticket, but there are two aspects of this implementation about which I would like some clarification.

  1. tr_torrentPieceNeedsCheck() (called from fillOutputBuffer() in libtransmission/peer-msgs.c) appears to be called whenever we receive a request from another peer for a block of data from a piece we already have. This is quite a frequent occurence. The function tr_torrentPieceNeedsCheck() itself calls tr_ioFindFileLocation() which is O(log N) (where N is the number of files in the torrent) and tr_cpFileIsComplete() which is O(M) (where M in the number of pieces in the file containing the piece passed to tr_torrentPieceNeedsCheck()).

    Then there is a comparison of the timeChecked field of the piece to the modification time ("mtime") of the file containing the piece. But this mtime will surely be larger than the one for the piece, since whenever some data is written to the file (i.e. of other pieces that make it up), the file's mtime will change, causing tr_torrentPieceNeedsCheck() to return TRUE and subsequently a needless and expensive call to tr_torrentCheckPiece().
  1. KEY_PROGRESS_CHECKTIME i.e. the time-checked key in the resume file has an integer time stamp for every piece in the torrent (corresponding to the timeChecked field). This is a lot of extra data to be reading from and writing to the resume file given that there are commonly several thousands (if not tens of thousands) of pieces in a torrent.

Perhaps I am mistaken in my cursory analysis, in which case I would appreciate being corrected, but these seem like quite significant performance penalties for all users, while these changes appear to be intended mainly to solve just the problem case in the ticket description (the RELATED PROBLEMS section). Or are there other use-cases that necessitate this particular implementation? If not, can the particular changes in the ticket be either improved, reverted, or made optional so that users not affected by the problem case can avoid the penalties?

comment:55 Changed 10 years ago by jordan

The function tr_torrentPieceNeedsCheck() itself calls tr_ioFindFileLocation() which is O(log N) (where N is the number of files in the torrent) and tr_cpFileIsComplete() which is O(M) (where M in the number of pieces in the file containing the piece passed to tr_torrentPieceNeedsCheck()).

tr_ioFindFileLocation() isn't showing up in my profiling at all, and after #3968 tr_cpFileIsComplete() has dropped from 1.77% of cpu use to 0.07%.

Improvements are always welcomed, so if you have ideas for this, that would be great. However #3957 seems to be a much more attractive optimization opportunity.

the file's mtime will change, causing tr_torrentPieceNeedsCheck() to return TRUE and subsequently a needless and expensive call to tr_torrentCheckPiece().

...which is why the call to tr_cpFileIsComplete() is necessary. It's counterproductive to compare the mtimes until after the file is finished downloading.

This is a lot of extra data to be reading from and writing to the resume file

Yes, it does. I haven't measured a significant change in IO, but it might be worse on (say) external drives, or with extremely large torrents. A more elegant (or, at least, a more terse) method of storing and retreiving those timestamps would be an improvement.

comment:58 Changed 10 years ago by jordan

r11814: improvements to the .resume file.

As pointed out by longinus00 and ijuxda, storing per-piece timestamps in the .resume file can involve a lot of overhead. This commit reduces the overhead by adding a couple of optimizations:

  • In cases where all or none of the files' pieces were checked after the file's mtime, we can safely fold all the pieces' "checked-at" timestamps into a single timestamp. This uses the same space as pre-2.20 resume files and is the most common use case.
  • Large numbers (such as unix timestamps) take up more space than small numbers when stored as integers in benc data, so when we store a file's "checked-at" timestamps piece-by-piece, use the oldest timestamp as a "baseline" number, then store all the timestamps as an offset from that baseline. This is ugly, but is measurably smaller...

Also, add documentation explaining this new format, and better explaining the pre-2.20 format.

Last edited 10 years ago by jordan (previous) (diff)

comment:59 follow-up: Changed 6 years ago by cfpp2p

The "just-in-time" verification works well but I've notice some shortcomings that will allow bad pieces to be sent to peers. This can happen when torrent's files might be replaced from an older source or when incomplete files are altered outside of transmission.

bool
tr_torrentPieceNeedsCheck (const tr_torrent * tor, tr_piece_index_t p)
{
  uint64_t unused;
  tr_file_index_t f;
  const tr_info * inf = tr_torrentInfo (tor);

  /* if we've never checked this piece, then it needs to be checked */
  if (!inf->pieces[p].timeChecked)
    return true;

  /* If we think we've completed one of the files in this piece,
   * but it's been modified since we last checked it,
   * then it needs to be rechecked */
  tr_ioFindFileLocation (tor, p, 0, &f, &unused);
  for (; f < inf->fileCount && pieceHasFile (p, &inf->files[f]); ++f)
    if (tr_cpFileIsComplete (&tor->completion, f))
      if (tr_torrentGetFileMTime (tor, f) > inf->pieces[p].timeChecked)
        return true;

  return false;
}

tr_torrentGetFileMTime isn't called to check all for a modified time stamp of completed piece's file(s) when either (or both) the beginning or ending piece boundary spans into an incomplete file (tr_cpFileIsComplete). If this incomplete file has somehow been altered outside transmission a bad piece is sent regardless of the time stamp. (A file rarely ends on even piece boundary.)

If a piece is fully contained in an altered incomplete file we never check for a modified time stamp at all and a bad piece is sent.

A piece completely within a file replaced from an older source is never checked since only times stamps greater than return true. > inf->pieces[p].timeChecked A bad piece is sent.

These situations of bad pieces sent I have reproduced.

comment:60 in reply to: ↑ 59 ; follow-up: Changed 6 years ago by x190

Replying to cfpp2p:

These situations of bad pieces sent I have reproduced.

Why hasn't this ticket been re-opened? Do you have a patch to fix?

comment:61 in reply to: ↑ 60 Changed 6 years ago by cfpp2p

Replying to x190:

Replying to cfpp2p:

These situations of bad pieces sent I have reproduced.

Why hasn't this ticket been re-opened? Do you have a patch to fix?

This ticket is listed as enhancement. Without this enhancement at all things would get worse yet. I'm unclear of the exact intentions or ability of the enhancement to prevent all instances of the user or system problems modifying files outside of transmission. Is this a bug or an incomplete enhancement? That's why I didn't reopen it.

I don't even have a conceptual version of a patch, although I moderately understand the processing flow. I came about testing this when looking at jordan's question at ticket:532#comment:188

I don't see how tr_torrentGetFileMTime() finds the correct time for a piece if, say, there are multiple files in the piece and also a part of the piece in the temp dir. Or for bonus fun, if both ends of the piece are in the temp dir, but there's one or more small, wanted file in the middle of the piece.

But to be clear, the issue of bad pieces sent is present in current trunk version.

Note: See TracTickets for help on using tickets.