Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5423 closed Bug (incomplete)

Remove detrimental POSIX_FADV_DONTNEED and F_NOCACHE hints

Reported by: x190 Owned by: jordan
Priority: Normal Milestone: None Set
Component: libtransmission Version: 2.80
Severity: Normal Keywords:
Cc:

Description

The time has come to remove POSIX_FADV_DONTNEED from verify.c and its SYS_DARWIN equivalent F_NOCACHE from fdlimit.c which also ends up in verify.c via tr_open_file_for_scanning() as their inclusion was never valid to begin with.

We definitely do want to keep this in memory, if possible, as it will facilitate subsequent seeding.

Further discussion:

https://forum.transmissionbt.com/viewtopic.php?f=1&t=14221#p65456

r14125 verify.c Line 98-100:

- #if defined HAVE_POSIX_FADVISE && defined POSIX_FADV_DONTNEED
-              posix_fadvise (fd, filePos, bytesThisPass, POSIX_FADV_DONTNEED);
- #endif

And for SYS_DARWIN:

r14125 fdlimit.c Line 262:

- fcntl (fd, F_NOCACHE, 1);

Change History (12)

comment:1 follow-up: Changed 9 years ago by jordan

  • Resolution set to incomplete
  • Status changed from new to closed

I would rather see some numbers on this than sweeping low-information statements about "the time has come" and "never valid to begin with."

Basically I see this as a pathological variation of bug #5102. It's one thing to not remove recently-loaded pieces from the cache when we're in the middle of uploading. It's another thing to choose to keep the entire torrent in memory.

  1. Consider the use case of a 4.3 GB torrent. The user hits 'verify'. Are the 4.3 GB all going to stay in the cache? No. Are the pieces that stay in the cache the ones that the next peer will need? Probably not. Does this reintroduce the 'Inactive Memory' issue? I see no stats on that here.
  1. Also consider the case where you have a dozen torrents actively seeding when you hit 'verify' on the 4.3 GB torrent. If we flood the cache with the contents of the 4.3 GB torrent, what's that going to do to the cache hit/miss stats on the other torrents?

comment:2 in reply to: ↑ 1 Changed 9 years ago by x190

Replying to jordan:

choose to keep the entire torrent in memory.

We are not doing any such thing. Memory management, in this context, is handled entirely by the OS. F_NOCACHE should only be used when it is known the data will never be needed again.

Consider the use case of a 4.3 GB torrent. The user hits 'verify'. Are the 4.3 GB all going to stay in the cache?

Again that is not the application's concern and will be handled by the OS. Modern Mac's come with 8 GB RAM standard and many upgrade to 16 or even 32GB when ordering a new computer.

Does this reintroduce the 'Inactive Memory' issue?

This was always a red herring for OS X at least. See Apple's documentation.

If we flood the cache with the contents of the 4.3 GB torrent...

We are doing no such thing. It is handled by the OS in its infinite pre-programmed wisdom. Our only concern is to flag data with F_NOCACHE that we know we will never need again to help the OS decide which data to clear from the cache first, if space is needed. Data which we will be seeding does not meet that criterion.

I believe that much or all of what I have said applies to POSIX_FADV_DONTNEED for Linux as well, afaik. Please note as well that POSIX_FADV_DONTNEED is only found in verify.c, while its SYS_DARWIN equivalent F_NOCACHE is much more exposed by being included in fdlimit.c's tr_set_file_for_single_pass(), which is very inconsistent. If certain specific files qualify as only needed once, then these hints should be defined for them only (tr_variantToFile()?).

That's my story and I'm sticking to it---for now, anyway. :-)

comment:3 Changed 9 years ago by x190

  • Resolution incomplete deleted
  • Status changed from closed to reopened

comment:4 Changed 9 years ago by jordan

  • Resolution set to incomplete
  • Status changed from reopened to closed

When I asked for numbers rather than sweeping low-information statements, I wasn't requesting a response based on "that is not the application's concern" and "it is handled by the OS in its infinite pre-programmed wisdom."

For example, the question of what happens to the prefetched pieces for torrents A, B, and C when we hit "Verify Local Data" on torrent D. The change you're suggesting has the potential to blow away that prefetch data. To handwave this away with "it's not our concern" is not productive.

comment:5 Changed 9 years ago by x190

https://forum.transmissionbt.com/viewtopic.php?f=1&t=14221&start=15#p65492

So it's doubly crap: it's *wrong*, and it didn't actually fix anything to begin with.
---Linus Torvalds---

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

comment:6 Changed 9 years ago by x190

  • Resolution incomplete deleted
  • Status changed from closed to reopened

comment:7 Changed 9 years ago by x190

Due to the following code, OS caching is completely disabled on OS X.
r14125 fdlimit.c Line 262:


fcntl (fd, F_NOCACHE, 1);

This results in drastically longer verification times (46 secs for 115MB vs only 17 secs for 906MB, for example) and io reads numbering in the 100's as opposed to a mere handful (10-15/sec vs 400-500/sec, for example). Seeding similarly involves highly unnecessary disk reads as much of that data could have been cached by the OS. "* Also, disable OS-level caching because "inactive memory" angers users. */" Apparently, it only angered uninformed OS X, not Linux, users who felt they knew more about memory management than Apple's engineers. The issue is, in fact, unrelated to 'inactive memory' as that is controlled by the OS. OS X can operate quite efficiently, if required, on next to no FM, as it shuffles memory around between active/VM/inactive/FM.

Linux, while not similarily totally broken, still needs to have the POSIX_FADV_DONTNEED call removed from verify.c.

