[Libguestfs] [PATCH libnbd 2/4] copy: file-ops.c: Remove unneeded check
Nir Soffer
nsoffer at redhat.com
Wed Mar 3 11:43:09 UTC 2021
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>
More information about the Libguestfs
mailing list