* [PATCH 03/18] block: add bio_set_polled() helper
From: Jens Axboe @ 2019-01-29 19:26 UTC (permalink / raw)
To: linux-aio, linux-block, linux-api; +Cc: hch, jmoyer, avi, jannh, Jens Axboe
In-Reply-To: <20190129192702.3605-1-axboe@kernel.dk>
For the upcoming async polled IO, we can't sleep allocating requests.
If we do, then we introduce a deadlock where the submitter already
has async polled IO in-flight, but can't wait for them to complete
since polled requests must be active found and reaped.
Utilize the helper in the blockdev DIRECT_IO code.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/block_dev.c | 4 ++--
include/linux/bio.h | 14 ++++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index f18d076a2596..392e2bfb636f 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -247,7 +247,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
task_io_account_write(ret);
}
if (iocb->ki_flags & IOCB_HIPRI)
- bio.bi_opf |= REQ_HIPRI;
+ bio_set_polled(&bio, iocb);
qc = submit_bio(&bio);
for (;;) {
@@ -415,7 +415,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
nr_pages = iov_iter_npages(iter, BIO_MAX_PAGES);
if (!nr_pages) {
if (iocb->ki_flags & IOCB_HIPRI)
- bio->bi_opf |= REQ_HIPRI;
+ bio_set_polled(bio, iocb);
qc = submit_bio(bio);
WRITE_ONCE(iocb->ki_cookie, qc);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7380b094dcca..f6f0a2b3cbc8 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -823,5 +823,19 @@ static inline int bio_integrity_add_page(struct bio *bio, struct page *page,
#endif /* CONFIG_BLK_DEV_INTEGRITY */
+/*
+ * Mark a bio as polled. Note that for async polled IO, the caller must
+ * expect -EWOULDBLOCK if we cannot allocate a request (or other resources).
+ * We cannot block waiting for requests on polled IO, as those completions
+ * must be found by the caller. This is different than IRQ driven IO, where
+ * it's safe to wait for IO to complete.
+ */
+static inline void bio_set_polled(struct bio *bio, struct kiocb *kiocb)
+{
+ bio->bi_opf |= REQ_HIPRI;
+ if (!is_sync_kiocb(kiocb))
+ bio->bi_opf |= REQ_NOWAIT;
+}
+
#endif /* CONFIG_BLOCK */
#endif /* __LINUX_BIO_H */
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 02/18] block: wire up block device iopoll method
From: Jens Axboe @ 2019-01-29 19:26 UTC (permalink / raw)
To: linux-aio, linux-block, linux-api; +Cc: hch, jmoyer, avi, jannh, Jens Axboe
In-Reply-To: <20190129192702.3605-1-axboe@kernel.dk>
From: Christoph Hellwig <hch@lst.de>
Just call blk_poll on the iocb cookie, we can derive the block device
from the inode trivially.
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/block_dev.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 58a4c1217fa8..f18d076a2596 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -293,6 +293,14 @@ struct blkdev_dio {
static struct bio_set blkdev_dio_pool;
+static int blkdev_iopoll(struct kiocb *kiocb, bool wait)
+{
+ struct block_device *bdev = I_BDEV(kiocb->ki_filp->f_mapping->host);
+ struct request_queue *q = bdev_get_queue(bdev);
+
+ return blk_poll(q, READ_ONCE(kiocb->ki_cookie), wait);
+}
+
static void blkdev_bio_end_io(struct bio *bio)
{
struct blkdev_dio *dio = bio->bi_private;
@@ -410,6 +418,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
bio->bi_opf |= REQ_HIPRI;
qc = submit_bio(bio);
+ WRITE_ONCE(iocb->ki_cookie, qc);
break;
}
@@ -2076,6 +2085,7 @@ const struct file_operations def_blk_fops = {
.llseek = block_llseek,
.read_iter = blkdev_read_iter,
.write_iter = blkdev_write_iter,
+ .iopoll = blkdev_iopoll,
.mmap = generic_file_mmap,
.fsync = blkdev_fsync,
.unlocked_ioctl = block_ioctl,
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 01/18] fs: add an iopoll method to struct file_operations
From: Jens Axboe @ 2019-01-29 19:26 UTC (permalink / raw)
To: linux-aio, linux-block, linux-api; +Cc: hch, jmoyer, avi, jannh, Jens Axboe
In-Reply-To: <20190129192702.3605-1-axboe@kernel.dk>
From: Christoph Hellwig <hch@lst.de>
This new methods is used to explicitly poll for I/O completion for an
iocb. It must be called for any iocb submitted asynchronously (that
is with a non-null ki_complete) which has the IOCB_HIPRI flag set.
The method is assisted by a new ki_cookie field in struct iocb to store
the polling cookie.
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
Documentation/filesystems/vfs.txt | 3 +++
include/linux/fs.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 8dc8e9c2913f..761c6fd24a53 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -857,6 +857,7 @@ struct file_operations {
ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
ssize_t (*read_iter) (struct kiocb *, struct iov_iter *);
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
+ int (*iopoll)(struct kiocb *kiocb, bool spin);
int (*iterate) (struct file *, struct dir_context *);
int (*iterate_shared) (struct file *, struct dir_context *);
__poll_t (*poll) (struct file *, struct poll_table_struct *);
@@ -902,6 +903,8 @@ otherwise noted.
write_iter: possibly asynchronous write with iov_iter as source
+ iopoll: called when aio wants to poll for completions on HIPRI iocbs
+
iterate: called when the VFS needs to read the directory contents
iterate_shared: called when the VFS needs to read the directory contents
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 811c77743dad..ccb0b7a63aa5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -310,6 +310,7 @@ struct kiocb {
int ki_flags;
u16 ki_hint;
u16 ki_ioprio; /* See linux/ioprio.h */
+ unsigned int ki_cookie; /* for ->iopoll */
} __randomize_layout;
static inline bool is_sync_kiocb(struct kiocb *kiocb)
@@ -1786,6 +1787,7 @@ struct file_operations {
ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
ssize_t (*read_iter) (struct kiocb *, struct iov_iter *);
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
+ int (*iopoll)(struct kiocb *kiocb, bool spin);
int (*iterate) (struct file *, struct dir_context *);
int (*iterate_shared) (struct file *, struct dir_context *);
__poll_t (*poll) (struct file *, struct poll_table_struct *);
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCHSET v9] io_uring IO interface
From: Jens Axboe @ 2019-01-29 19:26 UTC (permalink / raw)
To: linux-aio, linux-block, linux-api; +Cc: hch, jmoyer, avi, jannh
Following up on all the great review from yesterday (and today),
here's a v9 that addresses all known review concerns so far.
A particular big thanks to Jann Horn for looking into the grittier
details, which resulted in a slew of fixes. Also thanks to Christoph
for working through the patches. I feel like we're making good
progress here.
A note on ctx->compat still being there - we could store this in
struct sqe_submit, but this doesn't work for the io_sq_thread()
polled submission. Additionally, makes more sense to keep this in
the ctx instead of once per IO.
No new changes in the liburing user side library, but as a reference,
you can clone that here:
git://git.kernel.dk/liburing
We're still missing a man page for io_uring_enter(2), but the two other
system calls are documented.
Patches are against 5.0-rc4, and can also be found in my io_uring branch
here:
git://git.kernel.dk/linux-block io_uring
Changes since v8:
- Check for p->sq_thread_cpu being possible
- Check for valid flags in io_uring_enter(2)
- Cap 'to_submit' at SQ ring size in io_uring_enter(2)
- Fix files/mm references
- Don't bother with ctx referencing in io_uring_register(2)
- Use READ/WRITE_ONCE for ring updates/reads
- Use percpu_ref_tryget() for io_get_req()
- Protect sqe reads (that matter) with READ_ONCE()
- Store compat syscall info in the ctx. Still derived from
in_compat_syscall(), but we need access to it from the io_sq_thread()
as well.
- Don't make IORING_MAX_ENTRIES user visible
- Address various review comments
Documentation/filesystems/vfs.txt | 3 +
arch/x86/entry/syscalls/syscall_32.tbl | 3 +
arch/x86/entry/syscalls/syscall_64.tbl | 3 +
block/bio.c | 59 +-
fs/Makefile | 1 +
fs/block_dev.c | 19 +-
fs/file.c | 15 +-
fs/file_table.c | 9 +-
fs/gfs2/file.c | 2 +
fs/io_uring.c | 2599 ++++++++++++++++++++++++
fs/iomap.c | 48 +-
fs/xfs/xfs_file.c | 1 +
include/linux/bio.h | 14 +
include/linux/blk_types.h | 1 +
include/linux/file.h | 2 +
include/linux/fs.h | 6 +-
include/linux/iomap.h | 1 +
include/linux/sched/user.h | 2 +-
include/linux/syscalls.h | 8 +
include/uapi/asm-generic/unistd.h | 8 +-
include/uapi/linux/io_uring.h | 141 ++
init/Kconfig | 9 +
kernel/sys_ni.c | 3 +
23 files changed, 2916 insertions(+), 41 deletions(-)
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 07/18] io_uring: support for IO polling
From: Jens Axboe @ 2019-01-29 19:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-block, linux-man, linux-api, jmoyer, avi
In-Reply-To: <f21fc281-bbaa-3af5-3d64-a55933f1cbfb@kernel.dk>
On 1/29/19 11:31 AM, Jens Axboe wrote:
>> The code looks a little clumsy to me. Why not:
>>
>> while (!list_empty(&ctx->poll_list)) {
>> int ret = io_do_iopoll(ctx, nr_events, min);
>> if (ret)
>> return ret;
>>
>> if (!min || *nr_events >= min)
>> return 0;
>> }
>>
>> return 1;
>
> I think you messed up the 0/1 here, how about this:
>
> while (!list_empty(&ctx->poll_list)) {
> int ret;
>
> ret = io_do_iopoll(ctx, nr_events, min);
> if (ret < 0)
> return ret;
> if (!min || *nr_events >= min)
> return 1;
> }
>
> return 0;
Or I did... I think yours is correct.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 07/18] io_uring: support for IO polling
From: Jens Axboe @ 2019-01-29 18:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-block, linux-man, linux-api, jmoyer, avi
In-Reply-To: <20190129172414.GA15347@lst.de>
On 1/29/19 10:24 AM, Christoph Hellwig wrote:
>>
>> @@ -118,12 +120,16 @@ struct io_kiocb {
>> struct list_head list;
>> unsigned int flags;
>> #define REQ_F_FORCE_NONBLOCK 1 /* inline submission attempt */
>> +#define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
>> +#define REQ_F_IOPOLL_EAGAIN 4 /* submission got EAGAIN */
>> u64 user_data;
>> + u64 res;
>
> Should this be ret or error instead? res is kinda off. A little
> comment describing it won't hurt either. Last but not least with
> the actual errno value stored here we probably don't need the
> REQ_F_IOPOLL_EAGAIN flag, do we?
Yes good point, that flag pre-dates us having the error in there. I'll
rename the field, too.
>> + /*
>> + * Only spin for completions if we don't have multiple devices hanging
>> + * off our complete list, and we're under the requested amount.
>> + */
>> + spin = !ctx->poll_multi_file && (*nr_events < min);
>
> no need for the braces here.
Killed
>> +static int io_iopoll_getevents(struct io_ring_ctx *ctx, unsigned int *nr_events,
>> + long min)
>> +{
>> + int ret;
>> +
>> + do {
>> + if (list_empty(&ctx->poll_list))
>> + return 0;
>> +
>> + ret = io_do_iopoll(ctx, nr_events, min);
>> + if (ret < 0)
>> + break;
>> + } while (min && *nr_events < min);
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + return *nr_events < min;
>
> The code looks a little clumsy to me. Why not:
>
> while (!list_empty(&ctx->poll_list)) {
> int ret = io_do_iopoll(ctx, nr_events, min);
> if (ret)
> return ret;
>
> if (!min || *nr_events >= min)
> return 0;
> }
>
> return 1;
I think you messed up the 0/1 here, how about this:
while (!list_empty(&ctx->poll_list)) {
int ret;
ret = io_do_iopoll(ctx, nr_events, min);
if (ret < 0)
return ret;
if (!min || *nr_events >= min)
return 1;
}
return 0;
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 10/18] io_uring: batch io_kiocb allocation
From: Jens Axboe @ 2019-01-29 18:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-aio, linux-block, linux-man, linux-api, jmoyer, avi
In-Reply-To: <20190129172624.GB15347@lst.de>
On 1/29/19 10:26 AM, Christoph Hellwig wrote:
>> -static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
>> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
>> + struct io_submit_state *state)
>> {
>> + gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
>
> No actually in this patch, but why do we do GFP_ATOMIC allocations
> here? We aren't in irq context or under a spinlock.
Just to avoid dipping into more expensive allocs. Probably not a big
deal, I'll remove the atomic.
>
>> + if (!state)
>> + req = kmem_cache_alloc(req_cachep, gfp);
>
> Add a
> if (!req)
> goto out;
>
> plus the missing braces here..
>
>> + else if (!state->free_reqs) {
>> + size_t sz;
>> + int ret;
>> +
>> + sz = min_t(size_t, state->ios_left, ARRAY_SIZE(state->reqs));
>> + ret = kmem_cache_alloc_bulk(req_cachep, gfp, sz,
>> + state->reqs);
>> + if (ret <= 0)
>> + goto out;
>> + state->free_reqs = ret - 1;
>> + state->cur_req = 1;
>> + req = state->reqs[0];
>> + } else {
>> + req = state->reqs[state->cur_req];
>> + state->free_reqs--;
>> + state->cur_req++;
>> + }
>> +
>> if (req) {
>> req->ctx = ctx;
>> req->flags = 0;
>> return req;
>> }
>
> ... and we don't need this conditional, which would otherwise also
> be in the fast path.
Good idea, done.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 13/18] io_uring: add file set registration
From: Jens Axboe @ 2019-01-29 18:13 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez2tYBePMCkd-EOdAX1gfVwPJ-fTd-8XKu6J-QPyTXHN9g@mail.gmail.com>
On 1/29/19 9:36 AM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>> We normally have to fget/fput for each IO we do on a file. Even with
>> the batching we do, the cost of the atomic inc/dec of the file usage
>> count adds up.
>>
>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>> for the io_uring_register(2) system call. The arguments passed in must
>> be an array of __s32 holding file descriptors, and nr_args should hold
>> the number of file descriptors the application wishes to pin for the
>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>> called).
>>
>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>> to the index in the array passed in to IORING_REGISTER_FILES.
>>
>> Files are automatically unregistered when the io_uring context is
>> torn down. An application need only unregister if it wishes to
>> register a few set of fds.
>
> s/few/new/ ?
Indeed, thanks.
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 682714d6f217..77993972879b 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -98,6 +98,10 @@ struct io_ring_ctx {
>> struct fasync_struct *cq_fasync;
>> } ____cacheline_aligned_in_smp;
>>
>> + /* if used, fixed file set */
>> + struct file **user_files;
>> + unsigned nr_user_files;
>
> It'd be nice if you could add a comment about locking rules here -
> something like "writers must ensure that ->refs is dead, readers must
> ensure that ->refs is alive as long as the file* is used".
Will add.
> [...]
>> @@ -612,7 +625,14 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> struct kiocb *kiocb = &req->rw;
>> int ret;
>>
>> - kiocb->ki_filp = io_file_get(state, sqe->fd);
>> + if (sqe->flags & IOSQE_FIXED_FILE) {
>> + if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
>> + return -EBADF;
>> + kiocb->ki_filp = ctx->user_files[sqe->fd];
>
> It doesn't really matter as long as ctx->nr_user_files<=INT_MAX, but
> it'd be nice if you could explicitly cast sqe->fd to unsigned here.
OK, will do.
>> + req->flags |= REQ_F_FIXED_FILE;
>> + } else {
>> + kiocb->ki_filp = io_file_get(state, sqe->fd);
>> + }
> [...]
>> +static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
>> + unsigned nr_args)
>> +{
>> + __s32 __user *fds = (__s32 __user *) arg;
>> + int fd, i, ret = 0;
>> +
>> + if (ctx->user_files)
>> + return -EBUSY;
>> + if (!nr_args)
>> + return -EINVAL;
>> +
>> + ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
>> + if (!ctx->user_files)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < nr_args; i++) {
>> + ret = -EFAULT;
>> + if (copy_from_user(&fd, &fds[i], sizeof(fd)))
>> + break;
>
> "i" is signed, but "nr_args" is unsigned. You can't get through that
> kcalloc() call with nr_args>=0x80000000 on a normal kernel, someone
> would have to set CONFIG_FORCE_MAX_ZONEORDER really high for that, but
> still, in theory, if you reach this copy_to_user(..., &fds[i], ...)
> with a negative "i", that'd be bad. You might want to make "i"
> unsigned here and check that it's at least smaller than UINT_MAX...
Done.
Thanks for your review!
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 15/18] io_uring: add io_kiocb ref count
From: Christoph Hellwig @ 2019-01-29 17:26 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <20190128213538.13486-16-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 02:35:35PM -0700, Jens Axboe wrote:
> We'll use this for the POLL implementation. Regular requests will
> NOT be using references, so initialize it to 0. Any real use of
> the io_kiocb ref will initialize it to at least 2.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 10/18] io_uring: batch io_kiocb allocation
From: Christoph Hellwig @ 2019-01-29 17:26 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <20190128213538.13486-11-axboe@kernel.dk>
> -static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
> + struct io_submit_state *state)
> {
> + gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
No actually in this patch, but why do we do GFP_ATOMIC allocations
here? We aren't in irq context or under a spinlock.
> + if (!state)
> + req = kmem_cache_alloc(req_cachep, gfp);
Add a
if (!req)
goto out;
plus the missing braces here..
> + else if (!state->free_reqs) {
> + size_t sz;
> + int ret;
> +
> + sz = min_t(size_t, state->ios_left, ARRAY_SIZE(state->reqs));
> + ret = kmem_cache_alloc_bulk(req_cachep, gfp, sz,
> + state->reqs);
> + if (ret <= 0)
> + goto out;
> + state->free_reqs = ret - 1;
> + state->cur_req = 1;
> + req = state->reqs[0];
> + } else {
> + req = state->reqs[state->cur_req];
> + state->free_reqs--;
> + state->cur_req++;
> + }
> +
> if (req) {
> req->ctx = ctx;
> req->flags = 0;
> return req;
> }
... and we don't need this conditional, which would otherwise also
be in the fast path.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 07/18] io_uring: support for IO polling
From: Christoph Hellwig @ 2019-01-29 17:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <20190128213538.13486-8-axboe@kernel.dk>
>
> @@ -118,12 +120,16 @@ struct io_kiocb {
> struct list_head list;
> unsigned int flags;
> #define REQ_F_FORCE_NONBLOCK 1 /* inline submission attempt */
> +#define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
> +#define REQ_F_IOPOLL_EAGAIN 4 /* submission got EAGAIN */
> u64 user_data;
> + u64 res;
Should this be ret or error instead? res is kinda off. A little
comment describing it won't hurt either. Last but not least with
the actual errno value stored here we probably don't need the
REQ_F_IOPOLL_EAGAIN flag, do we?
> + /*
> + * Only spin for completions if we don't have multiple devices hanging
> + * off our complete list, and we're under the requested amount.
> + */
> + spin = !ctx->poll_multi_file && (*nr_events < min);
no need for the braces here.
> +static int io_iopoll_getevents(struct io_ring_ctx *ctx, unsigned int *nr_events,
> + long min)
> +{
> + int ret;
> +
> + do {
> + if (list_empty(&ctx->poll_list))
> + return 0;
> +
> + ret = io_do_iopoll(ctx, nr_events, min);
> + if (ret < 0)
> + break;
> + } while (min && *nr_events < min);
> +
> + if (ret < 0)
> + return ret;
> +
> + return *nr_events < min;
The code looks a little clumsy to me. Why not:
while (!list_empty(&ctx->poll_list)) {
int ret = io_do_iopoll(ctx, nr_events, min);
if (ret)
return ret;
if (!min || *nr_events >= min)
return 0;
}
return 1;
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC PATCH glibc 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v6)
From: Mathieu Desnoyers @ 2019-01-29 16:57 UTC (permalink / raw)
To: carlos, Florian Weimer
Cc: Joseph Myers, Szabolcs Nagy, libc-alpha, Thomas Gleixner,
Ben Maurer, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Will Deacon, Dave Watson, Paul Turner, Rich Felker, linux-kernel,
linux-api
In-Reply-To: <20190121213530.23803-1-mathieu.desnoyers@efficios.com>
----- On Jan 21, 2019, at 4:35 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
[...]
> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h
> b/sysdeps/unix/sysv/linux/sys/rseq.h
> new file mode 100644
> index 0000000000..61937fb193
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h
> @@ -0,0 +1,64 @@
[...]
> +
> +#ifndef _SYS_RSEQ_H
> +#define _SYS_RSEQ_H 1
> +
> +/* We use the structures declarations from the kernel headers. */
> +#include <linux/rseq.h>
> +#include <stdint.h>
> +
> +/* Signature required before each abort handler code. */
> +#define RSEQ_SIG 0x53053053
I recalled that aarch64 defines RSEQ_SIG to a different value which maps to
a valid trap instruction. So I plan to move the RSEQ_SIG define to per-arch
headers like this:
sysdeps/unix/sysv/linux/aarch64/bits/rseq.h | 24 ++
sysdeps/unix/sysv/linux/arm/bits/rseq.h | 24 ++
sysdeps/unix/sysv/linux/bits/rseq.h | 23 ++
sysdeps/unix/sysv/linux/mips/bits/rseq.h | 24 ++
sysdeps/unix/sysv/linux/powerpc/bits/rseq.h | 24 ++
sysdeps/unix/sysv/linux/s390/bits/rseq.h | 24 ++
sysdeps/unix/sysv/linux/x86/bits/rseq.h | 24 ++
where "bits/rseq.h" contains a #error:
# error "Architecture does not define RSEQ_SIG.
sys/rseq.h will now include <bits/rseq.h>.
> +
> +enum rseq_register_state
> +{
> + /* Value RSEQ_REGISTER_ALLOWED means it is allowed to update
> + the refcount field and to register/unregister rseq. */
> + RSEQ_REGISTER_ALLOWED = 0,
> + /* Value RSEQ_REGISTER_NESTED means it is temporarily forbidden
> + to update the refcount field or to register/unregister rseq. */
> + RSEQ_REGISTER_NESTED = 1,
I plan to rename "RSEQ_REGISTER_NESTED" to "RSEQ_REGISTER_ONGOING" which
seems to better represent the current registration state.
Please let me know if anything is wrong with those changes.
Thanks,
Mathieu
> + /* Value RSEQ_REGISTER_EXITING means it is forbidden to update the
> + refcount field or to register/unregister rseq for the rest of the
> + thread's lifetime. */
> + RSEQ_REGISTER_EXITING = 2,
> +};
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Christoph Hellwig @ 2019-01-29 16:55 UTC (permalink / raw)
To: Jann Horn
Cc: Jens Axboe, Florian Weimer, linux-aio, linux-block, Linux API,
hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez0PPWP7GU5kinzejN1B3invrcxCZT2fkpnHxMK2F0pj6A@mail.gmail.com>
On Tue, Jan 29, 2019 at 04:38:36PM +0100, Jann Horn wrote:
> > >> +#define IORING_MAX_ENTRIES 4096
> > >
> > > Where does this constant come from? Should it really be exposed to
> > > userspace?
> >
> > Seems pretty handy for the application to know what the limit is?
>
> The running kernel version might be different from the kernel version
> whose headers the userspace code was compiled with. So if this is a
> limit that might change at some point in the future, and you think
> userspace wants to know the limit, some other API might be better.
If it changes it will increase, so I'm not worried it is all that
harmful. What might be useful is a specific error code if it is too
large so that applications can check it. Just which one?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Arnd Bergmann @ 2019-01-29 16:46 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Linux FS-devel Mailing List, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man,
Deepa Dinamani
In-Reply-To: <4395ae02-54fe-d5fe-dea9-c50f7eaba3b1@kernel.dk>
On Tue, Jan 29, 2019 at 5:28 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 1/29/19 9:26 AM, Arnd Bergmann wrote:
> > On Tue, Jan 29, 2019 at 5:19 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/29/19 9:18 AM, Arnd Bergmann wrote:
> >>> On Tue, Jan 29, 2019 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> That's good info. I am currently using set_user_sigmask() for it.
> >>>> I'd really like to avoid having to pass in a sigset_t size for the
> >>>> system call, however.
> >>>
> >>> I really wouldn't do it, given that all other signal handling interfaces
> >>> are prepared for longer signal masks. You /could/ probably extend
> >>> it later with a flags bit to signify a longer mask instead of using
> >>> the entire register to hold the bit length, it just seems really
> >>> inconsistent with all other system calls.
> >>
> >> Damnit! OK, I'll keep what I currently have then.
> >
> > As long as you stay within the 6-argument syscall contraints,
> > the cost of passing the length is basically free, right?
> >
> > Is there anything else you are worried about?
>
> My main worry is not the extra argument, more that we're at capacity
> for the system call. If we wanted to add a timeout parameter, then we'd
> need to bundle them up, which sucks.
Ok, got it. If it turns out that we do need the timeout argument after all,
you could avoid one indirection level by grouping the sigset_t with
min_complete, timeout_ns and/or sigset_size, as long as the
sigset_t comes last (similar to struct sigaction).
Another (also awkward) trick might be to combine min_complete and
sigset_size into a 32-bit integer argument, as each of them can
fit into 16 bits.
Arnd
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 13/18] io_uring: add file set registration
From: Jann Horn @ 2019-01-29 16:36 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <20190128213538.13486-14-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> We normally have to fget/fput for each IO we do on a file. Even with
> the batching we do, the cost of the atomic inc/dec of the file usage
> count adds up.
>
> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
> for the io_uring_register(2) system call. The arguments passed in must
> be an array of __s32 holding file descriptors, and nr_args should hold
> the number of file descriptors the application wishes to pin for the
> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
> called).
>
> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
> to the index in the array passed in to IORING_REGISTER_FILES.
>
> Files are automatically unregistered when the io_uring context is
> torn down. An application need only unregister if it wishes to
> register a few set of fds.
s/few/new/ ?
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
> fs/io_uring.c | 125 +++++++++++++++++++++++++++++-----
> include/uapi/linux/io_uring.h | 9 ++-
> 2 files changed, 116 insertions(+), 18 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 682714d6f217..77993972879b 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -98,6 +98,10 @@ struct io_ring_ctx {
> struct fasync_struct *cq_fasync;
> } ____cacheline_aligned_in_smp;
>
> + /* if used, fixed file set */
> + struct file **user_files;
> + unsigned nr_user_files;
It'd be nice if you could add a comment about locking rules here -
something like "writers must ensure that ->refs is dead, readers must
ensure that ->refs is alive as long as the file* is used".
[...]
> @@ -612,7 +625,14 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> struct kiocb *kiocb = &req->rw;
> int ret;
>
> - kiocb->ki_filp = io_file_get(state, sqe->fd);
> + if (sqe->flags & IOSQE_FIXED_FILE) {
> + if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
> + return -EBADF;
> + kiocb->ki_filp = ctx->user_files[sqe->fd];
It doesn't really matter as long as ctx->nr_user_files<=INT_MAX, but
it'd be nice if you could explicitly cast sqe->fd to unsigned here.
> + req->flags |= REQ_F_FIXED_FILE;
> + } else {
> + kiocb->ki_filp = io_file_get(state, sqe->fd);
> + }
[...]
> +static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
> + unsigned nr_args)
> +{
> + __s32 __user *fds = (__s32 __user *) arg;
> + int fd, i, ret = 0;
> +
> + if (ctx->user_files)
> + return -EBUSY;
> + if (!nr_args)
> + return -EINVAL;
> +
> + ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
> + if (!ctx->user_files)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_args; i++) {
> + ret = -EFAULT;
> + if (copy_from_user(&fd, &fds[i], sizeof(fd)))
> + break;
"i" is signed, but "nr_args" is unsigned. You can't get through that
kcalloc() call with nr_args>=0x80000000 on a normal kernel, someone
would have to set CONFIG_FORCE_MAX_ZONEORDER really high for that, but
still, in theory, if you reach this copy_to_user(..., &fds[i], ...)
with a negative "i", that'd be bad. You might want to make "i"
unsigned here and check that it's at least smaller than UINT_MAX...
> + ctx->user_files[i] = fget(fd);
> +
> + ret = -EBADF;
> + if (!ctx->user_files[i])
> + break;
> + ctx->nr_user_files++;
> + ret = 0;
> + }
> +
> + if (ret)
> + io_sqe_files_unregister(ctx);
> +
> + return ret;
> +}
> +
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 16:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, Linux FS-devel Mailing List, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man,
Deepa Dinamani
In-Reply-To: <CAK8P3a1=kho718OSdZ0zp5g3mwmsYbd5J-e58kDyq2QMBCak=Q@mail.gmail.com>
On 1/29/19 9:26 AM, Arnd Bergmann wrote:
> On Tue, Jan 29, 2019 at 5:19 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 9:18 AM, Arnd Bergmann wrote:
>>> On Tue, Jan 29, 2019 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> That's good info. I am currently using set_user_sigmask() for it.
>>>> I'd really like to avoid having to pass in a sigset_t size for the
>>>> system call, however.
>>>
>>> I really wouldn't do it, given that all other signal handling interfaces
>>> are prepared for longer signal masks. You /could/ probably extend
>>> it later with a flags bit to signify a longer mask instead of using
>>> the entire register to hold the bit length, it just seems really
>>> inconsistent with all other system calls.
>>
>> Damnit! OK, I'll keep what I currently have then.
>
> As long as you stay within the 6-argument syscall contraints,
> the cost of passing the length is basically free, right?
>
> Is there anything else you are worried about?
My main worry is not the extra argument, more that we're at capacity
for the system call. If we wanted to add a timeout parameter, then we'd
need to bundle them up, which sucks.
But I think we're fine, I'll go with what I have.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Arnd Bergmann @ 2019-01-29 16:26 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Linux FS-devel Mailing List, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man,
Deepa Dinamani
In-Reply-To: <9a66c000-62f3-7567-7919-31e61e28defa@kernel.dk>
On Tue, Jan 29, 2019 at 5:19 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/29/19 9:18 AM, Arnd Bergmann wrote:
> > On Tue, Jan 29, 2019 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> That's good info. I am currently using set_user_sigmask() for it.
> >> I'd really like to avoid having to pass in a sigset_t size for the
> >> system call, however.
> >
> > I really wouldn't do it, given that all other signal handling interfaces
> > are prepared for longer signal masks. You /could/ probably extend
> > it later with a flags bit to signify a longer mask instead of using
> > the entire register to hold the bit length, it just seems really
> > inconsistent with all other system calls.
>
> Damnit! OK, I'll keep what I currently have then.
As long as you stay within the 6-argument syscall contraints,
the cost of passing the length is basically free, right?
Is there anything else you are worried about?
Arnd
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 16:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, Linux FS-devel Mailing List, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man,
Deepa Dinamani
In-Reply-To: <CAK8P3a38-FLBH0sp0vaebv29M5oDyiUkwwmVY+9_S3vSER9n5Q@mail.gmail.com>
On 1/29/19 9:18 AM, Arnd Bergmann wrote:
> On Tue, Jan 29, 2019 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/29/19 4:58 AM, Arnd Bergmann wrote:
>>> On Tue, Jan 29, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
>>>>> On Mon, Jan 28, 2019 at 11:25:12AM -0700, Jens Axboe wrote:
>>>>>> Especially with poll support now in the series, don't we need a ѕigmask
>>>>>> argument similar to pselect/ppoll/io_pgetevents now to deal with signal
>>>>>> blocking during waiting for events?
>>>>>
>>>>> Is there any way to avoid passing in the sigset_t size? If it's just a
>>>>> 32-bit/64-bit thing, surely the in_compat_syscall() could cover it? Or
>>>>> are there other cases that need to be catered to?
>>>>
>>>> As far as I can tell we never look at it, never looked at it and don't
>>>> have any plans to look at it anytime soon. But when I tried to omit
>>>> it for io_pgetevents I got stong pushback and thus had to add the
>>>> crazy double indirection calling convention.
>>
>> That's good info. I am currently using set_user_sigmask() for it.
>> I'd really like to avoid having to pass in a sigset_t size for the
>> system call, however.
>
> I really wouldn't do it, given that all other signal handling interfaces
> are prepared for longer signal masks. You /could/ probably extend
> it later with a flags bit to signify a longer mask instead of using
> the entire register to hold the bit length, it just seems really
> inconsistent with all other system calls.
Damnit! OK, I'll keep what I currently have then.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Arnd Bergmann @ 2019-01-29 16:18 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Linux FS-devel Mailing List, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man,
Deepa Dinamani
In-Reply-To: <4b12d149-99f7-0b2e-0c3f-9b477ce48520@kernel.dk>
On Tue, Jan 29, 2019 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/29/19 4:58 AM, Arnd Bergmann wrote:
> > On Tue, Jan 29, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
> >>> On Mon, Jan 28, 2019 at 11:25:12AM -0700, Jens Axboe wrote:
> >>>> Especially with poll support now in the series, don't we need a ѕigmask
> >>>> argument similar to pselect/ppoll/io_pgetevents now to deal with signal
> >>>> blocking during waiting for events?
> >>>
> >>> Is there any way to avoid passing in the sigset_t size? If it's just a
> >>> 32-bit/64-bit thing, surely the in_compat_syscall() could cover it? Or
> >>> are there other cases that need to be catered to?
> >>
> >> As far as I can tell we never look at it, never looked at it and don't
> >> have any plans to look at it anytime soon. But when I tried to omit
> >> it for io_pgetevents I got stong pushback and thus had to add the
> >> crazy double indirection calling convention.
>
> That's good info. I am currently using set_user_sigmask() for it.
> I'd really like to avoid having to pass in a sigset_t size for the
> system call, however.
I really wouldn't do it, given that all other signal handling interfaces
are prepared for longer signal masks. You /could/ probably extend
it later with a flags bit to signify a longer mask instead of using
the entire register to hold the bit length, it just seems really
inconsistent with all other system calls.
Arnd
> What's the best way of achieving that? Can I get
> away with doing something like this:
>
> if (in_compat_syscall()) {
> const compat_sigset_t __user *compat_sig;
>
> compat_sig = (const compat_sigset_t __user *) sig;
> ret = set_compat_user_sigmask(compat_sig, &ksigmask,
> &sigsaved, _NSIG_WORDS);
> } else {
> ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
> _NSIG_WORDS);
> }
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 16:06 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez0sNBjgW7ynpNC+sir-_AuccY1sKguiD+VAXvL9-j4iag@mail.gmail.com>
On 1/29/19 8:56 AM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 4:46 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 7:21 PM, Jann Horn wrote:
>>> Please create a local copy of the request before parsing it to keep
>>> the data from changing under you. Additionally, it might make sense to
>>> annotate every pointer to shared memory with a comment, or something
>>> like that, to ensure that anyone looking at the code can immediately
>>> see for which pointers special caution is required on access.
>>
>> I took a look at the viability of NOT having to local copy the data, and
>> I don't think it's too bad. Local copy has a noticeable impact on the
>> performance, hence I'd really (REALLY) like to avoid it.
>>
>> Here's something on top of the current git branch. I think I even went a
>> bit too far in some areas, but it should hopefully catch the cases where
>> we might end up double evaluating the parts of the sqe that we depend
>> on. For most of the sqe reading we don't really care too much. For
>> instance, the sqe->user_data. If the app changes this field, then it
>> just gets whatever passed back in cqe->user_data. That's not a kernel
>> issue.
>>
>> For cases like addr/len etc validation, it should be sound. I'll double
>> check this in the morning as well, and obviously would need to be folded
>> in along the way.
>>
>> I'd appreciate your opinion on this part, if you see any major issues
>> with it, or if I missed something.
>
> The io_sqe_needs_user() checks still look racy. If that helper sees a
> IORING_OP_READ_FIXED, but then __io_submit_sqe() sees a
> IORING_OP_READV - especially if this happens in io_sq_wq_submit_work()
> -, I think you could potentially end up in places like
> io_import_iovec() without having done the set_fs(USER_DS) and
> use_mm(), causing the access to potentially occur with KERNEL_DS and a
> lazy mm.
Indeed, for that case I think we should just copy the sqe. It's in the
async offload context anyway, so a copy won't really change anything
in terms of performance. And since the gap is so large between the two
problematic spots, it'd be trickier to fix.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 15:56 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <b5902860-30a9-3dc9-b513-3d892afdc51b@kernel.dk>
On Tue, Jan 29, 2019 at 4:46 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 7:21 PM, Jann Horn wrote:
> > Please create a local copy of the request before parsing it to keep
> > the data from changing under you. Additionally, it might make sense to
> > annotate every pointer to shared memory with a comment, or something
> > like that, to ensure that anyone looking at the code can immediately
> > see for which pointers special caution is required on access.
>
> I took a look at the viability of NOT having to local copy the data, and
> I don't think it's too bad. Local copy has a noticeable impact on the
> performance, hence I'd really (REALLY) like to avoid it.
>
> Here's something on top of the current git branch. I think I even went a
> bit too far in some areas, but it should hopefully catch the cases where
> we might end up double evaluating the parts of the sqe that we depend
> on. For most of the sqe reading we don't really care too much. For
> instance, the sqe->user_data. If the app changes this field, then it
> just gets whatever passed back in cqe->user_data. That's not a kernel
> issue.
>
> For cases like addr/len etc validation, it should be sound. I'll double
> check this in the morning as well, and obviously would need to be folded
> in along the way.
>
> I'd appreciate your opinion on this part, if you see any major issues
> with it, or if I missed something.
The io_sqe_needs_user() checks still look racy. If that helper sees a
IORING_OP_READ_FIXED, but then __io_submit_sqe() sees a
IORING_OP_READV - especially if this happens in io_sq_wq_submit_work()
-, I think you could potentially end up in places like
io_import_iovec() without having done the set_fs(USER_DS) and
use_mm(), causing the access to potentially occur with KERNEL_DS and a
lazy mm.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 15:54 UTC (permalink / raw)
To: Jann Horn
Cc: Florian Weimer, linux-aio, linux-block, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez0PPWP7GU5kinzejN1B3invrcxCZT2fkpnHxMK2F0pj6A@mail.gmail.com>
On 1/29/19 8:38 AM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 2:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 1/29/19 5:12 AM, Florian Weimer wrote:
>>> * Jens Axboe:
>>>
>>>> +#define IORING_MAX_ENTRIES 4096
>>>
>>> Where does this constant come from? Should it really be exposed to
>>> userspace?
>>
>> Seems pretty handy for the application to know what the limit is?
>
> The running kernel version might be different from the kernel version
> whose headers the userspace code was compiled with. So if this is a
> limit that might change at some point in the future, and you think
> userspace wants to know the limit, some other API might be better.
Probably best to just kill it, we'll error in io_uring_register() if
it's exceeded anyway.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 15:39 UTC (permalink / raw)
To: Jann Horn; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <CAG48ez1ZM5KxjYGuDysZfNDQ_RjrrXWAM=vwyOfRi=oFdby1mQ@mail.gmail.com>
On 1/29/19 8:35 AM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>> The submission queue (SQ) and completion queue (CQ) rings are shared
>> between the application and the kernel. This eliminates the need to
>> copy data back and forth to submit and complete IO.
> [...]
>> +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
>> + const struct io_uring_sqe *sqe,
>> + struct iovec **iovec, struct iov_iter *iter)
>> +{
>> + void __user *buf = u64_to_user_ptr(sqe->addr);
>> +
>> +#ifdef CONFIG_COMPAT
>> + if (in_compat_syscall())
>> + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
>> + iovec, iter);
>> +#endif
>> +
>> + return import_iovec(rw, buf, sqe->len, UIO_FASTIOV, iovec, iter);
>> +}
>
> This code can run in kthread context, right? I think
> in_compat_syscall() might not work if this is a kthread launched by a
> compat task; I don't see anything that propagates the compat flag to
> the kthread.
Good catch, yes it can. We should just carry this information in
sqe_submit for this out-of-line purpose. I'll make that change.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 15:38 UTC (permalink / raw)
To: Jens Axboe
Cc: Florian Weimer, linux-aio, linux-block, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <74fd3127-75f1-3a48-d3cb-a8f6c9bd0b74@kernel.dk>
On Tue, Jan 29, 2019 at 2:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 1/29/19 5:12 AM, Florian Weimer wrote:
> > * Jens Axboe:
> >
> >> +#define IORING_MAX_ENTRIES 4096
> >
> > Where does this constant come from? Should it really be exposed to
> > userspace?
>
> Seems pretty handy for the application to know what the limit is?
The running kernel version might be different from the kernel version
whose headers the userspace code was compiled with. So if this is a
limit that might change at some point in the future, and you think
userspace wants to know the limit, some other API might be better.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 15:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
[...]
> +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> + const struct io_uring_sqe *sqe,
> + struct iovec **iovec, struct iov_iter *iter)
> +{
> + void __user *buf = u64_to_user_ptr(sqe->addr);
> +
> +#ifdef CONFIG_COMPAT
> + if (in_compat_syscall())
> + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
> + iovec, iter);
> +#endif
> +
> + return import_iovec(rw, buf, sqe->len, UIO_FASTIOV, iovec, iter);
> +}
This code can run in kthread context, right? I think
in_compat_syscall() might not work if this is a kthread launched by a
compat task; I don't see anything that propagates the compat flag to
the kthread.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox