PR: 25978

[Mark Wielaard]

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