[NAK] copy_from_iter_ops()

[Al Viro <viro@ZenIV.linux.org.uk>]

	I should have looked and commented earlier, but I hadn't spotted
that thing until -next conflicts had shown up.  As the matter of fact,
I don't have this series in my mailbox - it had been Cc'd my way, apparently,
but it looks like it never made it there, so I'm posting from scratch instead
of replying.  Sorry.

	The following "primitive" is complete crap

+#ifdef CONFIG_COPY_FROM_ITER_OPS
+size_t copy_from_iter_ops(void *addr, size_t bytes, struct iov_iter *i,
+               int (*user)(void *, const void __user *, unsigned),
+               void (*page)(char *, struct page *, size_t, size_t),
+               void (*copy)(void *, void *, unsigned))
+{
+       char *to = addr;
+
+       if (unlikely(i->type & ITER_PIPE)) {
+               WARN_ON(1);
+               return 0;
+       }
+       iterate_and_advance(i, bytes, v,
+               user((to += v.iov_len) - v.iov_len, v.iov_base,
+                                v.iov_len),
+               page((to += v.bv_len) - v.bv_len, v.bv_page, v.bv_offset,
+                               v.bv_len),
+               copy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
+       )
+
+       return bytes;
+}
+EXPORT_SYMBOL_GPL(copy_from_iter_ops);
+#endif

	1) Every time we get a new copy-from flavour of iov_iter, you will
need an extra argument and every caller will need to be updated.

	2) If it's a general-purpose primitive, it should *not* be
behind a CONFIG_<whatever> to be selected by callers.  If it isn't,
it shouldn't be there at all, period.  And no, EXPORT_SYMBOL_GPL doesn't
make it any better.

	3) The caller makes very little sense.  Is that thing meant to
be x86-only?  What are the requirements regarding writeback?  Is that thing
just go-fast stripes, or...?  Basically, all questions asked back in Decemeber
thread (memcpy_nocache()) still apply.

	I strongly object to that interface.  Let's figure out what's
really needed for your copy_from_iter_pmem() and bloody put the
iterator-related part (without the callbacks, etc.) into lib/iov_iter.c
With memcpy_to_pmem() and pmem_from_user() used by it.

	Incidentally, your fallback for memcpy_to_pmem() is... odd.
It used to be "just use memcpy()" and now it's "just do nothing".  What
the hell?  If it's really "you should not use that if you don't have
arch-specific variant", let it at least BUG(), if not fail to link.

	On the uaccess side, should pmem_from_user() zero what it had failed
to copy?  And for !@#!@# sake, comments like this
+        * On x86_64 __copy_from_user_nocache() uses non-temporal stores
+        * for the bulk of the transfer, but we need to manually flush
+        * if the transfer is unaligned. A cached memory copy is used
+        * when destination or size is not naturally aligned. That is:
+        *   - Require 8-byte alignment when size is 8 bytes or larger.
+        *   - Require 4-byte alignment when size is 4 bytes.
mean only one thing: this should live in arch/x86/lib/usercopy_64.c,
right next to the actual function that does copying.  NOT in
drivers/nvdimm/x86.c.  At the very least it needs a comment in usercopy_64.c
with dire warnings along the lines of "don't touch that code without
looking into <filename>:pmem_from_user()"...
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm