Merge #8304 "default to 'nofsync'"

14 files changed, 137 insertions, 53 deletions

diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt
index 0b7c61ea18..a63bef5622 100644
--- a/runtime/doc/options.txt
+++ b/runtime/doc/options.txt
@@ -2690,17 +2690,19 @@ A jump table for the options with a short description can be found at |Q_op|.
 	security reasons.
 
 						*'fsync'* *'fs'*
-'fsync' 'fs'		boolean	(default on)
-			global
-	When on, the library function fsync() will be called after writing a
-	file.  This will flush a file to disk, ensuring that it is safely
-	written even on filesystems which do metadata-only journaling.  This
-	will force the harddrive to spin up on Linux systems running in laptop
-	mode, so it may be undesirable in some situations.  Be warned that
-	turning this off increases the chances of data loss after a crash.
-
-	Currently applies only to writing the buffer with e.g. |:w| and 
-	|writefile()|.
+'fsync' 'fs'		boolean	(default off)
+			global
+	When on, the OS function fsync() will be called after saving a file
+	(|:write|, |writefile()|, …), |swap-file| and |shada-file|. This
+	flushes the file to disk, ensuring that it is safely written.
+	Slow on some systems: writing buffers, quitting Nvim, and other
+	operations may sometimes take a few seconds.
+
+	Files are ALWAYS flushed ('fsync' is ignored) when:
+	- |CursorHold| event is triggered
+	- |:preserve| is called
+	- system signals low battery life
+	- Nvim exits abnormally
 
 				   *'gdefault'* *'gd'* *'nogdefault'* *'nogd'*
 'gdefault' 'gd'		boolean	(default off)
diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c
index 07ec6e8c27..64fe1359c6 100644
--- a/src/nvim/api/vim.c
+++ b/src/nvim/api/vim.c
@@ -1473,6 +1473,17 @@ Float nvim__id_float(Float flt)
   return flt;
 }
 
+/// Gets internal stats.
+///
+/// @return Map of various internal stats.
+Dictionary nvim__stats(void)
+{
+  Dictionary rv = ARRAY_DICT_INIT;
+  PUT(rv, "fsync", INTEGER_OBJ(g_stats.fsync));
+  PUT(rv, "redraw", INTEGER_OBJ(g_stats.redraw));
+  return rv;
+}
+
 /// Gets a list of dictionaries representing attached UIs.
 ///
 /// @return Array of UI dictionaries
diff --git a/src/nvim/event/process.c b/src/nvim/event/process.c
index 23433cf495..f82614e7a1 100644
--- a/src/nvim/event/process.c
+++ b/src/nvim/event/process.c
@@ -265,7 +265,7 @@ static void process_close_event(void **argv)
   if (proc->type == kProcessTypePty) {
     xfree(((PtyProcess *)proc)->term_name);
   }
-  if (proc->cb) {
+  if (proc->cb) {  // "on_exit" for jobstart(). See channel_job_start().
     proc->cb(proc, proc->status, proc->data);
   }
 }
diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c
index 0a20008cd9..8aa670fc74 100644
--- a/src/nvim/ex_docmd.c
+++ b/src/nvim/ex_docmd.c
@@ -6552,18 +6552,14 @@ void alist_slash_adjust(void)
 
 #endif
 
-/*
- * ":preserve".
- */
+/// ":preserve".
 static void ex_preserve(exarg_T *eap)
 {
   curbuf->b_flags |= BF_PRESERVED;
-  ml_preserve(curbuf, TRUE);
+  ml_preserve(curbuf, true, true);
 }
 
