<div dir="ltr"><div dir="ltr">On Wed, Mar 3, 2021 at 1:00 PM Richard W.M. Jones <<a href="mailto:rjones@redhat.com">rjones@redhat.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Mar 01, 2021 at 06:57:44PM +0200, Nir Soffer wrote:<br>
> This function is called only from page_cache_evict(), which already<br>
> check that we could map the cached pages. Add an assert to document this<br>
> assumption.<br>
> <br>
> Signed-off-by: Nir Soffer <<a href="mailto:nsoffer@redhat.com" target="_blank">nsoffer@redhat.com</a>><br>
> ---<br>
>  copy/file-ops.c | 21 +++++++++++----------<br>
>  1 file changed, 11 insertions(+), 10 deletions(-)<br>
> <br>
> diff --git a/copy/file-ops.c b/copy/file-ops.c<br>
> index 0995e92..57999cb 100644<br>
> --- a/copy/file-ops.c<br>
> +++ b/copy/file-ops.c<br>
> @@ -94,7 +94,7 @@ page_size_init (void)<br>
>  /* Load the page cache map for a particular file into<br>
>   * rwf->cached_pages.  Only used when reading files.  This doesn't<br>
>   * fail: if a system call fails then rwf->cached_pages.size will be<br>
> - * zero which is handled in page_was_cached.<br>
> + * zero which is handled in page_cache_evict.<br>
>   */<br>
>  static inline void<br>
>  page_cache_map (struct rw_file *rwf, int fd, int64_t size)<br>
> @@ -118,19 +118,16 @@ page_cache_map (struct rw_file *rwf, int fd, int64_t size)<br>
>    munmap (ptr, size);<br>
>  }<br>
>  <br>
> -/* Test if a single page of the file was cached before nbdcopy ran. */<br>
> +/* Test if a single page of the file was cached before nbdcopy ran.<br>
> + * Valid only if we mapped the cached pages.<br>
> + */<br>
>  static inline bool<br>
>  page_was_cached (struct rw_file *rwf, uint64_t offset)<br>
>  {<br>
>    uint64_t page = offset / page_size;<br>
> -  if (page < rwf->cached_pages.size)<br>
> -    return (rwf->cached_pages.ptr[page] & 1) != 0;<br>
> -  else<br>
> -    /* This path is taken if we didn't manage to map the input file<br>
> -     * for any reason.  In this case assume that pages were mapped so<br>
> -     * we will not evict them: essentially fall back to doing nothing.<br>
> -     */<br>
> -    return true;w<br>
> +  assert (page < rwf->cached_pages.size);<br>
<br>
This assert fires, but only on Fedora ppc64le.  This is the only<br>
architecture with 64K pages, I don't know if that is relevant or not.<br></blockquote><div><br></div><div>Good that we had this assert :-)</div><div><br></div><div>I think it will be safer to keep the previous approach, since a bug</div><div>in the cached pages mechanism will fail to evict pages but will not</div><div>fail the entire operation.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I wasn't able to come up with any plausible theory about why this<br>
patch is wrong, or any reproducer on x86.  I will see if I can get<br>
access to a ppc64le machine to play with this.<br>
<br>
I reverted the patch upstream in the hope it would fix the tests, but<br>
now all the copy-file-to-* tests hang (only on ppc64le) so there's<br>
still some kind of problem with the original technique that only<br>
affects Power.  I've no idea if the different page size can be the<br>
cause or if it's something else about Power.<br>
<br>
Anyhow still investigating ...<br>
<br>
Rich.<br>
<br>
<br>
> +  return (rwf->cached_pages.ptr[page] & 1) != 0;<br>
>  }<br>
>  <br>
>  /* Evict file contents from the page cache if they were not present in<br>
> @@ -142,6 +139,10 @@ page_cache_evict (struct rw_file *rwf, uint64_t orig_offset, size_t orig_len)<br>
>    uint64_t offset, n;<br>
>    size_t len;<br>
>  <br>
> +  /* If we didn't manage to map the input file for any reason, assume<br>
> +   * that pages were mapped so we will not evict them: essentially fall<br>
> +   * back to doing nothing.<br>
> +   */<br>
>    if (rwf->cached_pages.size == 0) return;<br>
>  <br>
>    /* Only bother with whole pages. */<br>
> -- <br>
> 2.26.2<br>
<br>
-- <br>
Richard Jones, Virtualization Group, Red Hat <a href="http://people.redhat.com/~rjones" rel="noreferrer" target="_blank">http://people.redhat.com/~rjones</a><br>
Read my programming and virtualization blog: <a href="http://rwmj.wordpress.com" rel="noreferrer" target="_blank">http://rwmj.wordpress.com</a><br>
libguestfs lets you edit virtual machines.  Supports shell scripting,<br>
bindings from many languages.  <a href="http://libguestfs.org" rel="noreferrer" target="_blank">http://libguestfs.org</a><br>
<br>
</blockquote></div></div>