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

On Wed, Mar 3, 2021 at 1:00 PM Richard W.M. Jones <rjones(a)redhat.com&gt; wrote: 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(a)redhat.com&gt; > --- >  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;w > +  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. Good that we had this assert :-) I think it will be safer to keep the previous approach, since a bug in the cached pages mechanism will fail to evict pages but will not fail the entire operation.

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