-/*
- * ":recover".
- */
+/// ":recover".
 static void ex_recover(exarg_T *eap)
 {
   /* Set recoverymode right away to avoid the ATTENTION prompt. */
diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c
index 4adff63b95..d96f5412ec 100644
--- a/src/nvim/fileio.c
+++ b/src/nvim/fileio.c
@@ -3068,7 +3068,7 @@ nobackup:
    */
   if (reset_changed && !newfile && overwriting
       && !(exiting && backup != NULL)) {
-    ml_preserve(buf, FALSE);
+    ml_preserve(buf, false, !!p_fs);
     if (got_int) {
       SET_ERRMSG(_(e_interr));
       goto restore_backup;
diff --git a/src/nvim/getchar.c b/src/nvim/getchar.c
index 98164b2653..e2566c8c66 100644
--- a/src/nvim/getchar.c
+++ b/src/nvim/getchar.c
@@ -1328,10 +1328,8 @@ int using_script(void)
   return scriptin[curscript] != NULL;
 }
 
-/*
- * This function is called just before doing a blocking wait.  Thus after
- * waiting 'updatetime' for a character to arrive.
- */
+/// This function is called just before doing a blocking wait.  Thus after
+/// waiting 'updatetime' for a character to arrive.
 void before_blocking(void)
 {
   updatescript(0);
@@ -1340,21 +1338,22 @@ void before_blocking(void)
   }
 }
 
-/*
- * updatescipt() is called when a character can be written into the script file
- * or when we have waited some time for a character (c == 0)
- *
- * All the changed memfiles are synced if c == 0 or when the number of typed
- * characters reaches 'updatecount' and 'updatecount' is non-zero.
- */
-void updatescript(int c)
+/// updatescript() is called when a character can be written to the script file
+/// or when we have waited some time for a character (c == 0).
+///
+/// All the changed memfiles are synced if c == 0 or when the number of typed
+/// characters reaches 'updatecount' and 'updatecount' is non-zero.
+static void updatescript(int c)
 {
   static int count = 0;
 
-  if (c && scriptout)
+  if (c && scriptout) {
     putc(c, scriptout);
-  if (c == 0 || (p_uc > 0 && ++count >= p_uc)) {
-    ml_sync_all(c == 0, TRUE);
+  }
+  bool idle = (c == 0);
+  if (idle || (p_uc > 0 && ++count >= p_uc)) {
+    ml_sync_all(idle, true,
+                (!!p_fs || idle));  // Always fsync at idle (CursorHold).
     count = 0;
   }
 }
diff --git a/src/nvim/globals.h b/src/nvim/globals.h
index 56790bc89b..403a5928c9 100644
--- a/src/nvim/globals.h
+++ b/src/nvim/globals.h
@@ -80,6 +80,11 @@ typedef enum {
   kTrue  = 1,
 } TriState;
 
+EXTERN struct nvim_stats_s {
+  int64_t fsync;
+  int64_t redraw;
+} g_stats INIT(= { 0, 0 });
+
 /* Values for "starting" */
 #define NO_SCREEN       2       /* no screen updating yet */
 #define NO_BUFFERS      1       /* not all buffers loaded yet */
diff --git a/src/nvim/memline.c b/src/nvim/memline.c
index c11ca74f5c..1e26a728a3 100644
--- a/src/nvim/memline.c
+++ b/src/nvim/memline.c
@@ -1593,7 +1593,7 @@ static int recov_file_names(char_u **names, char_u *path, int prepend_dot)
  * If 'check_char' is TRUE, stop syncing when character becomes available, but
  * always sync at least one block.
  */
-void ml_sync_all(int check_file, int check_char)
+void ml_sync_all(int check_file, int check_char, bool do_fsync)
 {
   FOR_ALL_BUFFERS(buf) {
     if (buf->b_ml.ml_mfp == NULL || buf->b_ml.ml_mfp->mf_fname == NULL)
@@ -1612,16 +1612,17 @@ void ml_sync_all(int check_file, int check_char)
       if (!os_fileinfo((char *)buf->b_ffname, &file_info)
           || file_info.stat.st_mtim.tv_sec != buf->b_mtime_read
           || os_fileinfo_size(&file_info) != buf->b_orig_size) {
-        ml_preserve(buf, FALSE);
-        did_check_timestamps = FALSE;
-        need_check_timestamps = TRUE;           /* give message later */
+        ml_preserve(buf, false, do_fsync);
+        did_check_timestamps = false;
+        need_check_timestamps = true;           // give message later
       }
     }
     if (buf->b_ml.ml_mfp->mf_dirty) {
       (void)mf_sync(buf->b_ml.ml_mfp, (check_char ? MFS_STOP : 0)
-          | (bufIsChanged(buf) ? MFS_FLUSH : 0));
-      if (check_char && os_char_avail())        /* character available now */
+                    | (do_fsync && bufIsChanged(buf) ? MFS_FLUSH : 0));
+      if (check_char && os_char_avail()) {      // character available now
         break;
+      }
     }
   }
 }
