From a02d22cca825f2c04381b40d50abfc7a15afec20 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sat, 21 Apr 2018 00:28:53 +0200 Subject: IO: shada should respect 'fsync' option shada_write_file() is called on exit (:quit and friends), this can be very slow. Note: AFAICT Vim (do_viminfo()) does not appear to fsync() viminfo. --- src/nvim/event/process.c | 2 +- src/nvim/shada.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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)); -- cgit From 498731615c2f879c0b67323aba385c17a4a39d24 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sat, 21 Apr 2018 00:34:13 +0200 Subject: IO: let 'fsync' option control more cases Vim has the 'swapsync' option which we removed in 62d137ce0969. Instead let 'fsync' control swapfile-fsync. These cases ALWAYS force fsync (ignoring 'fsync' option): - Idle (CursorHold). - Exit caused by deadly signal. - SIGPWR signal. - Explicit :preserve command. --- src/nvim/ex_docmd.c | 10 +++------- src/nvim/fileio.c | 2 +- src/nvim/getchar.c | 11 +++++++---- src/nvim/memline.c | 12 ++++++------ src/nvim/misc1.c | 2 +- src/nvim/os/signal.c | 2 +- 6 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c index 7cd6dbdeca..7c857cde82 100644 --- a/src/nvim/ex_docmd.c +++ b/src/nvim/ex_docmd.c @@ -6547,18 +6547,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..3541ba7cc8 100644 --- a/src/nvim/getchar.c +++ b/src/nvim/getchar.c @@ -1347,14 +1347,17 @@ void before_blocking(void) * 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) +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/memline.c b/src/nvim/memline.c index c11ca74f5c..dd80ec8d6a 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,14 +1612,14 @@ 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); + 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)); + | (do_fsync && bufIsChanged(buf) ? MFS_FLUSH : 0)); if (check_char && os_char_avail()) /* character available now */ break; } @@ -1636,7 +1636,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; @@ -1656,7 +1656,7 @@ void ml_preserve(buf_T *buf, int message) 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); + 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; @@ -1686,7 +1686,7 @@ void ml_preserve(buf_T *buf, int message) } (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) + 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 */ } 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/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 -- cgit From 9139bf81cf81a93e331f98dcddbe489fc3529787 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sat, 21 Apr 2018 01:27:55 +0200 Subject: defaults: disable 'fsync' ref #6725 fsync() is very slow on some systems. And since the parent commit, Nvim is smarter about flushing files at certain times (e.g. CursorHold), regardless of whether 'fsync' is enabled. So it's less risky to disable 'fsync'. Profiling showed slow (2-4s) :write and :quit caused by fsync(): :quit shada_write_file(NULL, false); :write + fsync 0 0x00007f72da567b2d in fsync () at ../sysdeps/unix/syscall-template.S:84 1 0x0000000000638970 in uv__fs_fsync (req=) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:150 2 uv__fs_work (w=) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:953 3 0x0000000000639a70 in uv_fs_fsync (loop=, req=, file=41, cb=0x7f72da567b2d ) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:1094 4 0x0000000000573694 in os_fsync (fd=41) at ../src/nvim/os/fs.c:631 5 0x00000000004ec9dc in buf_write (buf=, fname=, sfname=, start=1, end=1997, eap=0x7fffc864c570, append=, forceit=, reset_changed=, filtering=) at ../src/nvim/fileio.c:3387 6 0x00000000004b44ff in do_write (eap=0x7fffc864c570) at ../src/nvim/ex_cmds.c:1745 ... :write + nofsync 0 0x00007f72da567b2d in fsync () at ../sysdeps/unix/syscall-template.S:84 1 0x0000000000638970 in uv__fs_fsync (req=) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:150 2 uv__fs_work (w=) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:953 3 0x0000000000639a70 in uv_fs_fsync (loop=, req=, file=36, cb=0x7f72da567b2d ) at /home/vagrant/neovim/.deps/build/src/libuv/src/unix/fs.c:1094 4 0x0000000000573694 in os_fsync (fd=36) at ../src/nvim/os/fs.c:631 5 0x0000000000528f5a in mf_sync (mfp=0x7f72d8968d00, flags=5) at ../src/nvim/memfile.c:466 6 0x000000000052d569 in ml_preserve (buf=0x7f72d890f000, message=0) at ../src/nvim/memline.c:1659 7 0x00000000004ebadf in buf_write (buf=, fname=, sfname=, start=1, end=1997, eap=0x7fffc864c570, append=, forceit=, reset_changed=, filtering=) at ../src/nvim/fileio.c:3071 8 0x00000000004b44ff in do_write (eap=0x7fffc864c570) at ../src/nvim/ex_cmds.c:1745 ... --- runtime/doc/options.txt | 24 +++++++++++++----------- src/nvim/options.lua | 2 +- 2 files changed, 14 insertions(+), 12 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/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', -- cgit From b71697bc67dcb90002dcabaf4a61f69d04281f06 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Sat, 21 Apr 2018 01:55:10 +0200 Subject: lint --- src/nvim/getchar.c | 18 +++++++----------- src/nvim/memline.c | 20 +++++++++++--------- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/nvim/getchar.c b/src/nvim/getchar.c index 3541ba7cc8..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,13 +1338,11 @@ 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. - */ +/// 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; diff --git a/src/nvim/memline.c b/src/nvim/memline.c index dd80ec8d6a..1e26a728a3 100644 --- a/src/nvim/memline.c +++ b/src/nvim/memline.c @@ -1613,15 +1613,16 @@ void ml_sync_all(int check_file, int check_char, bool do_fsync) || file_info.stat.st_mtim.tv_sec != buf->b_mtime_read || os_fileinfo_size(&file_info) != buf->b_orig_size) { ml_preserve(buf, false, do_fsync); - did_check_timestamps = FALSE; - need_check_timestamps = TRUE; /* give message later */ + 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) | (do_fsync && bufIsChanged(buf) ? MFS_FLUSH : 0)); - if (check_char && os_char_avail()) /* character available now */ + if (check_char && os_char_avail()) { // character available now break; + } } } } @@ -1654,8 +1655,8 @@ void ml_preserve(buf_T *buf, int message, bool do_fsync) * before. */ got_int = FALSE; - ml_flush_line(buf); /* flush buffered line */ - (void)ml_find_line(buf, (linenr_T)0, ML_FLUSH); /* flush locked block */ + 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) */ @@ -1684,11 +1685,12 @@ void ml_preserve(buf_T *buf, int message, bool do_fsync) 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 | (do_fsync ? MFS_FLUSH : 0)) == 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; -- cgit From 32f3937477e05b13e05bba4463c5cfd417a2fde8 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 23 Apr 2018 04:43:00 +0200 Subject: test: fsync() codepaths --- test/functional/core/fileio_spec.lua | 61 ++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 test/functional/core/fileio_spec.lua diff --git a/test/functional/core/fileio_spec.lua b/test/functional/core/fileio_spec.lua new file mode 100644 index 0000000000..848ef7cae0 --- /dev/null +++ b/test/functional/core/fileio_spec.lua @@ -0,0 +1,61 @@ +local helpers = require('test.functional.helpers')(after_each) + +local clear = helpers.clear +local command = helpers.command +local eq = helpers.eq +local eval = helpers.eval +local feed = helpers.feed +local funcs = helpers.funcs +local nvim_prog = helpers.nvim_prog +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() + -- This is an "acceptance test" or "smoke test". + + 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('set swapfile') + command('set updatetime=1') + command('write Xtest_startup_file1') + feed('ifooh') + sleep(2) + eq(1, eval('&modified')) + + -- 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') + + -- 5. Enable 'fsync' option, write file. + command('set fsync') + feed('ibazh') + command('write') + end) +end) + -- cgit From 77cb14cc6da5dff685c6e5a4005da433c39d5ff7 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 24 Apr 2018 00:27:09 +0200 Subject: API: nvim__stats() Use it to verify fsync() behavior. --- src/nvim/api/vim.c | 11 +++++++++++ src/nvim/globals.h | 5 +++++ src/nvim/os/fs.c | 1 + test/functional/core/fileio_spec.lua | 21 ++++++++++++++------- 4 files changed, 31 insertions(+), 7 deletions(-) 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/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/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/test/functional/core/fileio_spec.lua b/test/functional/core/fileio_spec.lua index 848ef7cae0..09533e4e60 100644 --- a/test/functional/core/fileio_spec.lua +++ b/test/functional/core/fileio_spec.lua @@ -3,10 +3,11 @@ local helpers = require('test.functional.helpers')(after_each) local clear = helpers.clear local command = helpers.command local eq = helpers.eq -local eval = helpers.eval 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 @@ -22,20 +23,24 @@ describe('fileio', function() end) it('fsync() codepaths #8304', function() - -- This is an "acceptance test" or "smoke test". - 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('set swapfile') - command('set updatetime=1') command('write Xtest_startup_file1') feed('ifooh') - sleep(2) - eq(1, eval('&modified')) + command('write') + eq(0, request('nvim__stats').fsync) -- 'nofsync' is the default. + command('set swapfile') + command('set updatetime=1') + feed('izubh') -- 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', @@ -51,11 +56,13 @@ describe('fileio', function() -- 4. Explicit :preserve command. command('preserve') + eq(2, request('nvim__stats').fsync) -- 5. Enable 'fsync' option, write file. command('set fsync') feed('ibazh') command('write') + eq(4, request('nvim__stats').fsync) end) end) -- cgit