From fa11a09fed2b6f483231608866a682ee3a376277 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 13 Apr 2016 17:17:51 -0400 Subject: [PATCH] Fix assorted portability issues with using msync() for data flushing. Commit 428b1d6b29ca599c5700d4bc4f4ce4c5880369bf introduced the use of msync() for flushing dirty data from the kernel's file buffers. Several portability issues were overlooked, though: * Not all implementations of mmap() think that nbytes == 0 means "map the whole file". To fix, use lseek() to find out the true length. Fix callers of pg_flush_data to be aware that nbytes == 0 may result in trashing the file's seek position. * Not all implementations of mmap() will accept partial-page mmap requests. To fix, round down the length request to whatever sysconf() says the page size is. (I think this is OK from a portability standpoint, because sysconf() is required by SUS v2, and we aren't trying to compile this part on Windows anyway. Buildfarm should let us know if not.) * On 32-bit machines, the file size might exceed the available free address space, or even exceed what will fit in size_t. Check for the latter explicitly to avoid passing a false request size to mmap(). If mmap fails, silently fall through to the next implementation method, rather than bleating to the postmaster log and giving up. * mmap'ing directories fails on some platforms, and even if it works, msync'ing the directory is quite unlikely to help, as for that matter are the other flush implementations. In pre_sync_fname(), just skip flush attempts on directories. In passing, copy-edit the comments a bit. Stas Kelvich and myself --- src/backend/storage/file/fd.c | 131 +++++++++++++++++++++++----------- 1 file changed, 90 insertions(+), 41 deletions(-) diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index 3e02dceccd..f513554bff 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -64,6 +64,7 @@ #ifndef WIN32 #include #endif +#include #include #include #ifdef HAVE_SYS_RESOURCE_H @@ -391,34 +392,36 @@ pg_fdatasync(int fd) /* * pg_flush_data --- advise OS that the described dirty data should be flushed * - * An offset of 0 with an nbytes 0 means that the entire file should be - * flushed. + * offset of 0 with nbytes 0 means that the entire file should be flushed; + * in this case, this function may have side-effects on the file's + * seek position! */ void pg_flush_data(int fd, off_t offset, off_t nbytes) { /* * Right now file flushing is primarily used to avoid making later - * fsync()/fdatasync() calls have a less impact. Thus don't trigger - * flushes if fsyncs are disabled - that's a decision we might want to - * make configurable at some point. + * fsync()/fdatasync() calls have less impact. Thus don't trigger flushes + * if fsyncs are disabled - that's a decision we might want to make + * configurable at some point. */ if (!enableFsync) return; /* - * XXX: compile all alternatives, to find portability problems more easily + * We compile all alternatives that are supported on the current platform, + * to find portability problems more easily. */ #if defined(HAVE_SYNC_FILE_RANGE) { - int rc = 0; + int rc; /* * sync_file_range(SYNC_FILE_RANGE_WRITE), currently linux specific, - * tells the OS that writeback for the passed in blocks should be + * tells the OS that writeback for the specified blocks should be * started, but that we don't want to wait for completion. Note that * this call might block if too much dirty data exists in the range. - * This is the preferrable method on OSs supporting it, as it works + * This is the preferable method on OSs supporting it, as it works * reliably when available (contrast to msync()) and doesn't flush out * clean data (like FADV_DONTNEED). */ @@ -438,72 +441,107 @@ pg_flush_data(int fd, off_t offset, off_t nbytes) #endif #if !defined(WIN32) && defined(MS_ASYNC) { - int rc = 0; void *p; + static int pagesize = 0; /* * On several OSs msync(MS_ASYNC) on a mmap'ed file triggers - * writeback. On linux it only does so with MS_SYNC is specified, but + * writeback. On linux it only does so if MS_SYNC is specified, but * then it does the writeback synchronously. Luckily all common linux - * systems have sync_file_range(). This is preferrable over + * systems have sync_file_range(). This is preferable over * FADV_DONTNEED because it doesn't flush out clean data. * * We map the file (mmap()), tell the kernel to sync back the contents * (msync()), and then remove the mapping again (munmap()). */ - p = mmap(NULL, nbytes, - PROT_READ | PROT_WRITE, MAP_SHARED, - fd, offset); - if (p == MAP_FAILED) - { - ereport(WARNING, - (errcode_for_file_access(), - errmsg("could not mmap while flushing dirty data: %m"))); - return; - } - rc = msync(p, nbytes, MS_ASYNC); - if (rc != 0) + /* mmap() needs actual length if we want to map whole file */ + if (offset == 0 && nbytes == 0) { - ereport(WARNING, - (errcode_for_file_access(), - errmsg("could not flush dirty data: %m"))); - /* NB: need to fall through to munmap()! */ + nbytes = lseek(fd, 0, SEEK_END); + if (nbytes < 0) + { + ereport(WARNING, + (errcode_for_file_access(), + errmsg("could not determine dirty data size: %m"))); + return; + } } - rc = munmap(p, nbytes); - if (rc != 0) + /* + * Some platforms reject partial-page mmap() attempts. To deal with + * that, just truncate the request to a page boundary. If any extra + * bytes don't get flushed, well, it's only a hint anyway. + */ + + /* fetch pagesize only once */ + if (pagesize == 0) + pagesize = sysconf(_SC_PAGESIZE); + + /* align length to pagesize, dropping any fractional page */ + if (pagesize > 0) + nbytes = (nbytes / pagesize) * pagesize; + + /* fractional-page request is a no-op */ + if (nbytes <= 0) + return; + + /* + * mmap could well fail, particularly on 32-bit platforms where there + * may simply not be enough address space. If so, silently fall + * through to the next implementation. + */ + if (nbytes <= (off_t) SSIZE_MAX) + p = mmap(NULL, nbytes, PROT_READ, MAP_SHARED, fd, offset); + else + p = MAP_FAILED; + + if (p != MAP_FAILED) { - /* FATAL error because mapping would remain */ - ereport(FATAL, - (errcode_for_file_access(), - errmsg("could not munmap while flushing blocks: %m"))); - } + int rc; - return; + rc = msync(p, (size_t) nbytes, MS_ASYNC); + if (rc != 0) + { + ereport(WARNING, + (errcode_for_file_access(), + errmsg("could not flush dirty data: %m"))); + /* NB: need to fall through to munmap()! */ + } + + rc = munmap(p, (size_t) nbytes); + if (rc != 0) + { + /* FATAL error because mapping would remain */ + ereport(FATAL, + (errcode_for_file_access(), + errmsg("could not munmap() while flushing data: %m"))); + } + + return; + } } #endif #if defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED) { - int rc = 0; + int rc; /* * Signal the kernel that the passed in range should not be cached * anymore. This has the, desired, side effect of writing out dirty * data, and the, undesired, side effect of likely discarding useful * clean cached blocks. For the latter reason this is the least - * preferrable method. + * preferable method. */ rc = posix_fadvise(fd, offset, nbytes, POSIX_FADV_DONTNEED); - /* don't error out, this is just a performance optimization */ if (rc != 0) { + /* don't error out, this is just a performance optimization */ ereport(WARNING, (errcode_for_file_access(), errmsg("could not flush dirty data: %m"))); - return; } return; @@ -1510,6 +1548,13 @@ FileWriteback(File file, off_t offset, int amount) file, VfdCache[file].fileName, (int64) offset, amount)); + /* + * Caution: do not call pg_flush_data with amount = 0, it could trash the + * file's seek position. + */ + if (amount <= 0) + return; + returnCode = FileAccess(file); if (returnCode < 0) return; @@ -2904,11 +2949,15 @@ pre_sync_fname(const char *fname, bool isdir, int elevel) { int fd; + /* Don't try to flush directories, it'll likely just fail */ + if (isdir) + return; + fd = OpenTransientFile((char *) fname, O_RDONLY | PG_BINARY, 0); if (fd < 0) { - if (errno == EACCES || (isdir && errno == EISDIR)) + if (errno == EACCES) return; ereport(elevel, (errcode_for_file_access(), -- 2.40.0