@@ -1636,7 +1637,7 @@ void ml_sync_all(int check_file, int check_char)
  *
  * when message is TRUE the success of preserving is reported
  */
-void ml_preserve(buf_T *buf, int message)
+void ml_preserve(buf_T *buf, int message, bool do_fsync)
 {
   bhdr_T      *hp;
   linenr_T lnum;
@@ -1654,9 +1655,9 @@ void ml_preserve(buf_T *buf, int message)
    * before. */
   got_int = FALSE;
 
-  ml_flush_line(buf);                               /* flush buffered line */
-  (void)ml_find_line(buf, (linenr_T)0, ML_FLUSH);   /* flush locked block */
-  status = mf_sync(mfp, MFS_ALL | MFS_FLUSH);
+  ml_flush_line(buf);                               // flush buffered line
+  (void)ml_find_line(buf, (linenr_T)0, ML_FLUSH);   // flush locked block
+  status = mf_sync(mfp, MFS_ALL | (do_fsync ? MFS_FLUSH : 0));
 
   /* stack is invalid after mf_sync(.., MFS_ALL) */
   buf->b_ml.ml_stack_top = 0;
@@ -1684,11 +1685,12 @@ void ml_preserve(buf_T *buf, int message)
       CHECK(buf->b_ml.ml_locked_low != lnum, "low != lnum");
       lnum = buf->b_ml.ml_locked_high + 1;
     }
-    (void)ml_find_line(buf, (linenr_T)0, ML_FLUSH);     /* flush locked block */
-    /* sync the updated pointer blocks */
-    if (mf_sync(mfp, MFS_ALL | MFS_FLUSH) == FAIL)
+    (void)ml_find_line(buf, (linenr_T)0, ML_FLUSH);  // flush locked block
+    // sync the updated pointer blocks
+    if (mf_sync(mfp, MFS_ALL | (do_fsync ? MFS_FLUSH : 0)) == FAIL) {
       status = FAIL;
-    buf->b_ml.ml_stack_top = 0;             /* stack is invalid now */
+    }
+    buf->b_ml.ml_stack_top = 0;  // stack is invalid now
   }
 theend:
   got_int |= got_int_save;
diff --git a/src/nvim/misc1.c b/src/nvim/misc1.c
index b0232b6516..28455f0ba9 100644
--- a/src/nvim/misc1.c
+++ b/src/nvim/misc1.c
@@ -2643,7 +2643,7 @@ void preserve_exit(void)
     if (buf->b_ml.ml_mfp != NULL && buf->b_ml.ml_mfp->mf_fname != NULL) {
       mch_errmsg((uint8_t *)"Vim: preserving files...\n");
       ui_flush();
-      ml_sync_all(false, false);    // preserve all swap files
+      ml_sync_all(false, false, true);  // preserve all swap files
       break;
     }
   }
