| Message ID | 20180112110757.D4D2440172552@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ldconfig: Call fsync on temporary files before renaming them [BZ #20890] | expand |
On Fri, Jan 12, 2018 at 12:07:57PM +0100, Florian Weimer wrote: > If the system crashes before the file data has been written to disk, the > file system recovery upon the next mount may restore a partially > rewritten temporary file under the non-temporary (final) name (after the > rename operation). > > Some file systems perform an implicit fsync before renaming a file over > another one, but XFS does not, for example. At least on Linux no file system performs an actual fsync equivalent. A few do start a writeout, but don't actually wait on it.
On 01/12/2018 12:37 PM, Christoph Hellwig wrote: > On Fri, Jan 12, 2018 at 12:07:57PM +0100, Florian Weimer wrote: >> If the system crashes before the file data has been written to disk, the >> file system recovery upon the next mount may restore a partially >> rewritten temporary file under the non-temporary (final) name (after the >> rename operation). >> >> Some file systems perform an implicit fsync before renaming a file over >> another one, but XFS does not, for example. > > At least on Linux no file system performs an actual fsync equivalent. > A few do start a writeout, but don't actually wait on it. Oh. I'll drop the last paragraph from the commit message, then. Thanks, Florian
Florian Weimer wrote: > if (write (fd, strings, total_strlen) != (ssize_t) total_strlen > - || close (fd)) > + || fsync (fd) != 0 > + || close (fd) != 0) > error (EXIT_FAILURE, errno, _("Writing of cache data failed")); Would fdatasync suffice, instead of fsync?
On 01/13/2018 03:28 AM, Paul Eggert wrote: > Florian Weimer wrote: >> if (write (fd, strings, total_strlen) != (ssize_t) total_strlen >> - || close (fd)) >> + || fsync (fd) != 0 >> + || close (fd) != 0) >> error (EXIT_FAILURE, errno, _("Writing of cache data failed")); > > Would fdatasync suffice, instead of fsync? One can be fdatasync, I think. The first one should actually happen after the chmod call, and then we need fsync. Thanks, Florian Subject: [PATCH] ldconfig: Sync temporary files to disk before renaming them [BZ #20890] To: libc-alpha@sourceware.org If the system crashes before the file data has been written to disk, the file system recovery upon the next mount may restore a partially rewritten temporary file under the non-temporary (final) name (after the rename operation). 2018-01-12 Florian Weimer <fweimer@redhat.com> [BZ #20890] * elf/cache.c (save_cache): Call fsync on temporary file before renaming it. (save_aux_cache): Call fdatasync on temporary file before renaming it. diff --git a/elf/cache.c b/elf/cache.c index 1ec6ab36e7..f032081e5c 100644 --- a/elf/cache.c +++ b/elf/cache.c @@ -448,8 +448,7 @@ save_cache (const char *cache_name) error (EXIT_FAILURE, errno, _("Writing of cache data failed")); } - if (write (fd, strings, total_strlen) != (ssize_t) total_strlen - || close (fd)) + if (write (fd, strings, total_strlen) != (ssize_t) total_strlen) error (EXIT_FAILURE, errno, _("Writing of cache data failed")); /* Make sure user can always read cache file */ @@ -458,6 +457,10 @@ save_cache (const char *cache_name) _("Changing access rights of %s to %#o failed"), temp_name, S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR); + /* Make sure that data is written to disk. */ + if (fsync (fd) != 0 || close (fd) != 0) + error (EXIT_FAILURE, errno, _("Writing of cache data failed")); + /* Move temporary to its final location. */ if (rename (temp_name, cache_name)) error (EXIT_FAILURE, errno, _("Renaming of %s to %s failed"), temp_name, @@ -812,7 +815,8 @@ save_aux_cache (const char *aux_cache_name) if (write (fd, file_entries, file_entries_size + total_strlen) != (ssize_t) (file_entries_size + total_strlen) - || close (fd)) + || fdatasync (fd) != 0 + || close (fd) != 0) { unlink (temp_name); goto out_fail;
On 01/19/2018 05:39 PM, Florian Weimer wrote: > 2018-01-12 Florian Weimer<fweimer@redhat.com> > > [BZ #20890] > * elf/cache.c (save_cache): Call fsync on temporary file before > renaming it. > (save_aux_cache): Call fdatasync on temporary file before renaming > it. Ping? <https://sourceware.org/ml/libc-alpha/2018-01/msg00660.html> Thanks, Florian
On 20/02/2018 10:59, Florian Weimer wrote: > On 01/19/2018 05:39 PM, Florian Weimer wrote: >> 2018-01-12 Florian Weimer<fweimer@redhat.com> >> >> [BZ #20890] >> * elf/cache.c (save_cache): Call fsync on temporary file before >> renaming it. >> (save_aux_cache): Call fdatasync on temporary file before renaming >> it. > > Ping? LGTM. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > <https://sourceware.org/ml/libc-alpha/2018-01/msg00660.html> > > Thanks, > Florian
diff --git a/elf/cache.c b/elf/cache.c index 1ec6ab36e7..2165f8d305 100644 --- a/elf/cache.c +++ b/elf/cache.c @@ -449,7 +449,8 @@ save_cache (const char *cache_name) } if (write (fd, strings, total_strlen) != (ssize_t) total_strlen - || close (fd)) + || fsync (fd) != 0 + || close (fd) != 0) error (EXIT_FAILURE, errno, _("Writing of cache data failed")); /* Make sure user can always read cache file */ @@ -812,7 +813,8 @@ save_aux_cache (const char *aux_cache_name) if (write (fd, file_entries, file_entries_size + total_strlen) != (ssize_t) (file_entries_size + total_strlen) - || close (fd)) + || fsync (fd) != 0 + || close (fd) != 0) { unlink (temp_name); goto out_fail;