On 02/03/17 11:00, Timothy Arceri wrote:


On 01/03/17 23:58, Grazvydas Ignotas wrote:
On Wed, Mar 1, 2017 at 7:25 AM, Timothy Arceri <tarc...@itsqueeze.com>
wrote:
---
 src/util/disk_cache.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
index f5e1145..2a0edca 100644
--- a/src/util/disk_cache.c
+++ b/src/util/disk_cache.c
@@ -31,20 +31,21 @@
 #include <sys/file.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <pwd.h>
 #include <errno.h>
 #include <dirent.h>

+#include "util/crc32.h"
 #include "util/u_atomic.h"
 #include "util/mesa-sha1.h"
 #include "util/ralloc.h"
 #include "main/errors.h"

 #include "disk_cache.h"

 /* Number of bits to mask off from a cache key to get an index. */
 #define CACHE_INDEX_KEY_BITS 16

@@ -702,34 +703,48 @@ disk_cache_put(struct disk_cache *cache,
    /* OK, we're now on the hook to write out a file that we know is
     * not in the cache, and is also not being written out to the cache
     * by some other process.
     *
     * Before we do that, if the cache is too large, evict something
     * else first.
     */
    if (*cache->size + size > cache->max_size)
       evict_random_item(cache);

+   /* Create CRC of the data and store at the start of the file. We
will
+    * read this when restoring the cache and use it to check for
corruption.
+    */
+   uint32_t crc32 = util_hash_crc32(data, size);
+   size_t crc_size = sizeof(crc32);
+   for (len = 0; len < crc_size; len += ret) {
+      ret = write(fd, &crc32, crc_size - len);

I think you need to advance the data pointer if retrying. There is
also a case of EINTR where you should be retrying on error (ret ==
-1). Might be worth making a helper function.

So looking at the docs:

"       * read(2), readv(2), write(2), writev(2), and ioctl(2) calls on
         "slow" devices.  A "slow" device is one where the I/O call may
         block for an indefinite time, for example, a terminal, pipe, or
         socket.  If an I/O call on a slow device has already transferred
         some data by the time it is interrupted by a signal handler, then
         the call will return a success status (normally, the number of
         bytes transferred).  Note that a (local) disk is not a slow device
         according to this definition; I/O operations on disk devices are
         not interrupted by signals."


I assume this means I don't even need the loop? Can anyone with more
knowledge on this confirm?

Ok, so this could still happen on NFS so I've left it in but ignored the EINTR case. I'm happy to let it fallback to compiling rather than adding in more code for a use-case (home dirs on NFS) that is unlikely.





+      if (ret == -1) {
+         unlink(filename_tmp);
+         goto done;
+      }
+   }
+
    /* Now, finally, write out the contents to the temporary file, then
     * rename them atomically to the destination filename, and also
     * perform an atomic increment of the total cache size.
     */
    for (len = 0; len < size; len += ret) {
       ret = write(fd, p + len, size - len);
       if (ret == -1) {
          unlink(filename_tmp);
          goto done;
       }
    }

    rename(filename_tmp, filename);

+   size += crc_size;
    p_atomic_add(cache->size, size);

  done:
    if (fd_final != -1)
       close(fd_final);
    /* This close finally releases the flock, (now that the final dile
     * has been renamed into place and the size has been added).
     */
    if (fd != -1)
       close(fd);
@@ -758,31 +773,47 @@ disk_cache_get(struct disk_cache *cache,
cache_key key, size_t *size)
    if (fd == -1)
       goto fail;

    if (fstat(fd, &sb) == -1)
       goto fail;

    data = malloc(sb.st_size);
    if (data == NULL)
       goto fail;

-   for (len = 0; len < sb.st_size; len += ret) {
-      ret = read(fd, data + len, sb.st_size - len);
+   /* Load the CRC that was created when the file was written. */
+   uint32_t crc32;
+   size_t crc_size = sizeof(crc32);
+   assert(sb.st_size > crc_size);
+   for (len = 0; len < crc_size; len += ret) {
+      ret = read(fd, &crc32 + len, crc_size - len);

&crc32 + len doesn't look right, it's a uint32_t * pointer. EINTR
handling is also missing.

GraÅžvydas

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to