Hi Noah,
Like the other patch this really needs a commit message.
If I understand the code correctly you are introducing another cache
just for prefetches separate from the fdcache. But this new cache is a
part of larger libarchive_fdcache so access to it is shared with the
existing one, it is just the eviction policy that changes for
prefetched files.
I don't have an opinion on the renamed metrics, maybe Frank can say if
it matters how and if they are renamed.
On Fri, 2021-07-02 at 14:24 -0400, Noah Sanci via Elfutils-devel wrote:
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 286c910a..06d03e72 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-06-28 Noah Sanci <nsanci@redhat.com>
> +
> + PR25978
> + * debuginfod.cxx: Added command line options
> + --fdcache-prefetch-fds/mbs and associated metrics/functionality.
And I believe the code is good, but I look ChangeLog entries a bit more
verbose so they can be used to check that the changes made were
intentional.
> @@ -538,10 +546,22 @@ parse_opt (int key, char *arg,
> break;
> case ARGP_KEY_FDCACHE_MINTMP:
> fdcache_mintmp = atol (arg);
> + if( fdcache_mintmp > 100 || fdcache_mintmp < 0 )
> + argp_failure(state, 1, EINVAL, "fdcache mintmp percent");
> break;
For example here. This is a good change, but I had to double check
because the rest of the patch is about other keys.
> case ARGP_KEY_ARG:
> source_paths.insert(string(arg));
> break;
> + case ARGP_KEY_FDCACHE_PREFETCH_FDS:
> + fdcache_prefetch_fds = atol(arg);
> + if ( fdcache_prefetch_fds <= 0)
> + argp_failure(state, 1, EINVAL, "fdcache prefetch fds");
> + break;
> + case ARGP_KEY_FDCACHE_PREFETCH_MBS:
> + fdcache_prefetch_mbs = atol(arg);
> + if ( fdcache_prefetch_mbs <= 0)
> + argp_failure(state, 1, EINVAL, "fdcache prefetch mbs");
> + break;
Is it intentional that '0' is not allowed?
> void intern(const string& a, const string& b, string fd, off_t sz,
> bool front_p)
> @@ -1221,7 +1249,17 @@ public:
> {
> unlink (i->fd.c_str());
> lru.erase(i);
> - inc_metric("fdcache_op_count","op","dequeue");
> + inc_metric("fdcache_op_count","op","lru_dequeue");
> + break; // must not continue iterating
> + }
> + }
> + for (auto i = prefetch.begin(); i < prefetch.end(); i++) //
> nuke preexisting copy in prefetch
This comment could go on its own line before the for statement to keep
the lines a little shorter.
> + {
> + if (i->archive == a && i->entry == b)
> + {
> + unlink (i->fd.c_str());
> + prefetch.erase(i);
> + inc_metric("fdcache_op_count","op","prefetch_dequeue")
> ;
> break; // must not continue iterating
> }
This is already in the existing code, but the names 'a' and 'b' are not
really that helpful. They are the archive and filename. If you know
that, then the code does make sense.
> @@ -1266,21 +1304,32 @@ public:
> fdcache_entry n = *i;
> lru.erase(i); // invalidates i, so no more iteration!
> lru.push_front(n);
> - inc_metric("fdcache_op_count","op","requeue_front");
> + inc_metric("fdcache_op_count","op","lru_requeue_front"
> );
> + fd = open(n.fd.c_str(), O_RDONLY); // NB: no problem
> if
> dup() fails; looks like cache miss
This comment could also be on its own line.
I don't understand it though. Where does the dup happen?
Did you mean if open () fails, it is no problem that fd becomes -1?
> + break;
> + }
> + }
> + for ( auto i = prefetch.begin(); fd == -1 && i <
> prefetch.end(); ++i)
Should this be fd != -1 ?
> + {
> + if (i->archive == a && i->entry == b)
> + { // found it; take the entry from the prefetch deque to
> the lru deque, since it has now been accessed.
> + fdcache_entry n = *i;
> + prefetch.erase(i);
> + lru.push_front(n);
> + inc_metric("fdcache_op_count","op","prefetch_access");
> fd = open(n.fd.c_str(), O_RDONLY); // NB: no problem
> if
> dup() fails; looks like cache miss
> break;
> }
> }
> }
> -
Please try to avoid spurious whitespace changes, they make the diffs
bigger than necessary.
> index 1ba42cf6..8945eb9b 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -212,6 +212,16 @@ $TMPDIR or \fB/tmp\fP filesystem. This is
> because that is where the
> most recently used extracted files are kept. Grooming cleans this
> cache.
>
> +.TP
> +.B "\-\-fdcache\-\-prefetch\-fds=NUM" "\-\-fdcache\-\-prefetch\-
> mbs=MB"
> +Configure how many file descriptors (fds) and megabytes (mbs) are
> +allocated to the prefetch portion of the fdcache. If unspecified,
> +values of \fB\-\-prefetch\-fds\fP and \fB\-\-prefetch\-mbs\fP depend
> +on concurrency of the system and on the available disk space on
> +the $TMPDIR. Allocating more to the prefetch cache will improve
> +performance in environments where different parts of several large
> +archives are being accessed.
If they are portions of the full fdcache shouldn't there be a check in
the code that the specified fdcache_prefetch_fds and
fdcache_prefetch_mbs aren't larger than fdcache_fds and fdcache_mbs? Or
maybe they should be given as percentages?
> +####################################################################
> ####
> +## PR25978
> +# Ensure that the fdcache options are working.
> +grep "prefetch fds" vlog$PORT1
> +grep "prefetch mbs" vlog$PORT1
> +grep "fdcache fds" vlog$PORT1
> +grep "fdcache mbs" vlog$PORT1
> +
> +wait_ready $PORT1 'fdcache_op_count{op="lru_enqueue"}' 28
> +wait_ready $PORT1 'fdcache_op_count{op="lru_evict"}' 24
> +wait_ready $PORT1 'fdcache_op_count{op="lru_probe_hit"}' 6
> +wait_ready $PORT1 'fdcache_op_count{op="lru_requeue_front"}' 11
> +wait_ready $PORT1 'fdcache_op_count{op="lru_probe_miss"}' 0
> +wait_ready $PORT1 'fdcache_prefetch_bytes' 0
> +wait_ready $PORT1 'fdcache_prefetch_count' 0
> +wait_ready $PORT1 'fdcache_op_count{op="prefetch_access"}' 4
> +wait_ready $PORT1 'fdcache_op_count{op="prefetch_enqueue"}' 4
> +wait_ready $PORT1 'fdcache_op_count{op="prefetch_evict"}' 0
> +
> ####################################################################
> ####
Could you add a comment why we are expecting these exact values?
Thanks,
Mark
@@ -1,3 +1,9 @@
+2021-06-28 Noah Sanci <nsanci@redhat.com>
+
+ PR25978
+ * debuginfod.cxx: Added command line options
+ --fdcache-prefetch-fds/mbs and associated metrics/functionality.
+
2021-06-03 Frank Ch. Eigler <fche@redhat.com>
PR27863
@@ -368,7 +368,13 @@ static const struct argp_option options[] =
{ "fdcache-prefetch", ARGP_KEY_FDCACHE_PREFETCH, "NUM", 0, "Number
of archive files to prefetch into fdcache.", 0 },
#define ARGP_KEY_FDCACHE_MINTMP 0x1004
{ "fdcache-mintmp", ARGP_KEY_FDCACHE_MINTMP, "NUM", 0, "Minimum
free space% on tmpdir.", 0 },
- { NULL, 0, NULL, 0, NULL, 0 }
+#define ARGP_KEY_FDCACHE_PREFETCH_MBS 0x1005
+ { "fdcache-prefetch-mbs", ARGP_KEY_FDCACHE_PREFETCH_MBS, "MB",
0,"Megabytes allocated to the \
+ prefetch cache.", 0},
+#define ARGP_KEY_FDCACHE_PREFETCH_FDS 0x1006
+ { "fdcache-prefetch-fds", ARGP_KEY_FDCACHE_PREFETCH_FDS, "NUM",
0,"Number of files allocated to the \
+ prefetch cache.", 0},
+ { NULL, 0, NULL, 0, NULL, 0 },
};
/* Short description of program. */
@@ -412,6 +418,8 @@ static long fdcache_fds;
static long fdcache_mbs;
static long fdcache_prefetch;
static long fdcache_mintmp;
+static long fdcache_prefetch_mbs;
+static long fdcache_prefetch_fds;
static string tmpdir;
static void set_metric(const string& key, double value);
@@ -538,10 +546,22 @@ parse_opt (int key, char *arg,
break;
case ARGP_KEY_FDCACHE_MINTMP:
fdcache_mintmp = atol (arg);
+ if( fdcache_mintmp > 100 || fdcache_mintmp < 0 )
+ argp_failure(state, 1, EINVAL, "fdcache mintmp percent");
break;
case ARGP_KEY_ARG:
source_paths.insert(string(arg));
break;
+ case ARGP_KEY_FDCACHE_PREFETCH_FDS:
+ fdcache_prefetch_fds = atol(arg);
+ if ( fdcache_prefetch_fds <= 0)
+ argp_failure(state, 1, EINVAL, "fdcache prefetch fds");
+ break;
+ case ARGP_KEY_FDCACHE_PREFETCH_MBS:
+ fdcache_prefetch_mbs = atol(arg);
+ if ( fdcache_prefetch_mbs <= 0)
+ argp_failure(state, 1, EINVAL, "fdcache prefetch mbs");
+ break;
// case 'h': argp_state_help (state, stderr,
ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
default: return ARGP_ERR_UNKNOWN;
}
@@ -1199,16 +1219,24 @@ private:
};
deque<fdcache_entry> lru; // @head: most recently used
long max_fds;
+ deque<fdcache_entry> prefetch; // prefetched
long max_mbs;
+ long max_prefetch_mbs;
+ long max_prefetch_fds;
public:
void set_metrics()
{
- double total_mb = 0.0;
+ double fdcache_mb = 0.0;
+ double prefetch_mb = 0.0;
for (auto i = lru.begin(); i < lru.end(); i++)
- total_mb += i->fd_size_mb;
- set_metric("fdcache_bytes", (int64_t)(total_mb*1024.0*1024.0));
- set_metric("fdcache_count", lru.size());
+ fdcache_mb += i->fd_size_mb;
+ for (auto j = prefetch.begin(); j < prefetch.end(); j++)
+ prefetch_mb += j->fd_size_mb;
+ set_metric("fdcache_bytes", fdcache_mb*1024.0*1024.0);
+ set_metric("fdcache_count", fdcache_mb);
+ set_metric("fdcache_prefetch_bytes", prefetch_mb*1024.0*1024.0);
+ set_metric("fdcache_prefetch_count", prefetch_mb);
}
void intern(const string& a, const string& b, string fd, off_t sz,
bool front_p)
@@ -1221,7 +1249,17 @@ public:
{
unlink (i->fd.c_str());
lru.erase(i);
- inc_metric("fdcache_op_count","op","dequeue");
+ inc_metric("fdcache_op_count","op","lru_dequeue");
+ break; // must not continue iterating
+ }
+ }
+ for (auto i = prefetch.begin(); i < prefetch.end(); i++) //
nuke preexisting copy in prefetch
+ {
+ if (i->archive == a && i->entry == b)
+ {
+ unlink (i->fd.c_str());
+ prefetch.erase(i);
+ inc_metric("fdcache_op_count","op","prefetch_dequeue");
break; // must not continue iterating
}
}
@@ -1229,13 +1267,13 @@ public:
fdcache_entry n = { a, b, fd, mb };
if (front_p)
{
- inc_metric("fdcache_op_count","op","enqueue_front");
+ inc_metric("fdcache_op_count","op","lru_enqueue");
lru.push_front(n);
}
else
{
- inc_metric("fdcache_op_count","op","enqueue_back");
- lru.push_back(n);
+ inc_metric("fdcache_op_count","op","prefetch_enqueue");
+ prefetch.push_front(n);
}
if (verbose > 3)
obatched(clog) << "fdcache interned a=" << a << " b=" << b
@@ -1248,10 +1286,10 @@ public:
{
inc_metric("fdcache_op_count","op","emerg-flush");
obatched(clog) << "fdcache emergency flush for filling tmpdir" << endl;
- this->limit(0, 0); // emergency flush
+ this->limit(0, 0, 0, 0); // emergency flush
}
else if (front_p)
- this->limit(max_fds, max_mbs); // age cache if required
+ this->limit(max_fds, max_mbs, max_prefetch_fds,
max_prefetch_mbs); // age cache if required
}
int lookup(const string& a, const string& b)
@@ -1266,21 +1304,32 @@ public:
fdcache_entry n = *i;
lru.erase(i); // invalidates i, so no more iteration!
lru.push_front(n);
- inc_metric("fdcache_op_count","op","requeue_front");
+ inc_metric("fdcache_op_count","op","lru_requeue_front");
+ fd = open(n.fd.c_str(), O_RDONLY); // NB: no problem if
dup() fails; looks like cache miss
+ break;
+ }
+ }
+ for ( auto i = prefetch.begin(); fd == -1 && i < prefetch.end(); ++i)
+ {
+ if (i->archive == a && i->entry == b)
+ { // found it; take the entry from the prefetch deque to
the lru deque, since it has now been accessed.
+ fdcache_entry n = *i;
+ prefetch.erase(i);
+ lru.push_front(n);
+ inc_metric("fdcache_op_count","op","prefetch_access");
fd = open(n.fd.c_str(), O_RDONLY); // NB: no problem if
dup() fails; looks like cache miss
break;
}
}
}
-
if (statfs_free_enough_p(tmpdir, "tmpdir", fdcache_mintmp))
{
inc_metric("fdcache_op_count","op","emerg-flush");
obatched(clog) << "fdcache emergency flush for filling tmpdir";
- this->limit(0, 0); // emergency flush
+ this->limit(0, 0, 0, 0); // emergency flush
}
else if (fd >= 0)
- this->limit(max_fds, max_mbs); // age cache if required
+ this->limit(max_fds, max_mbs, max_prefetch_fds,
max_prefetch_mbs); // age cache if required
return fd;
}
@@ -1292,7 +1341,15 @@ public:
{
if (i->archive == a && i->entry == b)
{
- inc_metric("fdcache_op_count","op","probe_hit");
+ inc_metric("fdcache_op_count","op","lru_probe_hit");
+ return true;
+ }
+ }
+ for (auto i = prefetch.begin(); i < prefetch.end(); i++)
+ {
+ if (i->archive == a && i->entry == b)
+ {
+ inc_metric("fdcache_op_count","op","prefetch_probe_hit");
return true;
}
}
@@ -1306,19 +1363,30 @@ public:
for (auto i = lru.begin(); i < lru.end(); i++)
{
if (i->archive == a && i->entry == b)
- { // found it; move it to head of lru
+ { // found it; erase it from lru
fdcache_entry n = *i;
lru.erase(i); // invalidates i, so no more iteration!
- inc_metric("fdcache_op_count","op","clear");
+ inc_metric("fdcache_op_count","op","lru_clear");
unlink (n.fd.c_str());
set_metrics();
return;
}
}
+ for (auto i = prefetch.begin(); i < prefetch.end(); i++)
+ {
+ if (i->archive == a && i->entry == b)
+ { // found it; erase it from lru
+ fdcache_entry n = *i;
+ prefetch.erase(i); // invalidates i, so no more iteration!
+ inc_metric("fdcache_op_count","op","prefetch_clear");
+ unlink (n.fd.c_str());
+ set_metrics();
+ return;
+ }
+ }
}
-
- void limit(long maxfds, long maxmbs, bool metrics_p = true)
+ void limit(long maxfds, long maxmbs, long maxprefetchfds, long
maxprefetchmbs , bool metrics_p = true)
{
if (verbose > 3 && (this->max_fds != maxfds || this->max_mbs != maxmbs))
obatched(clog) << "fdcache limited to maxfds=" << maxfds << "
maxmbs=" << maxmbs << endl;
@@ -1326,7 +1394,8 @@ public:
unique_lock<mutex> lock(fdcache_lock);
this->max_fds = maxfds;
this->max_mbs = maxmbs;
-
+ this->max_prefetch_fds = maxprefetchfds;
+ this->max_prefetch_mbs = maxprefetchmbs;
long total_fd = 0;
double total_mb = 0.0;
for (auto i = lru.begin(); i < lru.end(); i++)
@@ -1334,7 +1403,7 @@ public:
// accumulate totals from most recently used one going backward
total_fd ++;
total_mb += i->fd_size_mb;
- if (total_fd > max_fds || total_mb > max_mbs)
+ if (total_fd > this->max_fds || total_mb > this->max_mbs)
{
// found the cut here point!
@@ -1344,7 +1413,7 @@ public:
obatched(clog) << "fdcache evicted a=" <<
j->archive << " b=" << j->entry
<< " fd=" << j->fd << " mb=" <<
j->fd_size_mb << endl;
if (metrics_p)
- inc_metric("fdcache_op_count","op","evict");
+ inc_metric("fdcache_op_count","op","lru_evict");
unlink (j->fd.c_str());
}
@@ -1352,6 +1421,29 @@ public:
break;
}
}
+ total_fd = 0;
+ total_mb = 0.0;
+ for(auto i = prefetch.begin(); i < prefetch.end(); i++){
+ // accumulate totals from most recently used one going backward
+ total_fd ++;
+ total_mb += i->fd_size_mb;
+ if (total_fd > this->max_prefetch_fds || total_mb >
this->max_prefetch_mbs)
+ {
+ // found the cut here point!
+ for (auto j = i; j < prefetch.end(); j++) // close all
the fds from here on in
+ {
+ if (verbose > 3)
+ obatched(clog) << "fdcache evicted from prefetch
a=" << j->archive << " b=" << j->entry
+ << " fd=" << j->fd << " mb=" <<
j->fd_size_mb << endl;
+ if (metrics_p)
+ inc_metric("fdcache_op_count","op","prefetch_evict");
+ unlink (j->fd.c_str());
+ }
+
+ prefetch.erase(i, prefetch.end()); // erase the nodes generally
+ break;
+ }
+ }
if (metrics_p) set_metrics();
}
@@ -1360,7 +1452,7 @@ public:
{
// unlink any fdcache entries in $TMPDIR
// don't update metrics; those globals may be already destroyed
- limit(0, 0, false);
+ limit(0, 0, 0, 0, false);
}
};
static libarchive_fdcache fdcache;
@@ -1547,7 +1639,7 @@ handle_buildid_r_match (bool internal_req_p,
// responsible for unlinking it later.
fdcache.intern(b_source0, fn,
tmppath, archive_entry_size(e),
- false); // prefetched ones go to back of lru
+ false); // prefetched ones go to the prefetch cache
prefetch_count --;
close (fd); // we're not saving this fd to make a mhd-response from!
continue;
@@ -3293,8 +3385,8 @@ void groom()
sqlite3_db_release_memory(dbq); // ... for both connections
debuginfod_pool_groom(); // and release any debuginfod_client
objects we've been holding onto
- fdcache.limit(0,0); // release the fdcache contents
- fdcache.limit(fdcache_fds,fdcache_mbs); // restore status quo parameters
+ fdcache.limit(0,0,0,0); // release the fdcache contents
+ fdcache.limit(fdcache_fds, fdcache_mbs, fdcache_prefetch_fds,
fdcache_prefetch_mbs); // restore status quo parameters
clock_gettime (CLOCK_MONOTONIC, &ts_end);
double deltas = (ts_end.tv_sec - ts_start.tv_sec) + (ts_end.tv_nsec
- ts_start.tv_nsec)/1.e9;
@@ -3456,7 +3548,7 @@ main (int argc, char *argv[])
if (scan_archives.size()==0 && !scan_files && source_paths.size()>0)
obatched(clog) << "warning: without -F -R -U -Z, ignoring PATHs" << endl;
- fdcache.limit(fdcache_fds, fdcache_mbs);
+ fdcache.limit(fdcache_fds, fdcache_mbs, fdcache_prefetch_fds,
fdcache_prefetch_mbs);
(void) signal (SIGPIPE, SIG_IGN); // microhttpd can generate it
incidentally, ignore
(void) signal (SIGINT, signal_handler); // ^C
@@ -3600,6 +3692,9 @@ main (int argc, char *argv[])
obatched(clog) << "fdcache tmpdir " << tmpdir << endl;
obatched(clog) << "fdcache tmpdir min% " << fdcache_mintmp << endl;
obatched(clog) << "groom time " << groom_s << endl;
+ obatched(clog) << "prefetch fds " << fdcache_prefetch_fds << endl;
+ obatched(clog) << "prefetch mbs " << fdcache_prefetch_mbs << endl;
+
if (scan_archives.size()>0)
{
obatched ob(clog);
@@ -212,6 +212,16 @@ $TMPDIR or \fB/tmp\fP filesystem. This is
because that is where the
most recently used extracted files are kept. Grooming cleans this
cache.
+.TP
+.B "\-\-fdcache\-\-prefetch\-fds=NUM" "\-\-fdcache\-\-prefetch\-mbs=MB"
+Configure how many file descriptors (fds) and megabytes (mbs) are
+allocated to the prefetch portion of the fdcache. If unspecified,
+values of \fB\-\-prefetch\-fds\fP and \fB\-\-prefetch\-mbs\fP depend
+on concurrency of the system and on the available disk space on
+the $TMPDIR. Allocating more to the prefetch cache will improve
+performance in environments where different parts of several large
+archives are being accessed.
+
.TP
.B "\-\-fdcache\-mintmp=NUM"
Configure a disk space threshold for emergency flushing of the cache.
@@ -1,3 +1,10 @@
+2021-06-28 Noah Sanci <nsanci@redhat.com>
+
+ PR25978
+ * run-debuginfod-find.sh: Test to ensure options
+ fdcache-prefetch-fds/mbs are set. Check that inc_metric works for lru
+ and prefetch cache metrics.
+
2021-06-16 Frank Ch. Eigler <fche@redhat.com>
* run-debuginfod-find.sh: Fix intermittent groom/stale failure,
@@ -119,12 +119,15 @@ wait_ready()
fi
}
+FDCACHE_FDS=50
+FDCACHE_MBS=190
+PREFETCH_FDS=10
+PREFETCH_MBS=120
# create a bogus .rpm file to evoke a metric-visible error
# Use a cyclic symlink instead of chmod 000 to make sure even root
# would see an error (running the testsuite under root is NOT encouraged).
ln -s R/nothing.rpm R/nothing.rpm
-
-env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=
${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -R -d $DB -p
$PORT1 -t0 -g0 --fdcache-fds 1 --fdcache-mbs 2 --fdcache-mintmp 0 -Z
.tar.xz -Z .tar.bz2=bzcat -v R F Z L > vlog$PORT1 2>&1 &
+env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS=