On 9 December 2016 at 05:58, Tapani Pälli <[email protected]> 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 <[email protected]> > 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 > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
