On 2/11/23 8:22?PM, Ming Lei wrote: >>>> Also seems like this should be separately testable. We can't add new >>>> opcodes that don't have a feature test at least, and should also have >>>> various corner case tests. A bit of commenting outside of this below. >>> >>> OK, I will write/add one very simple ublk userspace to liburing for >>> test purpose. >> >> Thanks! > > Thinking of further, if we use ublk for liburing test purpose, root is > often needed, even though we support un-privileged mode, which needs > administrator to grant access, so is it still good to do so? That's fine, some tests already depend on root for certain things, like passthrough. When I run the tests, I do a pass as both a regular user and as root. The important bit is just that the tests skip when they are not root rather than fail. > It could be easier to add ->splice_read() on /dev/zero for test > purpose, just allocate zeroed pages in ->splice_read(), and add > them to pipe like ublk->splice_read(), and sink side can read > from or write to these pages, but zero's read_iter_zero() won't > be affected. And normal splice/tee won't connect to zero too > because we only allow it from kernel use. Arguably /dev/zero should still support splice_read() as a regression fix as I argued to Linus, so I'd just add that as a prep patch. >>>> Seems like this should check for SPLICE_F_FD_IN_FIXED, and also use >>>> io_file_get_normal() for the non-fixed case in case someone passed in an >>>> io_uring fd. >>> >>> SPLICE_F_FD_IN_FIXED needs one extra word for holding splice flags, if >>> we can use sqe->addr3, I think it is doable. >> >> I haven't checked the rest, but you can't just use ->splice_flags for >> this? > > ->splice_flags shares memory with rwflags, so can't be used. > > I think it is fine to use ->addr3, given io_getxattr()/io_setxattr()/ > io_msg_ring() has used that. This is part of the confusion, as you treat it basically like a read/write internally, but the opcode names indicate differently. Why not just have a separate prep helper for these and then use a layout that makes more sense, surely rwflags aren't applicable for these anyway? I think that'd make it a lot cleaner. Yeah, addr3 could easily be used, but it's makes for a really confusing command structure when the command is kinda-read but also kinda-splice. And it arguable makes more sense to treat it as the latter, as it takes the two fds like splice. -- Jens Axboe
