Re: [PATCH] Use FIEMAP to sort .list files before scanning

[Guillem Jover <guillem@debian.org>]

Hi Morten!

I applied the patch some days ago with some modifications, which is
going to be part of 1.15.6 targetting experimental. Thanks for the
patch!

  <http://git.debian.org/?p=dpkg/dpkg.git;a=commitdiff;h=8e31b0f0>

On Tue, 2009-11-03 at 16:11:46 +0100, Morten Hustveit wrote:
> diff --git a/src/filesdb.c b/src/filesdb.c
> index 0cc0791..a2eda65 100644
> --- a/src/filesdb.c
> +++ b/src/filesdb.c
> @@ -254,10 +262,29 @@ ensure_packagefiles_available(struct pkginfo *pkg)
>    pkg->clientdata->fileslistvalid= 1;
>  }
>  
> +#if HAVE_LINUX_FIEMAP_H
> +int
> +pkg_sorter_by_listfile_phys_offset(const void *a, const void *b) {
> +  struct pkginfo *const *lhs = a;
> +  struct pkginfo *const *rhs = b;

Here I just used a pointer (instead of a pointer to pointer), and did
the needed cast from a and b.

> +  /* We can't simply subtract, because the difference may be greater than INT_MAX.
> +   */
> +  if((*lhs)->clientdata->listfile_phys_offset < (*rhs)->clientdata->listfile_phys_offset)
> +    return -1;
> +  else
> +    return 1;
> +}
> +#endif

I refactored the code inside ensure_allinstfiles_available into a new
pkg_files_optimize_load() function to make it clearer. In addition this
made it easier to use posix_fadvise whenever FIEMAP is not available.

>  void ensure_allinstfiles_available(void) {
> -  struct pkgiterator *it;
>    struct pkginfo *pkg;
>    struct progress progress;
> +  struct pkg_array array;
> +  int i;
> +#if HAVE_LINUX_FIEMAP_H
> +  int blocksize;
> +#endif

This was missing initialization, or it could get garbage and then the
FIGETBSZ ioctl would not be perfomered.

>    if (allpackagesdone) return;
>    if (saidread<2) {
> @@ -267,14 +294,56 @@ void ensure_allinstfiles_available(void) {
>      progress_init(&progress, _("(Reading database ... "), max);
>    }
>  
> -  it= iterpkgstart();
> -  while ((pkg = iterpkgnext(it)) != NULL) {
> +  pkg_array_init_from_db(&array);
> +
> +  /* Sort packages by the physical location of their list files, so that
> +   * scanning them later will minimize disk drive head movements.
> +   */
> +#if HAVE_LINUX_FIEMAP_H
> +  for (i = 0; i < array.n_pkgs; i++) {
> +    pkg = array.pkgs[i];

There was a missing call to ensure_package_clientdata() here as in
some conditions clientdata is NULL.

> +    if(!pkg->clientdata->listfile_phys_offset
> +       && pkg->status != stat_notinstalled) {

I inverted the logic and did a continue here to avoid the nesting.

> +      struct {
> +        struct fiemap fiemap;
> +        struct fiemap_extent extent;
> +      } fm;
> +      const char *listfile;
> +      int fd;
> +
> +      pkg->clientdata->listfile_phys_offset = -1;
> +
> +      listfile= pkgadminfile(pkg, LISTFILE);
> +
> +      if(-1 == (fd= open(listfile, O_RDONLY)))
> +        continue;
> +
> +      if(!blocksize && -1 == ioctl(fd, FIGETBSZ, &blocksize))
> +        break;
> +
> +      memset(&fm, 0, sizeof(fm));
> +      fm.fiemap.fm_start = 0;
> +      fm.fiemap.fm_length = blocksize;
> +      fm.fiemap.fm_flags = 0;
> +      fm.fiemap.fm_extent_count = 1;
> +
> +      if(-1 != ioctl(fd, FS_IOC_FIEMAP, (unsigned long) &fm))
> +        pkg->clientdata->listfile_phys_offset = fm.fiemap.fm_extents[0].fe_physical;

Inverted the logic and switched the function call with the return
value check for some of the error checks. And minor missing space
additions.

> +      close(fd);
> +    }
> +  }
> +  pkg_array_sort(&array, pkg_sorter_by_listfile_phys_offset);
> +#endif
> +
> +  for (i = 0; i < array.n_pkgs; i++) {
> +    pkg = array.pkgs[i];
>      ensure_packagefiles_available(pkg);
>  
>      if (saidread == 1)
>        progress_step(&progress);
>    }
> -  iterpkgend(it);
> +

There was a missing pkg_array_destroy here.

>    allpackagesdone= 1;
>  
>    if (saidread==1) {

thanks,
guillem