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

[Richard W.M. Jones]

On Wed, Mar 03, 2021 at 01:43:09PM +0200, Nir Soffer wrote:
> On Wed, Mar 3, 2021 at 1:00 PM Richard W.M. Jones <rjones at redhat.com> 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 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;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.

Actually I believe your assertion was correct.  The problem was that
length calculation overflowed and it was trying to evict a huge number
of pages after the end of the file.  With your assertion it failed
early.  Without, it sat in a loop (in the "return true" branch) for a
very long time.

I have restored this patch and added a fix for the overflow in the
calculation.

Rich.

> 
>     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
> 
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html