flush modified pages only in write mode for mmap files

Author: JanSellner

blosc/blosc2-stdio.c

@@ -466,17 +466,19 @@ int blosc2_stdio_mmap_destroy(void* params) {
   int err = 0;
 
 #if defined(_WIN32)
-  /* Ensure modified pages are written to disk */
-  if (!FlushViewOfFile(mmap_file->addr, mmap_file->file_size)) {
-    _print_last_error();
-    BLOSC_TRACE_ERROR("Cannot flush the memory-mapped view to disk.");
-    err = -1;
-  }
-  HANDLE file_handle = (HANDLE) _get_osfhandle(mmap_file->fd);
-  if (!FlushFileBuffers(file_handle)) {
-    _print_last_error();
-    BLOSC_TRACE_ERROR("Cannot flush the memory-mapped file to disk.");
-    err = -1;
+  if (mmap_file->access_flags == PAGE_READWRITE) {
+    /* Ensure modified pages are written to disk */
+    if (!FlushViewOfFile(mmap_file->addr, mmap_file->file_size)) {
+      _print_last_error();
+      BLOSC_TRACE_ERROR("Cannot flush the memory-mapped view to disk.");
+      err = -1;
+    }
+    HANDLE file_handle = (HANDLE) _get_osfhandle(mmap_file->fd);
+    if (!FlushFileBuffers(file_handle)) {
+      _print_last_error();
+      BLOSC_TRACE_ERROR("Cannot flush the memory-mapped file to disk.");
+      err = -1;
+    }
   }
 
   if (!UnmapViewOfFile(mmap_file->addr)) {
@@ -496,13 +498,15 @@ int blosc2_stdio_mmap_destroy(void* params) {
     err = -1;
   }
 #else
-  /* Ensure modified pages are written to disk */
-  /* This is important since not every munmap implementation flushes modified pages to disk
-  (e.g.: https://nfs.sourceforge.net/#faq_d8) */
-  int rc = msync(mmap_file->addr, mmap_file->file_size, MS_SYNC);
-  if (rc < 0) {
-    BLOSC_TRACE_ERROR("Cannot sync the memory-mapped file to disk (error: %s).", strerror(errno));
-    err = -1;
+  if ((mmap_file->access_flags & PROT_WRITE) && !mmap_file->is_memory_only) {
+    /* Ensure modified pages are written to disk */
+    /* This is important since not every munmap implementation flushes modified pages to disk
+    (e.g.: https://nfs.sourceforge.net/#faq_d8) */
+    int rc = msync(mmap_file->addr, mmap_file->file_size, MS_SYNC);
+    if (rc < 0) {
+      BLOSC_TRACE_ERROR("Cannot sync the memory-mapped file to disk (error: %s).", strerror(errno));
+      err = -1;
+    }
   }
 
   if (munmap(mmap_file->addr, mmap_file->mapping_size) < 0) {

blosc/schunk.c

@@ -484,6 +484,8 @@ int64_t blosc2_schunk_append_file(blosc2_schunk* schunk, const char* urlpath) {
 
 /* Free all memory from a super-chunk. */
 int blosc2_schunk_free(blosc2_schunk *schunk) {
+  int err = 0;
+
   if (schunk->data != NULL) {
     for (int i = 0; i < schunk->nchunks; i++) {
       free(schunk->data[i]);
@@ -516,6 +518,7 @@ int blosc2_schunk_free(blosc2_schunk *schunk) {
       int rc = io_cb->destroy(schunk->storage->io->params);
       if (rc < 0) {
         BLOSC_TRACE_ERROR("Could not free the I/O resources.");
+        err = 1;
       }
     }
 
@@ -546,7 +549,7 @@ int blosc2_schunk_free(blosc2_schunk *schunk) {
 
   free(schunk);
 
-  return 0;
+  return err;
 }
 
 

tests/test_mmap.c

@@ -99,32 +99,37 @@ CUTEST_TEST_TEST(mmap) {
   cbytes = blosc2_schunk_append_buffer(schunk_write_mmap, data_buffer2, sizeof(data_buffer2));
   CUTEST_ASSERT("Could not write second chunk", cbytes > 0);
 
-  blosc2_schunk_free(schunk_write_default);
-  blosc2_schunk_free(schunk_write_mmap);
+  CUTEST_ASSERT("Could not free the schunk ressources", blosc2_schunk_free(schunk_write_default) == 0);
+  CUTEST_ASSERT("Could not free the schunk ressources", blosc2_schunk_free(schunk_write_mmap) == 0);
 
   /* The compressed file content should not depend on the I/O which created it */
   CUTEST_ASSERT("Files are not identical", are_files_identical(urlpath_default, urlpath_mmap));
 
   /* Read the schunk data back again (using mmap) */
-  mmap_file = BLOSC2_STDIO_MMAP_DEFAULTS;
-  mmap_file.mode = "r";
-  mmap_file.initial_mapping_size = initial_mapping_size;
-  io.params = &mmap_file;
-  blosc2_schunk* schunk_read = blosc2_schunk_open_udio(urlpath_mmap, &io);
-  CUTEST_ASSERT("Mismatch in number of chunks", schunk_read->nchunks == 2);
-
-  float* chunk_data = (float*)malloc(schunk_read->chunksize);
-  int dsize = blosc2_schunk_decompress_chunk(schunk_read, 0, chunk_data, schunk_read->chunksize);
-  CUTEST_ASSERT("Size of decompressed chunk 1 does not match", dsize == sizeof(data_buffer));
-  CUTEST_ASSERT("Value 1 of chunk 1 is wrong", fabs(chunk_data[0] - 0.1) < 1e-6);
-  CUTEST_ASSERT("Value 2 of chunk 1 is wrong", fabs(chunk_data[1] - 0.2) < 1e-6);
-
-  dsize = blosc2_schunk_decompress_chunk(schunk_read, 1, chunk_data, schunk_read->chunksize);
-  CUTEST_ASSERT("Size of decompressed chunk 1 does not match", dsize == sizeof(data_buffer2));
-  CUTEST_ASSERT("Value 1 of chunk 2 is wrong", fabs(chunk_data[0] - 0.3) < 1e-6);
-  CUTEST_ASSERT("Value 2 of chunk 2 is wrong", fabs(chunk_data[1] - 0.4) < 1e-6);
-
-  blosc2_schunk_free(schunk_read);
+  int n_repeated_reads = 2;  // Make sure reading the same file again does not lead to any problems
+  int dsize;
+  float* chunk_data;
+  for (int i = 0; i < n_repeated_reads; i++) {
+    mmap_file = BLOSC2_STDIO_MMAP_DEFAULTS;
+    mmap_file.mode = "r";
+    mmap_file.initial_mapping_size = initial_mapping_size;
+    io.params = &mmap_file;
+    blosc2_schunk* schunk_read = blosc2_schunk_open_udio(urlpath_mmap, &io);
+    CUTEST_ASSERT("Mismatch in number of chunks", schunk_read->nchunks == 2);
+
+    chunk_data = (float*)malloc(schunk_read->chunksize);
+    dsize = blosc2_schunk_decompress_chunk(schunk_read, 0, chunk_data, schunk_read->chunksize);
+    CUTEST_ASSERT("Size of decompressed chunk 1 does not match", dsize == sizeof(data_buffer));
+    CUTEST_ASSERT("Value 1 of chunk 1 is wrong", fabs(chunk_data[0] - 0.1) < 1e-6);
+    CUTEST_ASSERT("Value 2 of chunk 1 is wrong", fabs(chunk_data[1] - 0.2) < 1e-6);
+
+    dsize = blosc2_schunk_decompress_chunk(schunk_read, 1, chunk_data, schunk_read->chunksize);
+    CUTEST_ASSERT("Size of decompressed chunk 1 does not match", dsize == sizeof(data_buffer2));
+    CUTEST_ASSERT("Value 1 of chunk 2 is wrong", fabs(chunk_data[0] - 0.3) < 1e-6);
+    CUTEST_ASSERT("Value 2 of chunk 2 is wrong", fabs(chunk_data[1] - 0.4) < 1e-6);
+
+    CUTEST_ASSERT("Could not free the schunk ressources", blosc2_schunk_free(schunk_read) == 0);
+  }
 
 #if defined(__linux__)
   /* Append some data to the existing schunk in memory (does not work on Windows) */
@@ -148,7 +153,7 @@ CUTEST_TEST_TEST(mmap) {
   CUTEST_ASSERT("Value 1 of chunk 2 is wrong", fabs(chunk_data[0] - 0.5) < 1e-6);
   CUTEST_ASSERT("Value 2 of chunk 2 is wrong", fabs(chunk_data[1] - 0.6) < 1e-6);
 
-  blosc2_schunk_free(schunk_memory);
+  CUTEST_ASSERT("Could not free the schunk ressources", blosc2_schunk_free(schunk_memory) == 0);
   CUTEST_ASSERT("Files are not identical", are_files_identical(urlpath_default, urlpath_mmap));
 #endif
 
@@ -173,7 +178,7 @@ CUTEST_TEST_TEST(mmap) {
   CUTEST_ASSERT("Value 1 of chunk 2 is wrong", fabs(chunk_data[0] - 0.5) < 1e-6);
   CUTEST_ASSERT("Value 2 of chunk 2 is wrong", fabs(chunk_data[1] - 0.6) < 1e-6);
 
-  blosc2_schunk_free(schunk_append);
+  CUTEST_ASSERT("Could not free the schunk ressources", blosc2_schunk_free(schunk_append) == 0);
   CUTEST_ASSERT("Files are identical", !are_files_identical(urlpath_default, urlpath_mmap));
 
   free(chunk_data);