[Mesa-dev] [PATCH] glsl: fix disk cache eviction issue

[Emil Velikov]

On 9 December 2016 at 05:58, Tapani Pälli <tapani.palli at intel.com> wrote:
> When 'evict_random_item' attempts to remove cache content to make more
> space, it may try to remove from new cache directory it just created
> which only has '.tmp' content at this phase and not proper cache files,
> this will cause eviction to fail and 'test_put_and_get' subtest
> assumptions about cache size and item count won't hold.
>
> Patch fixes this issue by adding 'exception' argument for random file
> matching so that algorithm won't pick the new directory.
>
Hi Tapani,

I've mentioned this over at #android-ia, but adding it here for posterity.


The issue itself is actually quite different - in my experiments with
importing SHA1 implementation to mesa, I've noticed that there bug
lies with a buggy (and/or undefined behaviour) SHA1 implementation
given the input data.

Namely:
As one observes the output of _mesa_sha1_compute they will notice that
it differs across implementations. This in itself combined with the
(afaict) correct assumption in test_put_and_get() that the beginning
for the SHA1blob_key_byte_zero


 above


As mentioned

> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> https://bugs.freedesktop.org/show_bug.cgi?id=97967
> ---
>  src/util/disk_cache.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
> index ac172ef..710c08e 100644
> --- a/src/util/disk_cache.c
> +++ b/src/util/disk_cache.c
> @@ -340,25 +340,30 @@ get_cache_file(struct disk_cache *cache, cache_key key)
>   *
>   * Obviously, the implementation here must closely match
>   * _get_cache_file above.
> + *
> + * returns the created directory name for the particular key
>  */
> -static void
> +static char*
>  make_cache_file_directory(struct disk_cache *cache, cache_key key)
>  {
>     char *dir;
> +   char *ret;
>     char buf[41];
>
>     _mesa_sha1_format(buf, key);
>
>     dir = ralloc_asprintf(cache, "%s/%c%c", cache->path, buf[0], buf[1]);
> +   ret = ralloc_asprintf(cache, "%c%c", buf[0], buf[1]);
>
>     mkdir_if_needed(dir);
>
>     ralloc_free(dir);
> +   return ret;
>  }
>
>  /* Given a directory path and predicate function, count all entries in
>   * that directory for which the predicate returns true. Then choose a
> - * random entry from among those counted.
> + * random entry from among those counted except 'exception' if given.
>   *
>   * Returns: A malloc'ed string for the path to the chosen file, (or
>   * NULL on any error). The caller should free the string when
> @@ -366,7 +371,8 @@ make_cache_file_directory(struct disk_cache *cache, cache_key key)
>   */
>  static char *
>  choose_random_file_matching(const char *dir_path,
> -                            bool (*predicate)(struct dirent *))
> +                            bool (*predicate)(struct dirent *),
> +                            char *exception)
>  {
>     DIR *dir;
>     struct dirent *entry;
> @@ -385,6 +391,8 @@ choose_random_file_matching(const char *dir_path,
>           break;
>        if (! predicate(entry))
>           continue;
> +      if (exception && strcmp(entry->d_name, exception) == 0)
> +         continue;
>
>        count++;
>     }
> @@ -449,7 +457,7 @@ unlink_random_file_from_directory(const char *path)
>     struct stat sb;
>     char *filename;
>
> -   filename = choose_random_file_matching(path, is_regular_non_tmp_file);
> +   filename = choose_random_file_matching(path, is_regular_non_tmp_file, NULL);
>     if (filename == NULL)
>        return 0;
>
> @@ -483,8 +491,11 @@ is_two_character_sub_directory(struct dirent *entry)
>     return true;
>  }
>
> +/*
> + * Evict item from anywhere expect from 'exception' if given.
> + */
>  static void
> -evict_random_item(struct disk_cache *cache)
> +evict_random_item(struct disk_cache *cache, char *exception)
>  {
>     const char hex[] = "0123456789abcde";
>     char *dir_path;
> @@ -517,8 +528,9 @@ evict_random_item(struct disk_cache *cache)
>      * tests to work, (which use an artificially-small cache to be able
>      * to force a single cached item to be evicted).
>      */
> -   dir_path = choose_random_file_matching(cache->path,
> -                                          is_two_character_sub_directory);
> +   dir_path =
> +      choose_random_file_matching(cache->path,
> +                                  is_two_character_sub_directory, exception);
>     if (dir_path == NULL)
>        return;
>
> @@ -540,6 +552,7 @@ disk_cache_put(struct disk_cache *cache,
>     size_t len;
>     char *filename = NULL, *filename_tmp = NULL;
>     const char *p = data;
> +   char *dir_name = NULL;
>
>     filename = get_cache_file(cache, key);
>     if (filename == NULL)
> @@ -560,7 +573,7 @@ disk_cache_put(struct disk_cache *cache,
>        if (errno != ENOENT)
>           goto done;
>
> -      make_cache_file_directory(cache, key);
> +      dir_name = make_cache_file_directory(cache, key);
>
>        fd = open(filename_tmp, O_WRONLY | O_CLOEXEC | O_CREAT, 0644);
>        if (fd == -1)
> @@ -591,10 +604,10 @@ disk_cache_put(struct disk_cache *cache,
>      * by some other process.
>      *
>      * Before we do that, if the cache is too large, evict something
> -    * else first.
> +    * else first (except from the path we just made).
>      */
>     if (*cache->size + size > cache->max_size)
> -      evict_random_item(cache);
> +      evict_random_item(cache, dir_name);
>
>     /* Now, finally, write out the contents to the temporary file, then
>      * rename them atomically to the destination filename, and also
> --
> 2.9.3
>