| Message ID | 20211203215531.GC3930@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsdcld: use WAL journal for faster commits | expand |
> On Dec 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > Currently nfsdcld is doing three fdatasyncs for each upcall. Based on > SQLite documentation, WAL mode should also be safe, and I can confirm > from an strace that it results in only one fdatasync each. > > This may be a bottleneck e.g. when lots of clients are being created or > expired at once (e.g. on reboot). > > Not bothering with error checking, as this is just an optimization and > nfsdcld will still function without. (Might be better to log something > on failure, though.) I'm in full philosophical agreement for performance improvements in this area. There are some caveats for WAL: - It requires SQLite v3.7.0 (2010). configure.ac may need to be updated. - All processes using the DB file have to be on the same system. Not sure if this matters for some DR scenarios that nfs-utils is supposed to support. - WAL mode is persistent; you could set this at database creation time and it should be sticky. - Documentation says "synchronous = FULL is the most commonly used synchronous setting when not in WAL mode." Why are both PRAGMAs necessary here? I agree that nfsdcld functionality is not affected if the first PRAGMA fails. > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > utils/nfsdcld/sqlite.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c > index 03016fb95823..b248eeffa204 100644 > --- a/utils/nfsdcld/sqlite.c > +++ b/utils/nfsdcld/sqlite.c > @@ -826,6 +826,9 @@ sqlite_prepare_dbh(const char *topdir) > goto out_close; > } > > + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); > + sqlite3_exec(dbh, "PRAGMA synchronous = FULL;", NULL, NULL, NULL); > + > ret = sqlite_query_schema_version(); > switch (ret) { > case CLD_SQLITE_LATEST_SCHEMA_VERSION: > -- > 2.33.1 > -- Chuck Lever
On Fri, Dec 03, 2021 at 10:07:19PM +0000, Chuck Lever III wrote: > > > > On Dec 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > Currently nfsdcld is doing three fdatasyncs for each upcall. Based on > > SQLite documentation, WAL mode should also be safe, and I can confirm > > from an strace that it results in only one fdatasync each. > > > > This may be a bottleneck e.g. when lots of clients are being created or > > expired at once (e.g. on reboot). > > > > Not bothering with error checking, as this is just an optimization and > > nfsdcld will still function without. (Might be better to log something > > on failure, though.) > > I'm in full philosophical agreement for performance improvements > in this area. There are some caveats for WAL: > > - It requires SQLite v3.7.0 (2010). configure.ac may need to be > updated. Makes sense. But I dug around a bit, and how to do this is a total mystery to me.... > - All processes using the DB file have to be on the same system. > Not sure if this matters for some DR scenarios that nfs-utils > is supposed to support. I don't think we support that. > - WAL mode is persistent; you could set this at database creation > time and it should be sti cky. I wanted to upgrade existing databases too, and figured there's no harm in calling it on each startup. > - Documentation says "synchronous = FULL is the most commonly > used synchronous setting when not in WAL mode." Why are both > PRAGMAs necessary here? Maybe they're not; I think I did see that FULL is actually the default but I can't find that in the documentation right now. --b. > > I agree that nfsdcld functionality is not affected if the first > PRAGMA fails. > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > utils/nfsdcld/sqlite.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c > > index 03016fb95823..b248eeffa204 100644 > > --- a/utils/nfsdcld/sqlite.c > > +++ b/utils/nfsdcld/sqlite.c > > @@ -826,6 +826,9 @@ sqlite_prepare_dbh(const char *topdir) > > goto out_close; > > } > > > > + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); > > + sqlite3_exec(dbh, "PRAGMA synchronous = FULL;", NULL, NULL, NULL); > > + > > ret = sqlite_query_schema_version(); > > switch (ret) { > > case CLD_SQLITE_LATEST_SCHEMA_VERSION: > > -- > > 2.33.1 > > > > -- > Chuck Lever > >
> On Dec 3, 2021, at 5:39 PM, Bruce Fields <bfields@fieldses.org> wrote: > > On Fri, Dec 03, 2021 at 10:07:19PM +0000, Chuck Lever III wrote: >> >> >>>> On Dec 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: >>> >>> From: "J. Bruce Fields" <bfields@redhat.com> >>> >>> Currently nfsdcld is doing three fdatasyncs for each upcall. Based on >>> SQLite documentation, WAL mode should also be safe, and I can confirm >>> from an strace that it results in only one fdatasync each. >>> >>> This may be a bottleneck e.g. when lots of clients are being created or >>> expired at once (e.g. on reboot). >>> >>> Not bothering with error checking, as this is just an optimization and >>> nfsdcld will still function without. (Might be better to log something >>> on failure, though.) >> >> I'm in full philosophical agreement for performance improvements >> in this area. There are some caveats for WAL: >> >> - It requires SQLite v3.7.0 (2010). configure.ac may need to be >> updated. > > Makes sense. But I dug around a bit, and how to do this is a total > mystery to me.... aclocal/libsqlite3.m4 has an SQLITE_VERSION_NUMBER check. >> - All processes using the DB file have to be on the same system. >> Not sure if this matters for some DR scenarios that nfs-utils >> is supposed to support. > > I don't think we support that. > >> - WAL mode is persistent; you could set this at database creation >> time and it should be sti cky. > > I wanted to upgrade existing databases too, and figured there's no harm > in calling it on each startup. Ah. Makes sense! >> - Documentation says "synchronous = FULL is the most commonly >> used synchronous setting when not in WAL mode." Why are both >> PRAGMAs necessary here? > > Maybe they're not; I think I did see that FULL is actually the default > but I can't find that in the documentation right now. > > --b. > >> >> I agree that nfsdcld functionality is not affected if the first >> PRAGMA fails. >> >> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> --- >>> utils/nfsdcld/sqlite.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c >>> index 03016fb95823..b248eeffa204 100644 >>> --- a/utils/nfsdcld/sqlite.c >>> +++ b/utils/nfsdcld/sqlite.c >>> @@ -826,6 +826,9 @@ sqlite_prepare_dbh(const char *topdir) >>> goto out_close; >>> } >>> >>> + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); >>> + sqlite3_exec(dbh, "PRAGMA synchronous = FULL;", NULL, NULL, NULL); >>> + >>> ret = sqlite_query_schema_version(); >>> switch (ret) { >>> case CLD_SQLITE_LATEST_SCHEMA_VERSION: >>> -- >>> 2.33.1 >>> >> >> -- >> Chuck Lever >> >>
On Sat, Dec 04, 2021 at 12:35:11AM +0000, Chuck Lever III wrote: > > > On Dec 3, 2021, at 5:39 PM, Bruce Fields <bfields@fieldses.org> wrote: > > > > On Fri, Dec 03, 2021 at 10:07:19PM +0000, Chuck Lever III wrote: > >> > >> > >>>> On Dec 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: > >>> > >>> From: "J. Bruce Fields" <bfields@redhat.com> > >>> > >>> Currently nfsdcld is doing three fdatasyncs for each upcall. Based on > >>> SQLite documentation, WAL mode should also be safe, and I can confirm > >>> from an strace that it results in only one fdatasync each. > >>> > >>> This may be a bottleneck e.g. when lots of clients are being created or > >>> expired at once (e.g. on reboot). > >>> > >>> Not bothering with error checking, as this is just an optimization and > >>> nfsdcld will still function without. (Might be better to log something > >>> on failure, though.) > >> > >> I'm in full philosophical agreement for performance improvements > >> in this area. There are some caveats for WAL: > >> > >> - It requires SQLite v3.7.0 (2010). configure.ac may need to be > >> updated. > > > > Makes sense. But I dug around a bit, and how to do this is a total > > mystery to me.... > > aclocal/libsqlite3.m4 has an SQLITE_VERSION_NUMBER check. I looked got stuck trying to figure out why the #%^ it's comparing to SQLITE_VERSION_NUMBER. OK, I see now, it's just some sanity check. Here's take 2. --b. From c22d3a2d8576273934773fefac933d4f8e04776b Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" <bfields@redhat.com> Date: Fri, 3 Dec 2021 10:27:53 -0500 Subject: [PATCH] nfsdcld: use WAL journal for faster commits Currently nfsdcld is doing three fdatasyncs for each upcall. Based on SQLite documentation, WAL mode should also be safe, and I can confirm from an strace that it results in only one fdatasync each. This may be a bottleneck e.g. when lots of clients are being created or expired at once (e.g. on reboot). Not bothering with error checking, as this is just an optimization and nfsdcld will still function without. (Might be better to log something on failure, though.) Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- aclocal/libsqlite3.m4 | 2 +- utils/nfsdcld/sqlite.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/aclocal/libsqlite3.m4 b/aclocal/libsqlite3.m4 index 8c38993cbba8..c3beb4d56f0c 100644 --- a/aclocal/libsqlite3.m4 +++ b/aclocal/libsqlite3.m4 @@ -22,7 +22,7 @@ AC_DEFUN([AC_SQLITE3_VERS], [ int vers = sqlite3_libversion_number(); return vers != SQLITE_VERSION_NUMBER || - vers < 3003000; + vers < 3007000; } ], [libsqlite3_cv_is_recent=yes], [libsqlite3_cv_is_recent=no], [libsqlite3_cv_is_recent=unknown]) diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c index 03016fb95823..eabb0daa95f5 100644 --- a/utils/nfsdcld/sqlite.c +++ b/utils/nfsdcld/sqlite.c @@ -826,6 +826,8 @@ sqlite_prepare_dbh(const char *topdir) goto out_close; } + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); + ret = sqlite_query_schema_version(); switch (ret) { case CLD_SQLITE_LATEST_SCHEMA_VERSION:
> On Dec 3, 2021, at 8:24 PM, Bruce Fields <bfields@fieldses.org> wrote: > > On Sat, Dec 04, 2021 at 12:35:11AM +0000, Chuck Lever III wrote: >> >>> On Dec 3, 2021, at 5:39 PM, Bruce Fields <bfields@fieldses.org> wrote: >>> >>> On Fri, Dec 03, 2021 at 10:07:19PM +0000, Chuck Lever III wrote: >>>> >>>> >>>>>> On Dec 3, 2021, at 4:55 PM, Bruce Fields <bfields@fieldses.org> wrote: >>>>> >>>>> From: "J. Bruce Fields" <bfields@redhat.com> >>>>> >>>>> Currently nfsdcld is doing three fdatasyncs for each upcall. Based on >>>>> SQLite documentation, WAL mode should also be safe, and I can confirm >>>>> from an strace that it results in only one fdatasync each. >>>>> >>>>> This may be a bottleneck e.g. when lots of clients are being created or >>>>> expired at once (e.g. on reboot). >>>>> >>>>> Not bothering with error checking, as this is just an optimization and >>>>> nfsdcld will still function without. (Might be better to log something >>>>> on failure, though.) >>>> >>>> I'm in full philosophical agreement for performance improvements >>>> in this area. There are some caveats for WAL: >>>> >>>> - It requires SQLite v3.7.0 (2010). configure.ac may need to be >>>> updated. >>> >>> Makes sense. But I dug around a bit, and how to do this is a total >>> mystery to me.... >> >> aclocal/libsqlite3.m4 has an SQLITE_VERSION_NUMBER check. > > I looked got stuck trying to figure out why the #%^ it's comparing to > SQLITE_VERSION_NUMBER. OK, I see now, it's just some sanity check. > Here's take 2. > > --b. > > From c22d3a2d8576273934773fefac933d4f8e04776b Mon Sep 17 00:00:00 2001 > From: "J. Bruce Fields" <bfields@redhat.com> > Date: Fri, 3 Dec 2021 10:27:53 -0500 > Subject: [PATCH] nfsdcld: use WAL journal for faster commits > > Currently nfsdcld is doing three fdatasyncs for each upcall. Based on > SQLite documentation, WAL mode should also be safe, and I can confirm > from an strace that it results in only one fdatasync each. > > This may be a bottleneck e.g. when lots of clients are being created or > expired at once (e.g. on reboot). > > Not bothering with error checking, as this is just an optimization and > nfsdcld will still function without. (Might be better to log something > on failure, though.) > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> Nice. Reviewed-by: Chuck Lever <chuck.lever@oracle.com> > --- > aclocal/libsqlite3.m4 | 2 +- > utils/nfsdcld/sqlite.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/aclocal/libsqlite3.m4 b/aclocal/libsqlite3.m4 > index 8c38993cbba8..c3beb4d56f0c 100644 > --- a/aclocal/libsqlite3.m4 > +++ b/aclocal/libsqlite3.m4 > @@ -22,7 +22,7 @@ AC_DEFUN([AC_SQLITE3_VERS], [ > int vers = sqlite3_libversion_number(); > > return vers != SQLITE_VERSION_NUMBER || > - vers < 3003000; > + vers < 3007000; > } > ], [libsqlite3_cv_is_recent=yes], [libsqlite3_cv_is_recent=no], > [libsqlite3_cv_is_recent=unknown]) > diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c > index 03016fb95823..eabb0daa95f5 100644 > --- a/utils/nfsdcld/sqlite.c > +++ b/utils/nfsdcld/sqlite.c > @@ -826,6 +826,8 @@ sqlite_prepare_dbh(const char *topdir) > goto out_close; > } > > + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); > + > ret = sqlite_query_schema_version(); > switch (ret) { > case CLD_SQLITE_LATEST_SCHEMA_VERSION: > -- > 2.33.1 -- Chuck Lever
diff --git a/utils/nfsdcld/sqlite.c b/utils/nfsdcld/sqlite.c index 03016fb95823..b248eeffa204 100644 --- a/utils/nfsdcld/sqlite.c +++ b/utils/nfsdcld/sqlite.c @@ -826,6 +826,9 @@ sqlite_prepare_dbh(const char *topdir) goto out_close; } + sqlite3_exec(dbh, "PRAGMA journal_mode = WAL;", NULL, NULL, NULL); + sqlite3_exec(dbh, "PRAGMA synchronous = FULL;", NULL, NULL, NULL); + ret = sqlite_query_schema_version(); switch (ret) { case CLD_SQLITE_LATEST_SCHEMA_VERSION: