| Message ID | cover.1582530525.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring: add splice(2) support | expand |
On 2/24/20 1:32 AM, Pavel Begunkov wrote: > *on top of for-5.6 + async patches* > > Not the fastets implementation, but I'd need to stir up/duplicate > splice.c bits to do it more efficiently. > > note: rebase on top of the recent inflight patchset. Let's get this queued up, looks good to go to me. Do you have a few liburing test cases we can add for this?
On 2/24/20 8:35 AM, Jens Axboe wrote: > On 2/24/20 1:32 AM, Pavel Begunkov wrote: >> *on top of for-5.6 + async patches* >> >> Not the fastets implementation, but I'd need to stir up/duplicate >> splice.c bits to do it more efficiently. >> >> note: rebase on top of the recent inflight patchset. > > Let's get this queued up, looks good to go to me. Do you have a few > liburing test cases we can add for this? Seems to me like we have an address space issue for the off_in and off_out parameters. Why aren't we passing in pointers to these and making them work like regular splice? diff --git a/fs/io_uring.c b/fs/io_uring.c index 792ef01a521c..b0cfd68be8c9 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -448,8 +448,8 @@ struct io_epoll { struct io_splice { struct file *file_out; struct file *file_in; - loff_t off_out; - loff_t off_in; + loff_t __user *off_out; + loff_t __user *off_in; u64 len; unsigned int flags; }; @@ -2578,8 +2578,8 @@ static int io_splice_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return 0; sp->file_in = NULL; - sp->off_in = READ_ONCE(sqe->splice_off_in); - sp->off_out = READ_ONCE(sqe->off); + sp->off_in = u64_to_user_ptr(READ_ONCE(sqe->splice_off_in)); + sp->off_out = u64_to_user_ptr(READ_ONCE(sqe->off)); sp->len = READ_ONCE(sqe->len); sp->flags = READ_ONCE(sqe->splice_flags); @@ -2614,7 +2614,6 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, struct file *in = sp->file_in; struct file *out = sp->file_out; unsigned int flags = sp->flags & ~SPLICE_F_FD_IN_FIXED; - loff_t *poff_in, *poff_out; long ret; if (force_nonblock) { @@ -2623,9 +2622,7 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, flags |= SPLICE_F_NONBLOCK; } - poff_in = (sp->off_in == -1) ? NULL : &sp->off_in; - poff_out = (sp->off_out == -1) ? NULL : &sp->off_out; - ret = do_splice(in, poff_in, out, poff_out, sp->len, flags); + ret = do_splice(in, sp->off_in, out, sp->off_out, sp->len, flags); if (force_nonblock && ret == -EAGAIN) return -EAGAIN;
On 25/02/2020 01:34, Jens Axboe wrote: > On 2/24/20 8:35 AM, Jens Axboe wrote: >> On 2/24/20 1:32 AM, Pavel Begunkov wrote: >>> *on top of for-5.6 + async patches* >>> >>> Not the fastets implementation, but I'd need to stir up/duplicate >>> splice.c bits to do it more efficiently. >>> >>> note: rebase on top of the recent inflight patchset. >> >> Let's get this queued up, looks good to go to me. Do you have a few >> liburing test cases we can add for this? > > Seems to me like we have an address space issue for the off_in and Is that a problem? From the old fixing thread loop_rw_iter() it appeared to me, that it's ok to pass a kernel address as a user one. f_op->write of some implemented through the same copy_to_user(). > off_out parameters. Why aren't we passing in pointers to these > and making them work like regular splice? That's one extra copy_to_user() + copy_from_user(), which I hope to remove in the future. And I'm not really a fan of such API, and would prefer to give away such tracking to the userspace. > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 792ef01a521c..b0cfd68be8c9 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -448,8 +448,8 @@ struct io_epoll { > struct io_splice { > struct file *file_out; > struct file *file_in; > - loff_t off_out; > - loff_t off_in; > + loff_t __user *off_out; > + loff_t __user *off_in; > u64 len; > unsigned int flags; > }; > @@ -2578,8 +2578,8 @@ static int io_splice_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > return 0; > > sp->file_in = NULL; > - sp->off_in = READ_ONCE(sqe->splice_off_in); > - sp->off_out = READ_ONCE(sqe->off); > + sp->off_in = u64_to_user_ptr(READ_ONCE(sqe->splice_off_in)); > + sp->off_out = u64_to_user_ptr(READ_ONCE(sqe->off)); > sp->len = READ_ONCE(sqe->len); > sp->flags = READ_ONCE(sqe->splice_flags); > > @@ -2614,7 +2614,6 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, > struct file *in = sp->file_in; > struct file *out = sp->file_out; > unsigned int flags = sp->flags & ~SPLICE_F_FD_IN_FIXED; > - loff_t *poff_in, *poff_out; > long ret; > > if (force_nonblock) { > @@ -2623,9 +2622,7 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, > flags |= SPLICE_F_NONBLOCK; > } > > - poff_in = (sp->off_in == -1) ? NULL : &sp->off_in; > - poff_out = (sp->off_out == -1) ? NULL : &sp->off_out; > - ret = do_splice(in, poff_in, out, poff_out, sp->len, flags); > + ret = do_splice(in, sp->off_in, out, sp->off_out, sp->len, flags); > if (force_nonblock && ret == -EAGAIN) > return -EAGAIN; > >
On 25/02/2020 01:51, Pavel Begunkov wrote: > On 25/02/2020 01:34, Jens Axboe wrote: >> On 2/24/20 8:35 AM, Jens Axboe wrote: >>> On 2/24/20 1:32 AM, Pavel Begunkov wrote: >>>> *on top of for-5.6 + async patches* >>>> >>>> Not the fastets implementation, but I'd need to stir up/duplicate >>>> splice.c bits to do it more efficiently. >>>> >>>> note: rebase on top of the recent inflight patchset. >>> >>> Let's get this queued up, looks good to go to me. Do you have a few >>> liburing test cases we can add for this? >> >> Seems to me like we have an address space issue for the off_in and > > Is that a problem? From the old fixing thread loop_rw_iter() it appeared > to me, that it's ok to pass a kernel address as a user one. > f_op->write of some implemented through the same copy_to_user(). Either I finally need to check myself how the protection is implemented... > >> off_out parameters. Why aren't we passing in pointers to these >> and making them work like regular splice? > > That's one extra copy_to_user() + copy_from_user(), which I hope to remove > in the future. And I'm not really a fan of such API, and would prefer to give > away such tracking to the userspace. > >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index 792ef01a521c..b0cfd68be8c9 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -448,8 +448,8 @@ struct io_epoll { >> struct io_splice { >> struct file *file_out; >> struct file *file_in; >> - loff_t off_out; >> - loff_t off_in; >> + loff_t __user *off_out; >> + loff_t __user *off_in; >> u64 len; >> unsigned int flags; >> }; >> @@ -2578,8 +2578,8 @@ static int io_splice_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> return 0; >> >> sp->file_in = NULL; >> - sp->off_in = READ_ONCE(sqe->splice_off_in); >> - sp->off_out = READ_ONCE(sqe->off); >> + sp->off_in = u64_to_user_ptr(READ_ONCE(sqe->splice_off_in)); >> + sp->off_out = u64_to_user_ptr(READ_ONCE(sqe->off)); >> sp->len = READ_ONCE(sqe->len); >> sp->flags = READ_ONCE(sqe->splice_flags); >> >> @@ -2614,7 +2614,6 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, >> struct file *in = sp->file_in; >> struct file *out = sp->file_out; >> unsigned int flags = sp->flags & ~SPLICE_F_FD_IN_FIXED; >> - loff_t *poff_in, *poff_out; >> long ret; >> >> if (force_nonblock) { >> @@ -2623,9 +2622,7 @@ static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt, >> flags |= SPLICE_F_NONBLOCK; >> } >> >> - poff_in = (sp->off_in == -1) ? NULL : &sp->off_in; >> - poff_out = (sp->off_out == -1) ? NULL : &sp->off_out; >> - ret = do_splice(in, poff_in, out, poff_out, sp->len, flags); >> + ret = do_splice(in, sp->off_in, out, sp->off_out, sp->len, flags); >> if (force_nonblock && ret == -EAGAIN) >> return -EAGAIN; >> >> >