On Mon, 2018-12-17 at 19:55 +0100, Geert Uytterhoeven wrote:
> Hi Trond,
>
> (For the newly added CCs, first message was
> https://lore.kernel.org/lkml/CAMuHMdVJr0PwvJg3FeTCy7vxuyY1=s1tplho7hpsozx4wz+...@mail.gmail.com/)
>
> > On Mon, Dec 17, 2018 at 3:51 PM Trond Myklebust <
> > tron...@hammerspace.com> wrote:
> > > On Mon, 2018-12-17 at 15:03 +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Dec 5, 2018 at 3:47 PM Geert Uytterhoeven <
> > > > ge...@linux-m68k.org> wrote:
> > > > > On Wed, Dec 5, 2018 at 2:45 PM Trond Myklebust <
> > > > > tron...@hammerspace.com> wrote:
> > > > > > On Wed, 2018-12-05 at 14:41 +0100, Geert Uytterhoeven
> > > > > > wrote:
> > > > > > > On Wed, Dec 5, 2018 at 2:11 PM Atsushi Nemoto <
> > > > > > > an...@mba.ocn.ne.jp>
> > > > > > > wrote:
> > > > > > > > On Tue, 4 Dec 2018 14:53:07 +0100, Geert Uytterhoeven <
> > > > > > > > ge...@linux-m68k.org> wrote:
> > > > > > > > > I found similar crashes in a report from 2006, but of
> > > > > > > > > course the
> > > > > > > > > code
> > > > > > > > > has changed too much to apply the solution proposed
> > > > > > > > > there
> > > > > > > > > (
> > > > > > > > > https://www.linux-mips.org/archives/linux-mips/2006-09/msg00169.html
> > > > > > > > > ).
> > > > > > > > >
> > > > > > > > > Userland is Debian 8 (the last release supporting
> > > > > > > > > "old"
> > > > > > > > > MIPS).
> > > > > > > > > My kernel is based on v4.20.0-rc5, but the issue
> > > > > > > > > happens
> > > > > > > > > with
> > > > > > > > > v4.20-rc1,
> > > > > > > > > too.
> > > > > > > > >
> > > > > > > > > However, I noticed it works in v4.19! Hence I've
> > > > > > > > > bisected
> > > > > > > > > this,
> > > > > > > > > to commit
> > > > > > > > > 277e4ab7d530bf28 ("SUNRPC: Simplify TCP receive code
> > > > > > > > > by
> > > > > > > > > switching
> > > > > > > > > to using
> > > > > > > > > iterators").
> > > > > > > > >
> > > > > > > > > Dropping the ",tcp" part from the nfsroot parameter
> > > > > > > > > also
> > > > > > > > > fixes
> > > > > > > > > the issue.
> > > > > > > > >
> > > > > > > > > Given RBTX4927 is little endian, just like my
> > > > > > > > > arm/arm64
> > > > > > > > > boards,
> > > > > > > > > it's probably
> > > > > > > > > not an endianness issue. Sparse didn't show anything
> > > > > > > > > suspicious
> > > > > > > > > before/after
> > > > > > > > > the guilty commit.
> > > > > > > > >
> > > > > > > > > Do you have a clue?
> > > > > > > >
> > > > > > > > If it was a cache issue, disabling i-cache or d-cache
> > > > > > > > completely
> > > > > > > > might
> > > > > > > > help understanding the problem. I added TXx9 specific
> > > > > > > > "icdisable"
> > > > > > > > and
> > > > > > > > "dcdisable" kernel options for debugging long ago.
> > > > > > > >
> > > > > > > > I hope these options still works correctly with recent
> > > > > > > > kernel
> > > > > > > > but
> > > > > > > > not
> > > > > > > > sure.
> > > > > > > >
> > > > > > > > Also, disabling i-cache makes your board VERY slow, of
> > > > > > > > course.
> > > > > > >
> > > > > > > Thanks!
> > > > > > >
> > > > > > > When using these options, I do see a slowdown in early
> > > > > > > boot,
> > > > > > > but the
> > > > > > > issue
> > > > > > > is still there.
> > > > > > >
> > > > > > > My next guess is an unaligned access not using
> > > > > > > {get,put}_unaligned(),
> > > > > > > which
> > > > > > > doesn't seem to work on tx4927, but doesn't cause an
> > > > > > > exception
> > > > > > > neither.
> > > > > >
> > > > > > Can you try my linux-next branch on git.linux-nfs.org? It
> > > > > > contains a
> > > > > > fixes for a hang that results from the above commit.
> > > > > >
> > > > > > git pull git://git.linux-nfs.org/projects/trondmy/linux-
> > > > > > nfs.git
> > > > > > linux-next
> > > > >
> > > > > Thanks for the suggestion, but unfortunately it doesn't help.
> > > >
> > > > In the mean time, I tried your newer linux-next, no change.
> > > > I tried several other things:
> > > > - remove the packed attribute (why did you add that?),
> > >
> > > The packed attribute allows us to avoid a series of copy
> > > operations
> > > when decoding the first three elements of a RPC over TCP header
> > > (which
> > > is why they are all declared as big endian). The alternative
> > > would be
> > > to have a 12 byte buffer there for temporary storage, and then a
> > > duplicate set of 3 32-bit words into which we copy the buffer
> > > contents
> > > after extracting them from the (non-blocking) socket.
> > >
> > > > - verify (at runtime) that all accesses to fraghdr, xid, and
> > > > calldir
> > > > are aligned,
> > > > - enable RPC_DEBUG_DATA, nothing fishy seen at first sight.
> > > >
> > > > Is anyone else seeing this on MIPS, or any other platform?
> > > > Does mounting NFS with -o nfsvers=3,tcp work on other MIPS
> > > > platforms?
> > >
> > > I have no access to any MIPS hardware for the purposes of testing
> > > so
> > > that would be a question for the community.
> > >
> > > One thing that I have noticed is that unlike the old code, the
> > > bvec
> > > 'generic' code does appear to fail to call flush_dcache_page().
> > > Could
> > > that be causing the problem here? If so, why would that not be a
> > > problem in the context of regular block I/O?
>
> Thanks for the hint!
>
> It wasn't clear to me where exactly the old code called
> flush_dcache_page(), but as rpcrdma_inline_fixup() calls it in
> between
> copying to a page, and unmapping the page, I added a call to
> flush_dcache_page() to all functions in lib/iov_iter.c that map a
> page
> and copy to it, cfr. the patch below.
>
> And suddenly NFS root over TCP is working again!
Hah! ☺
>
> Note that I have no idea if it affects regular block I/O, as my
> RBTX4927
> does not have block devices.
>
> Also note that this platform does not use highmem.
>
> So, where's the proper place to fix this?
> Thanks in advance!
Given that one of the main use cases for iov_iter is the page cache, I
think that your patch below is the correct one. However perhaps Al can
comment?
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 54c248526b55fc49..5be62db33414d3f9 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -277,6 +277,7 @@ static size_t copy_page_from_iter_iovec(struct
> page *page, size_t offset, size_t
> to += copy;
> bytes -= copy;
> }
> + flush_dcache_page(page);
> if (likely(!bytes)) {
> kunmap_atomic(kaddr);
> goto done;
> @@ -463,6 +464,7 @@ static void memcpy_to_page(struct page *page,
> size_t offset, const char *from, s
> {
> char *to = kmap_atomic(page);
> memcpy(to + offset, from, len);
> + flush_dcache_page(page);
> kunmap_atomic(to);
> }
>
> @@ -470,6 +472,7 @@ static void memzero_page(struct page *page,
> size_t offset, size_t len)
> {
> char *addr = kmap_atomic(page);
> memset(addr + offset, 0, len);
> + flush_dcache_page(page);
> kunmap_atomic(addr);
> }
>
> @@ -580,6 +583,7 @@ static size_t csum_and_copy_to_pipe_iter(const
> void *addr, size_t bytes,
> char *p = kmap_atomic(pipe->bufs[idx].page);
> next = csum_partial_copy_nocheck(addr, p + r, chunk,
> 0);
> sum = csum_block_add(sum, next, off);
> + flush_dcache_page(pipe->bufs[idx].page);
> kunmap_atomic(p);
> i->idx = idx;
> i->iov_offset = r + chunk;
> @@ -628,6 +632,7 @@ static unsigned long memcpy_mcsafe_to_page(struct
> page *page, size_t offset,
>
> to = kmap_atomic(page);
> ret = memcpy_mcsafe(to + offset, from, len);
> + flush_dcache_page(page);
> kunmap_atomic(to);
>
> return ret;
> @@ -894,6 +899,7 @@ size_t copy_page_from_iter(struct page *page,
> size_t offset, size_t bytes,
> if (i->type & (ITER_BVEC|ITER_KVEC)) {
> void *kaddr = kmap_atomic(page);
> size_t wanted = _copy_from_iter(kaddr + offset, bytes,
> i);
> + flush_dcache_page(page);
> kunmap_atomic(kaddr);
> return wanted;
> } else
> @@ -958,6 +964,7 @@ size_t iov_iter_copy_from_user_atomic(struct page
> *page,
> v.bv_offset, v.bv_len),
> memcpy((p += v.iov_len) - v.iov_len, v.iov_base,
> v.iov_len)
> )
> + flush_dcache_page(page);
> kunmap_atomic(kaddr);
> return bytes;
> }
> @@ -1494,6 +1501,7 @@ size_t csum_and_copy_to_iter(const void *addr,
> size_t bytes, __wsum *csum,
> next = csum_partial_copy_nocheck((from += v.bv_len) -
> v.bv_len,
> p + v.bv_offset,
> v.bv_len, 0);
> + flush_dcache_page(v.bv_page);
> kunmap_atomic(p);
> sum = csum_block_add(sum, next, off);
> off += v.bv_len;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.mykleb...@hammerspace.com