diff --git a/src/nvim/options.lua b/src/nvim/options.lua
index 80484d0ad2..66018b2475 100644
--- a/src/nvim/options.lua
+++ b/src/nvim/options.lua
@@ -976,7 +976,7 @@ return {
       secure=true,
       vi_def=true,
       varname='p_fs',
-      defaults={if_true={vi=true}}
+      defaults={if_true={vi=false}}
     },
     {
       full_name='gdefault', abbreviation='gd',
diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c
index 0414794d01..3e1af7f1c2 100644
--- a/src/nvim/os/fs.c
+++ b/src/nvim/os/fs.c
@@ -629,6 +629,7 @@ int os_fsync(int fd)
 {
   int r;
   RUN_UV_FS_FUNC(r, uv_fs_fsync, fd, NULL);
+  g_stats.fsync++;
   return r;
 }
 
diff --git a/src/nvim/os/signal.c b/src/nvim/os/signal.c
index 732be072e1..fc7f9cefd1 100644
--- a/src/nvim/os/signal.c
+++ b/src/nvim/os/signal.c
@@ -145,7 +145,7 @@ static void on_signal(SignalWatcher *handle, int signum, void *data)
     case SIGPWR:
       // Signal of a power failure(eg batteries low), flush the swap files to
       // be safe
-      ml_sync_all(false, false);
+      ml_sync_all(false, false, true);
       break;
 #endif
 #ifdef SIGPIPE
diff --git a/src/nvim/shada.c b/src/nvim/shada.c
index 15ac28e1bf..f4454504a4 100644
--- a/src/nvim/shada.c
+++ b/src/nvim/shada.c
@@ -811,7 +811,7 @@ static int open_shada_file_for_reading(const char *const fname,
 /// Wrapper for closing file descriptors
 static void close_file(void *cookie)
 {
-  const int error = file_free(cookie, true);
+  const int error = file_free(cookie, !!p_fs);
   if (error != 0) {
     emsgf(_(SERR "System error while closing ShaDa file: %s"),
           os_strerror(error));
diff --git a/test/functional/core/fileio_spec.lua b/test/functional/core/fileio_spec.lua
new file mode 100644
index 0000000000..09533e4e60
--- /dev/null
+++ b/test/functional/core/fileio_spec.lua
@@ -0,0 +1,68 @@
+local helpers = require('test.functional.helpers')(after_each)
+
+local clear = helpers.clear
+local command = helpers.command
+local eq = helpers.eq
+local feed = helpers.feed
+local funcs = helpers.funcs
+local nvim_prog = helpers.nvim_prog
+local request = helpers.request
+local retry = helpers.retry
+local rmdir = helpers.rmdir
+local sleep = helpers.sleep
+
+describe('fileio', function()
+  before_each(function()
+  end)
+  after_each(function()
+    command(':qall!')
+    os.remove('Xtest_startup_shada')
+    os.remove('Xtest_startup_file1')
+    os.remove('Xtest_startup_file2')
+    rmdir('Xtest_startup_swapdir')
+  end)
+
+  it('fsync() codepaths #8304', function()
+    clear({ args={ '-i', 'Xtest_startup_shada',
+                   '--cmd', 'set directory=Xtest_startup_swapdir' } })
+
+    -- These cases ALWAYS force fsync (regardless of 'fsync' option):
+
+    -- 1. Idle (CursorHold) with modified buffers (+ 'swapfile').
+    command('write Xtest_startup_file1')
+    feed('ifoo<esc>h')
+    command('write')
+    eq(0, request('nvim__stats').fsync)   -- 'nofsync' is the default.
+    command('set swapfile')
+    command('set updatetime=1')
+    feed('izub<esc>h')                    -- File is 'modified'.
+    sleep(3)                              -- Allow 'updatetime' to expire.
+    retry(3, nil, function()
+      eq(1, request('nvim__stats').fsync)
+    end)
+    command('set updatetime=9999')
+
+    -- 2. Exit caused by deadly signal (+ 'swapfile').
+    local j = funcs.jobstart({ nvim_prog, '-u', 'NONE', '-i',
+                               'Xtest_startup_shada', '--headless',
+                               '-c', 'set swapfile',
+                               '-c', 'write Xtest_startup_file2',
+                               '-c', 'put =localtime()', })
+    sleep(10)         -- Let Nvim start.
+    funcs.jobstop(j)  -- Send deadly signal.
+
+    -- 3. SIGPWR signal.
+    -- ??
+
+    -- 4. Explicit :preserve command.
+    command('preserve')
+    eq(2, request('nvim__stats').fsync)
+
+    -- 5. Enable 'fsync' option, write file.
+    command('set fsync')
+    feed('ibaz<esc>h')
+    command('write')
+    eq(4, request('nvim__stats').fsync)
+  end)
+end)
+