[Libguestfs] [PATCH 4/4] lib: qemu: Memoize qemu feature detection.

[Pino Toscano]

On Thursday 12 May 2016 22:26:25 Richard W.M. Jones wrote:
> qemu feature detection takes about 95ms on my laptop.  The overhead is
> almost all due to the time taken by the glibc link loader opening the
> 170+ libraries that qemu is linked to (×2 because we need to run qemu
> twice).
> 
> Fixing that is seriously hard work.
> 
> Therefore memoize the results of guestfs_int_test_qemu.
> 
> This is keyed on the size and mtime of the qemu binary, so if the user
> changes the qemu binary (eg. setting LIBGUESTFS_HV) we discard the
> memoized result and rerun the qemu commands.  There is also a
> generation number so we can bump the generation in future versions of
> libguestfs to invalidate all previously cached data.
> 
> The memo is stored in the supermin cache directory (eg. /var/tmp/.guestfs-*)
> in the files:
> 
>   qemu.stat     Result of 'stat(2)' of the qemu binary
>   qemu.help     qemu -help output
>   qemu.devices  qemu -devices ? output
> 
> Note these files are only stored when using the 'direct' backend.  For
> the libvirt backend, libvirt itself memoizes this data in its own
> place.
> ---

The idea looks ok, just one note below.

>  src/qemu.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 146 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu.c b/src/qemu.c
> index feb8bd6..259a6fd 100644
> --- a/src/qemu.c
> +++ b/src/qemu.c
> @@ -58,23 +58,163 @@ struct qemu_data {
>                               guestfs_int_qemu_supports_virtio_scsi */
>  };
>  
> +static int test_qemu (guestfs_h *g, struct qemu_data *data);
>  static void parse_qemu_version (guestfs_h *g, struct qemu_data *data);
>  static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
>  
> +/* This is saved in the qemu.stat file, so if we decide to change the
> + * test_qemu memoization format/data in future, we should increment
> + * this to discard any memoized data cached by previous versions of
> + * libguestfs.
> + */
> +#define MEMO_GENERATION 1
> +
>  /**
>   * Test qemu binary (or wrapper) runs, and do C<qemu -help> so we know
>   * the version of qemu what options this qemu supports, and
>   * C<qemu -device ?> so we know what devices are available.
> + *
> + * This caches the results in the cachedir so that as long as the qemu
> + * binary does not change, calling this is effectively free.
>   */
>  struct qemu_data *
>  guestfs_int_test_qemu (guestfs_h *g)
>  {
> +  struct qemu_data *data;
> +  struct stat statbuf;
> +  CLEANUP_FREE char *cachedir = NULL, *qemu_stat_filename = NULL,
> +    *qemu_help_filename = NULL, *qemu_devices_filename = NULL;
> +  FILE *fp;
> +  int generation;
> +  uint64_t prev_size, prev_mtime;
> +
> +  if (stat (g->hv, &statbuf) == -1) {
> +    perrorf (g, "stat: %s", g->hv);
> +    return NULL;
> +  }
> +
> +  cachedir = guestfs_int_lazy_make_supermin_appliance_dir (g);
> +  if (cachedir == NULL)
> +    return NULL;
> +
> +  qemu_stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir);
> +  qemu_help_filename = safe_asprintf (g, "%s/qemu.help", cachedir);
> +  qemu_devices_filename = safe_asprintf (g, "%s/qemu.devices", cachedir);

Instead of writing the generation number to the stat file, what about
doing a simplier approach, i.e. add it to the filenames:
  - $cachedir/qemu.$generation.stat
  - $cachedir/qemu.$generation.help
  - etc

Less data to read, fopen will just open the data for the right
generation.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160518/c972456a/attachment.sig>