Libguestfs - Mar 2021 - [PATCH libnbd 2/4] copy: file-ops.c: Remove unneeded check

This function is called only from page_cache_evict(), which already
check that we could map the cached pages. Add an assert to document this
assumption.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/file-ops.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/copy/file-ops.c b/copy/file-ops.c
index 0995e92..57999cb 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -94,7 +94,7 @@ page_size_init (void)
 /* Load the page cache map for a particular file into
  * rwf->cached_pages.  Only used when reading files.  This doesn't
  * fail: if a system call fails then rwf->cached_pages.size will be
- * zero which is handled in page_was_cached.
+ * zero which is handled in page_cache_evict.
  */
 static inline void
 page_cache_map (struct rw_file *rwf, int fd, int64_t size)
@@ -118,19 +118,16 @@ page_cache_map (struct rw_file *rwf, int fd, int64_t size)
   munmap (ptr, size);
 }
 
-/* Test if a single page of the file was cached before nbdcopy ran. */
+/* Test if a single page of the file was cached before nbdcopy ran.
+ * Valid only if we mapped the cached pages.
+ */
 static inline bool
 page_was_cached (struct rw_file *rwf, uint64_t offset)
 {
   uint64_t page = offset / page_size;
-  if (page < rwf->cached_pages.size)
-    return (rwf->cached_pages.ptr[page] & 1) != 0;
-  else
-    /* This path is taken if we didn't manage to map the input file
-     * for any reason.  In this case assume that pages were mapped so
-     * we will not evict them: essentially fall back to doing nothing.
-     */
-    return true;
+  assert (page < rwf->cached_pages.size);
+
+  return (rwf->cached_pages.ptr[page] & 1) != 0;
 }
 
 /* Evict file contents from the page cache if they were not present in
@@ -142,6 +139,10 @@ page_cache_evict (struct rw_file *rwf, uint64_t
orig_offset, size_t orig_len)
   uint64_t offset, n;
   size_t len;
 
+  /* If we didn't manage to map the input file for any reason, assume
+   * that pages were mapped so we will not evict them: essentially fall
+   * back to doing nothing.
+   */
   if (rwf->cached_pages.size == 0) return;
 
   /* Only bother with whole pages. */
-- 
2.26.2

On Mon, Mar 01, 2021 at 06:57:44PM +0200, Nir Soffer
wrote:
> This function is called only from page_cache_evict(), which already
> check that we could map the cached pages. Add an assert to document this
> assumption.
> 
> Signed-off-by: Nir Soffer <nsoffer at redhat.com>
> ---
>  copy/file-ops.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/copy/file-ops.c b/copy/file-ops.c
> index 0995e92..57999cb 100644
> --- a/copy/file-ops.c
> +++ b/copy/file-ops.c
> @@ -94,7 +94,7 @@ page_size_init (void)
>  /* Load the page cache map for a particular file into
>   * rwf->cached_pages.  Only used when reading files.  This doesn't
>   * fail: if a system call fails then rwf->cached_pages.size will be
> - * zero which is handled in page_was_cached.
> + * zero which is handled in page_cache_evict.
>   */
>  static inline void
>  page_cache_map (struct rw_file *rwf, int fd, int64_t size)
> @@ -118,19 +118,16 @@ page_cache_map (struct rw_file *rwf, int fd, int64_t
size)
>    munmap (ptr, size);
>  }
>  
> -/* Test if a single page of the file was cached before nbdcopy ran. */
> +/* Test if a single page of the file was cached before nbdcopy ran.
> + * Valid only if we mapped the cached pages.
> + */
>  static inline bool
>  page_was_cached (struct rw_file *rwf, uint64_t offset)
>  {
>    uint64_t page = offset / page_size;
> -  if (page < rwf->cached_pages.size)
> -    return (rwf->cached_pages.ptr[page] & 1) != 0;
> -  else
> -    /* This path is taken if we didn't manage to map the input file
> -     * for any reason.  In this case assume that pages were mapped so
> -     * we will not evict them: essentially fall back to doing nothing.
> -     */
> -    return true;
> +  assert (page < rwf->cached_pages.size);
This assert fires, but only on Fedora ppc64le.  This is the only
architecture with 64K pages, I don't know if that is relevant or not.

I wasn't able to come up with any plausible theory about why this
patch is wrong, or any reproducer on x86.  I will see if I can get
access to a ppc64le machine to play with this.

I reverted the patch upstream in the hope it would fix the tests, but
now all the copy-file-to-* tests hang (only on ppc64le) so there's
still some kind of problem with the original technique that only
affects Power.  I've no idea if the different page size can be the
cause or if it's something else about Power.

Anyhow still investigating ...

Rich.

> +  return (rwf->cached_pages.ptr[page] & 1) != 0;
>  }
>  
>  /* Evict file contents from the page cache if they were not present in
> @@ -142,6 +139,10 @@ page_cache_evict (struct rw_file *rwf, uint64_t
orig_offset, size_t orig_len)
>    uint64_t offset, n;
>    size_t len;
>  
> +  /* If we didn't manage to map the input file for any reason, assume
> +   * that pages were mapped so we will not evict them: essentially fall
> +   * back to doing nothing.
> +   */
>    if (rwf->cached_pages.size == 0) return;
>  
>    /* Only bother with whole pages. */
> -- 
> 2.26.2
-- 
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