Think of the children/flowers/flower children, for god's sake man, or as your hero Linus would say, "So it's doubly crap: it's *wrong*, and it didn't actually fix anything to begin with."

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

comment:8 follow-up: Changed 9 years ago by jordan

Replying to x190:

Due to the following code, OS caching is completely disabled on OS X.

No it's not. OS caching is disabled for data that we load during the "Verify local data" pass. It's enabled for other piece data, such as blocks that we load to seed to peers.

This results in drastically longer verification times (46 secs for 115MB vs only 17 secs for 906MB, for example)

Cold-verifying a 248.9 MB torrent on my machine takes 14 seconds in stock 2.81 and 12 seconds in trunk + this ticket. The actual speeds are 72922271 and 84141082 bytes per second, so the latter is about 0.15 times faster.

Your description describes stock speeds of 2.5 MB/s (115 MB in 46 seconds) and trunk + this ticket of 53 MB/s (906 MB in 17 seconds), about 21 times faster.

There's a big difference between 0.15x and 21x faster, so I have to wonder about your methodology. Verifying the same torrent again and again might show speedups; however, it begs the question "is this an actual use case, or is this something that I can use to torture the numbers?" Moreover it does not address the common use case I asked about in comment:2 and comment:4.

A more useful benchmark, for example, would be measuring the number of cache hits to misses across the whole app when A, B, and C are actively seeding and D gets its local data verified.

Think of the children/flowers/flower children, for god's sake man, or as your hero Linus would say, "So it's doubly crap: it's *wrong*, and it didn't actually fix anything to begin with."

I've already asked you twice to avoid low-information arguments in this ticket and am done discussing this with you.

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

comment:9 Changed 9 years ago by jordan

  • Resolution set to incomplete
  • Status changed from reopened to closed

comment:10 in reply to: ↑ 8 Changed 9 years ago by x190

Replying to jordan:

Replying to x190:

Due to the following code, OS caching is completely disabled on OS X.

No it's not. OS caching is disabled for data that we load during the "Verify local data" pass. It's enabled for other piece data, such as blocks that we load to seed to peers.

The code seems to tell a different story.

fdlimit.c:

static int
cached_file_open (struct tr_cached_file  * o,
                  const char             * filename,
                  bool                     writable,
                  tr_preallocation_mode    allocation,
                  uint64_t                 file_size)
...

  /* Many (most?) clients request blocks in ascending order,
   * so increase the readahead buffer.
-> * Also, disable OS-level caching because "inactive memory" angers users. */
  tr_set_file_for_single_pass (o->fd);

  return 0;
}

void
tr_set_file_for_single_pass( int fd )
{
    if( fd >= 0 )
    {
        /* Set hints about the lookahead buffer and caching. It's okay
           for these to fail silently, so don't let them affect errno */
        const int err = errno;
#ifdef HAVE_POSIX_FADVISE
  ----> /* Note: POSIX_FADV_DONTNEED was not defined here, so only SYS_DARWIN
         * is affected---why? */
        posix_fadvise( fd, 0, 0, POSIX_FADV_SEQUENTIAL ); 
#endif
#ifdef SYS_DARWIN
        fcntl( fd, F_RDAHEAD, 1 );
 -----> fcntl( fd, F_NOCACHE, 1 ); <-----
#endif
        errno = err;
    }
}

The actual speeds are 72922271 and 84141082 bytes per second, so the latter is about 0.15 times faster.

IMO, a 15% speed gain and reduced disk reads are a worthy goal.

speeds of 2.5 MB/s (115 MB in 46 seconds) and trunk + this ticket of 53 MB/s (906 MB in 17 seconds)

I should have quoted the file size of 1890 MB in the first example rather than data downloaded. That gives 41 MB/s vs 53 MB/s, still a very impressive gain of 30%.

it does not address the common use case I asked about in comment:2 and comment:4.

Wouldn't POSIX_FADV_WILLNEED and F_RDADVISE take precedence? They would not be very useful otherwise. Anyway, they are never guaranteed and involve very small amounts of data relative to, potentially, GBs that will be needed for seeding and/or verification. Also, see comment:11 below.

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

comment:11 Changed 9 years ago by x190

Allowed to use cache while downloading, then verified:
2013-07-23 01:36:36 -0600 verify.c:170 [Debug] Verification is done. It took 6 seconds to verify 444014131 bytes (63430590 bytes per second)

This is 74MB/sec, which is more than 50% better than my best non- or partially-cached file verification time. FYI, verification, in itself, does not 'flood the cache' in the sense of using free memory on OS X. Only if you repeatedly verify the same torrent will the OS then start to allocate chunks of about 100MB.

Just completed with caching enabled: 2013-07-23 02:51:42 -0600 verify.c:170 [Debug] Verification is done. It took 10 seconds to verify 1018777295 bytes (92616117 bytes per second)

Even better, at 88 MB/s, 80% gain over non-cached verification speeds. I also monitored disk reads while verifying and while I can't provide precise hit numbers, the results were obvious with IO reads going from 0 to ~1/10th of non-cached read numbers measured as reads in/sec. It would be logical to assume that similar benefits would accrue to the seeding process, allowing for other dynamic factors of course.

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

comment:12 Changed 9 years ago by x190

2013-07-24 04:02:17 -0600 verify.c:170 [Debug] Verification is done. It took 3 seconds to verify 429106031 bytes (107276507 bytes per second)

143 MB/s --- no disk reads

Note: See TracTickets for help on using tickets.