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

[Nir Soffer]

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.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210303/d50a2dd8/attachment.htm>