On Sat, Aug 08, 2020 at 01:24:02AM +0300, Nir Soffer wrote: > The new cache=none mode is misleading since it does not avoid usage of > the page cache. When using shared storage, we may get stale data from > the page cache. When writing, we flush after every write which is > inefficient and unneeded. > Hmm. This isn't actually what the cache=none parameter is currently doing. cache=none evicts pages from the cache "soon" after they are last needed by the plugin, whether that is for read or write. It's true that it does not completely avoid the page cache (so I agree it's not precisely like O_DIRECT), but it tries to minimize the time that pages spend there.

> Rename the cache modes to: > - writeback - write complete when the system call returned, and the data > was copied to the page cache. > - writethrough - write completes when the data reach storage. read marks > pages as not needed to minimize use of the page cache. I'm not convinced that writethrough is the same as cache=none. In qemu it only applies to writes, and says nothing about reads AFAIK. Where do you get the information that writethrough (in qemu, or anywhere else) marks pages as not needed if they are only read and never written?

I'm not convinced by this change.

Rich. > These terms are more aligned with other software like qemu or LIO and > common caching concepts. > > [1] https://www.linuxjournal.com/article/7105 > [2] https://www.qemu.org/docs/master/system/invocation.html > (see cache=cache) > > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; > --- > plugins/file/file.c | 22 +++++++++++----------- > plugins/file/nbdkit-file-plugin.pod | 19 +++++++++++++------ > tests/test-gzip.c | 2 +- > 3 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/plugins/file/file.c b/plugins/file/file.c > index f6f91955..deac9b7f 100644 > --- a/plugins/file/file.c > +++ b/plugins/file/file.c > @@ -76,7 +76,7 @@ static int fadvise_mode = > ; > > /* cache mode */ > -static enum { cache_default, cache_none } cache_mode = cache_default; > +static enum { cache_writeback, cache_writethrough } cache_mode = cache_writeback; > > /* Any callbacks using lseek must be protected by this lock. */ > static pthread_mutex_t lseek_lock = PTHREAD_MUTEX_INITIALIZER; > @@ -140,10 +140,10 @@ file_config (const char *key, const char *value) > } > } > else if (strcmp (key, "cache") == 0) { > - if (strcmp (value, "default") == 0) > - cache_mode = cache_default; > - else if (strcmp (value, "none") == 0) > - cache_mode = cache_none; > + if (strcmp (value, "writeback") == 0) > + cache_mode = cache_writeback; > + else if (strcmp (value, "writethrough") == 0) > + cache_mode = cache_writethrough; > else { > nbdkit_error ("unknown cache mode: %s", value); > return -1; > @@ -423,7 +423,7 @@ file_pread (void *handle, void *buf, uint32_t count, uint64_t offset, > > #ifdef HAVE_POSIX_FADVISE > /* On Linux this will evict the pages we just read from the page cache. */ > - if (cache_mode == cache_none) > + if (cache_mode == cache_writethrough) > posix_fadvise (h->fd, orig_offset, orig_count, POSIX_FADV_DONTNEED); > #endif > > @@ -441,11 +441,11 @@ file_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, > uint32_t orig_count = count; > uint64_t orig_offset = offset; > > - /* If cache=none we want to force pages we have just written to the > - * file to be flushed to disk so we can immediately evict them from > - * the page cache. > + /* If cache=writethrough we want to force pages we have just written > + * to the file to be flushed to disk so we can immediately evict them > + * from the page cache. > */ > - if (cache_mode == cache_none) flags |= NBDKIT_FLAG_FUA; > + if (cache_mode == cache_writethrough) flags |= NBDKIT_FLAG_FUA; > #endif > > while (count > 0) { > @@ -464,7 +464,7 @@ file_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, > > #ifdef HAVE_POSIX_FADVISE > /* On Linux this will evict the pages we just wrote from the page cache. */ > - if (cache_mode == cache_none) > + if (cache_mode == cache_writethrough) > posix_fadvise (h->fd, orig_offset, orig_count, POSIX_FADV_DONTNEED); > #endif > > diff --git a/plugins/file/nbdkit-file-plugin.pod b/plugins/file/nbdkit-file-plugin.pod > index e2ae0b1b..6a5bd088 100644 > --- a/plugins/file/nbdkit-file-plugin.pod > +++ b/plugins/file/nbdkit-file-plugin.pod > @@ -5,7 +5,7 @@ nbdkit-file-plugin - nbdkit file plugin > =head1 SYNOPSIS > > nbdkit file [file=]FILENAME > - [cache=default|none] [fadvise=normal|random|sequential] > + [cache=writeback|writethrough] [fadvise=normal|random|sequential] > > =head1 DESCRIPTION > > @@ -18,12 +18,19 @@ It serves the named C<FILENAME> over NBD. Local block devices > > =over 4 > > -=item B<cache=default> > +=item B<cache=writeback> > > -=item B<cache=none> > +=item B<cache=writethrough> > > -Using C<cache=none> tries to prevent the kernel from keeping parts of > -the file that have already been read or written in the page cache. > +Using C<cache=writeback>, write will be considered as completed as soon > +as the the data is store in the kernel page cache, before it reached the > +actual stoarge. > + > +Using C<cache=writethrough>, write will be considered as completed only > +when the data reach actual storage, minimizing use of kernel page cache. > +reading will try to minimize use of the page cache. > + > +The default is C<writeback>. > > =item B<fadvise=normal> > > @@ -73,7 +80,7 @@ the file sequentially one time (eg for making a single copy or backup) > then this will stop other processes from being evicted from the page > cache: > > - nbdkit file disk.img fadvise=sequential cache=none > + nbdkit file disk.img fadvise=sequential cache=writethrough > > =head2 Files on tmpfs > > diff --git a/tests/test-gzip.c b/tests/test-gzip.c > index 8f81c5b7..b1685098 100644 > --- a/tests/test-gzip.c > +++ b/tests/test-gzip.c > @@ -62,7 +62,7 @@ main (int argc, char *argv[]) > > /* Test the new filter. */ > if (test_start_nbdkit ("file", "--filter=gzip", disk, > - "fadvise=sequential", "cache=none", > + "fadvise=sequential", "cache=writethrough", > NULL) == -1) > exit (EXIT_FAILURE); > do_test (); > -- > 2.25.4 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org