LinuxLists.cc - [PATCH 0/8 v2] Non-blocking AIO

[permalink] [raw]

Subject: [PATCH 5/8] nowait aio: return on congested block device

From: Goldwyn Rodrigues <[email protected]>

A new flag BIO_NOWAIT is introduced to identify bio's
orignating from iocb with IOCB_NOWAIT. This flag indicates
to return immediately if a request cannot be made instead
of retrying.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
block/blk-core.c | 13 +++++++++++--
fs/direct-io.c | 11 +++++++++--
include/linux/blk_types.h | 1 +
3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 61ba08c..e5cfc50 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1258,6 +1258,11 @@ static struct request *get_request(struct request_queue *q, unsigned int op,
if (!IS_ERR(rq))
return rq;

+ if (bio_flagged(bio, BIO_NOWAIT)) {
+ blk_put_rl(rl);
+ return ERR_PTR(-EAGAIN);
+ }
+
if (!gfpflags_allow_blocking(gfp_mask) || unlikely(blk_queue_dying(q))) {
blk_put_rl(rl);
return rq;
@@ -2018,7 +2023,7 @@ blk_qc_t generic_make_request(struct bio *bio)
do {
struct request_queue *q = bdev_get_queue(bio->bi_bdev);

- if (likely(blk_queue_enter(q, false) == 0)) {
+ if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT)) == 0)) {
ret = q->make_request_fn(q, bio);

blk_queue_exit(q);
@@ -2027,7 +2032,11 @@ blk_qc_t generic_make_request(struct bio *bio)
} else {
struct bio *bio_next = bio_list_pop(current->bio_list);

- bio_io_error(bio);
+ if (unlikely(bio_flagged(bio, BIO_NOWAIT))) {
+ bio->bi_error = -EAGAIN;
+ bio_endio(bio);
+ } else
+ bio_io_error(bio);
bio = bio_next;
}
} while (bio);
diff --git a/fs/direct-io.c b/fs/direct-io.c
index c87bae4..2973df0 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -386,6 +386,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
else
bio->bi_end_io = dio_bio_end_io;

+ if (dio->iocb->ki_flags & IOCB_NOWAIT)
+ bio_set_flag(bio, BIO_NOWAIT);
+
sdio->bio = bio;
sdio->logical_offset_in_bio = sdio->cur_page_fs_offset;
}
@@ -480,8 +483,12 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
unsigned i;
int err;

- if (bio->bi_error)
- dio->io_error = -EIO;
+ if (bio->bi_error) {
+ if (bio_flagged(bio, BIO_NOWAIT))
+ dio->io_error = bio->bi_error;
+ else
+ dio->io_error = -EIO;
+ }

if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) {
err = bio->bi_error;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 519ea2c..1d77e9b 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -102,6 +102,7 @@ struct bio {
#define BIO_REFFED 8 /* bio has elevated ->bi_cnt */
#define BIO_THROTTLED 9 /* This bio has already been subjected to
* throttling rules. Don't do it again. */
+#define BIO_NOWAIT 10 /* don't block over blk device congestion */

/*
* Flags starting here get preserved by bio_reset() - this includes
--
2.10.2

2017-02-28 23:36:03

[permalink] [raw]

Subject: [PATCH 1/8] nowait aio: Introduce IOCB_FLAG_NOWAIT

From: Goldwyn Rodrigues <[email protected]>

This flag informs kernel to bail out if an AIO request will block
for reasons such as file allocations, or a writeback triggered,
or would block while allocating requests while performing
direct I/O.

IOCB_FLAG_NOWAIT is translated to IOCB_NOWAIT for
iocb->ki_flags.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
fs/aio.c | 3 +++
include/linux/fs.h | 1 +
include/uapi/linux/aio_abi.h | 3 +++
3 files changed, 7 insertions(+)

diff --git a/fs/aio.c b/fs/aio.c
index 873b4ca..5ae19ba 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1586,6 +1586,9 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
req->common.ki_flags |= IOCB_EVENTFD;
}

+ if (iocb->aio_flags & IOCB_FLAG_NOWAIT)
+ req->common.ki_flags |= IOCB_NOWAIT;
+
ret = put_user(KIOCB_KEY, &user_iocb->aio_key);
if (unlikely(ret)) {
pr_debug("EFAULT: aio_key\n");
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba0743..ab2f556 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -270,6 +270,7 @@ struct writeback_control;
#define IOCB_DSYNC (1 << 4)
#define IOCB_SYNC (1 << 5)
#define IOCB_WRITE (1 << 6)
+#define IOCB_NOWAIT (1 << 7)

struct kiocb {
struct file *ki_filp;
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index bb2554f..82d1d94 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -51,8 +51,11 @@ enum {
*
* IOCB_FLAG_RESFD - Set if the "aio_resfd" member of the "struct iocb"
* is valid.
+ * IOCB_FLAG_NOWAIT - Set if the user wants the iocb to fail if it would block
+ * for operations such as disk allocation.
*/
#define IOCB_FLAG_RESFD (1 << 0)
+#define IOCB_FLAG_NOWAIT (1 << 1)

/* read() from /dev/aio returns these structures. */
struct io_event {
--
2.10.2

2017-02-28 23:36:08

[permalink] [raw]

Subject: [PATCH 6/8] nowait aio: ext4

From: Goldwyn Rodrigues <[email protected]>

Return EAGAIN if any of the following checks fail for direct I/O:
+ i_rwsem is lockable
+ Writing beyond end of file (will trigger allocation)
+ Blocks are not allocated at the write location

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
fs/ext4/file.c | 53 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d663d3d..391e03b 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -124,27 +124,22 @@ ext4_unaligned_aio(struct inode *inode, struct iov_iter *from, loff_t pos)
return 0;
}

-/* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
+/* Are IO blocks allocated */
+static bool ext4_blocks_mapped(struct inode *inode, loff_t pos, loff_t len,
+ struct ext4_map_blocks *map)
{
- struct ext4_map_blocks map;
unsigned int blkbits = inode->i_blkbits;
int err, blklen;

if (pos + len > i_size_read(inode))
return false;

- map.m_lblk = pos >> blkbits;
- map.m_len = EXT4_MAX_BLOCKS(len, pos, blkbits);
- blklen = map.m_len;
+ map->m_lblk = pos >> blkbits;
+ map->m_len = EXT4_MAX_BLOCKS(len, pos, blkbits);
+ blklen = map->m_len;

- err = ext4_map_blocks(NULL, inode, &map, 0);
- /*
- * 'err==len' means that all of the blocks have been preallocated,
- * regardless of whether they have been initialized or not. To exclude
- * unwritten extents, we need to check m_flags.
- */
- return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
+ err = ext4_map_blocks(NULL, inode, map, 0);
+ return err == blklen;
}

static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from)
@@ -176,6 +171,7 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
struct inode *inode = file_inode(iocb->ki_filp);
ssize_t ret;
bool overwrite = false;
+ struct ext4_map_blocks map;

inode_lock(inode);
ret = ext4_write_checks(iocb, from);
@@ -188,7 +184,9 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (ret)
goto out;

- if (ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) {
+ if (ext4_blocks_mapped(inode, iocb->ki_pos,
+ iov_iter_count(from), &map) &&
+ (map.m_flags & EXT4_MAP_MAPPED)) {
overwrite = true;
downgrade_write(&inode->i_rwsem);
}
@@ -209,6 +207,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
{
struct inode *inode = file_inode(iocb->ki_filp);
int o_direct = iocb->ki_flags & IOCB_DIRECT;
+ int nowait = iocb->ki_flags & IOCB_NOWAIT;
int unaligned_aio = 0;
int overwrite = 0;
ssize_t ret;
@@ -218,7 +217,13 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
return ext4_dax_write_iter(iocb, from);
#endif

- inode_lock(inode);
+ if (o_direct && nowait) {
+ if (!inode_trylock(inode))
+ return -EAGAIN;
+ } else {
+ inode_lock(inode);
+ }
+
ret = ext4_write_checks(iocb, from);
if (ret <= 0)
goto out;
@@ -237,9 +242,21 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)

iocb->private = &overwrite;
/* Check whether we do a DIO overwrite or not */
- if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio &&
- ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from)))
- overwrite = 1;
+ if (o_direct && !unaligned_aio) {
+ struct ext4_map_blocks map;
+ if (ext4_blocks_mapped(inode, iocb->ki_pos,
+ iov_iter_count(from), &map)) {
+ /* To exclude unwritten extents, we need to check
+ * m_flags.
+ */
+ if (ext4_should_dioread_nolock(inode) &&
+ (map.m_flags & EXT4_MAP_MAPPED))
+ overwrite = 1;
+ } else if (iocb->ki_flags & IOCB_NOWAIT) {
+ ret = -EAGAIN;
+ goto out;
+ }
+ }

ret = __generic_file_write_iter(iocb, from);
inode_unlock(inode);
--
2.10.2

2017-02-28 23:36:04

[permalink] [raw]

Subject: [PATCH 2/8] nowait aio: Return if cannot get hold of i_rwsem

From: Goldwyn Rodrigues <[email protected]>

A failure to lock i_rwsem would mean there is I/O being performed
by another thread. So, let's bail.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
mm/filemap.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 3f9afde..78dd50e 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2973,7 +2973,12 @@ ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
struct inode *inode = file->f_mapping->host;
ssize_t ret;

- inode_lock(inode);
+ if (!inode_trylock(inode)) {
+ /* Don't sleep on inode rwsem */
+ if (iocb->ki_flags & IOCB_NOWAIT)
+ return -EAGAIN;
+ inode_lock(inode);
+ }
ret = generic_write_checks(iocb, from);
if (ret > 0)
ret = __generic_file_write_iter(iocb, from);
--
2.10.2

2017-02-28 23:36:10

[permalink] [raw]

Subject: [PATCH 8/8] nowait aio: btrfs

From: Goldwyn Rodrigues <[email protected]>

Return EAGAIN if any of the following checks fail
+ i_rwsem is not lockable
+ NODATACOW or PREALLOC is not set
+ Cannot nocow at the desired location
+ Writing beyond end of file which is not allocated

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
fs/btrfs/file.c | 25 ++++++++++++++++++++-----
fs/btrfs/inode.c | 3 +++
2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index b5c5da2..8640280 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1819,12 +1819,29 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
ssize_t num_written = 0;
bool sync = (file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host);
ssize_t err;
- loff_t pos;
- size_t count;
+ loff_t pos = iocb->ki_pos;
+ size_t count = iov_iter_count(from);
loff_t oldsize;
int clean_page = 0;

- inode_lock(inode);
+ if ((iocb->ki_flags & IOCB_NOWAIT) &&
+ (iocb->ki_flags & IOCB_DIRECT)) {
+ /* Don't sleep on inode rwsem */
+ if (!inode_trylock(inode))
+ return -EAGAIN;
+ /*
+ * We will allocate space in case nodatacow is not set,
+ * so bail
+ */
+ if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
+ BTRFS_INODE_PREALLOC)) ||
+ check_can_nocow(inode, pos, &count) <= 0) {
+ inode_unlock(inode);
+ return -EAGAIN;
+ }
+ } else
+ inode_lock(inode);
+
err = generic_write_checks(iocb, from);
if (err <= 0) {
inode_unlock(inode);
@@ -1858,8 +1875,6 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
*/
update_time_for_write(inode);

- pos = iocb->ki_pos;
- count = iov_iter_count(from);
start_pos = round_down(pos, fs_info->sectorsize);
oldsize = i_size_read(inode);
if (start_pos > oldsize) {
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1e861a0..c5041ea 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8681,6 +8681,9 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
if (offset + count <= inode->i_size) {
inode_unlock(inode);
relock = true;
+ } else if (iocb->ki_flags & IOCB_NOWAIT) {
+ ret = -EAGAIN;
+ goto out;
}
ret = btrfs_delalloc_reserve_space(inode, offset, count);
if (ret)
--
2.10.2

2017-02-28 23:36:05

[permalink] [raw]

Subject: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

From: Goldwyn Rodrigues <[email protected]>

Find out if the write will trigger a wait due to writeback. If yes,
return -EAGAIN.

This introduces a new function filemap_range_has_page() which
returns true if the file's mapping has a page within the range
mentioned.

Return -EINVAL for buffered AIO: there are multiple causes of
delay such as page locks, dirty throttling logic, page loading
from disk etc. which cannot be taken care of.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
include/linux/fs.h | 2 ++
mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index ab2f556..527ef53 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2494,6 +2494,8 @@ extern int filemap_fdatawait(struct address_space *);
extern void filemap_fdatawait_keep_errors(struct address_space *);
extern int filemap_fdatawait_range(struct address_space *, loff_t lstart,
loff_t lend);
+extern int filemap_range_has_page(struct address_space *, loff_t lstart,
+ loff_t lend);
extern int filemap_write_and_wait(struct address_space *mapping);
extern int filemap_write_and_wait_range(struct address_space *mapping,
loff_t lstart, loff_t lend);
diff --git a/mm/filemap.c b/mm/filemap.c
index 78dd50e..82335f4 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -375,6 +375,39 @@ int filemap_flush(struct address_space *mapping)
}
EXPORT_SYMBOL(filemap_flush);

+/**
+ * filemap_range_has_page - check if a page exists in range.
+ * @mapping: address space structure to wait for
+ * @start_byte: offset in bytes where the range starts
+ * @end_byte: offset in bytes where the range ends (inclusive)
+ *
+ * Find at least one page in the range supplied, usually used to check if
+ * direct writing in this range will trigger a writeback.
+ */
+int filemap_range_has_page(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte)
+{
+ pgoff_t index = start_byte >> PAGE_SHIFT;
+ pgoff_t end = end_byte >> PAGE_SHIFT;
+ struct pagevec pvec;
+ int ret;
+
+ if (end_byte < start_byte)
+ return 0;
+
+ if (mapping->nrpages == 0)
+ return 0;
+
+ pagevec_init(&pvec, 0);
+ ret = pagevec_lookup(&pvec, mapping, index, 1);
+ if (!ret)
+ return 0;
+ ret = (pvec.pages[0]->index <= end);
+ pagevec_release(&pvec);
+ return ret;
+}
+EXPORT_SYMBOL(filemap_range_has_page);
+
static int __filemap_fdatawait_range(struct address_space *mapping,
loff_t start_byte, loff_t end_byte)
{
@@ -2631,6 +2664,9 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)

pos = iocb->ki_pos;

+ if ((iocb->ki_flags & IOCB_NOWAIT) && !(iocb->ki_flags & IOCB_DIRECT))
+ return -EINVAL;
+
if (limit != RLIM_INFINITY) {
if (iocb->ki_pos >= limit) {
send_sig(SIGXFSZ, current, 0);
@@ -2700,9 +2736,17 @@ generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
write_len = iov_iter_count(from);
end = (pos + write_len - 1) >> PAGE_SHIFT;

- written = filemap_write_and_wait_range(mapping, pos, pos + write_len - 1);
- if (written)
- goto out;
+ if (iocb->ki_flags & IOCB_NOWAIT) {
+ /* If there are pages to writeback, return */
+ if (filemap_range_has_page(inode->i_mapping, pos,
+ pos + iov_iter_count(from)))
+ return -EAGAIN;
+ } else {
+ written = filemap_write_and_wait_range(mapping, pos,
+ pos + write_len - 1);
+ if (written)
+ goto out;
+ }

/*
* After a write we want buffered reads to be sure to go to disk to get
--
2.10.2

2017-02-28 23:36:06

[permalink] [raw]

Subject: [PATCH 4/8] nowait aio: Introduce IOMAP_NOWAIT

From: Goldwyn Rodrigues <[email protected]>

IOCB_NOWAIT translates to IOMAP_NOWAIT for iomaps.
This is used by XFS in the XFS patch.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
fs/iomap.c | 2 ++
include/linux/iomap.h | 1 +
2 files changed, 3 insertions(+)

diff --git a/fs/iomap.c b/fs/iomap.c
index a51cb4c..3fb68d2 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -883,6 +883,8 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, struct iomap_ops *ops,
} else {
dio->flags |= IOMAP_DIO_WRITE;
flags |= IOMAP_WRITE;
+ if (iocb->ki_flags & IOCB_NOWAIT)
+ flags |= IOMAP_NOWAIT;
}

if (mapping->nrpages) {
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index a4c94b8..d1c33ef 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -51,6 +51,7 @@ struct iomap {
#define IOMAP_REPORT (1 << 2) /* report extent status, e.g. FIEMAP */
#define IOMAP_FAULT (1 << 3) /* mapping for page fault */
#define IOMAP_DIRECT (1 << 4) /* direct I/O */
+#define IOMAP_NOWAIT (1 << 5) /* Don't wait for writeback */

struct iomap_ops {
/*
--
2.10.2

2017-02-28 23:36:09

[permalink] [raw]

Subject: [PATCH 7/8] nowait aio: xfs

From: Goldwyn Rodrigues <[email protected]>

If IOCB_NOWAIT is set, bail if the i_rwsem is not lockable
immediately.

IF IOMAP_NOWAIT is set, return EAGAIN in xfs_file_iomap_begin
if it needs allocation either due to file extending, writing to a hole,
or COW.

Signed-off-by: Goldwyn Rodrigues <[email protected]>
---
fs/xfs/xfs_file.c | 9 +++++++--
fs/xfs/xfs_iomap.c | 9 +++++++++
2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index bbb9eb6..7e16a83 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -528,12 +528,17 @@ xfs_file_dio_aio_write(
((iocb->ki_pos + count) & mp->m_blockmask)) {
unaligned_io = 1;
iolock = XFS_IOLOCK_EXCL;
+ if (iocb->ki_flags & IOCB_NOWAIT)
+ return -EAGAIN;
} else {
iolock = XFS_IOLOCK_SHARED;
}

- xfs_ilock(ip, iolock);
-
+ if (!xfs_ilock_nowait(ip, iolock)) {
+ if (iocb->ki_flags & IOCB_NOWAIT)
+ return -EAGAIN;
+ xfs_ilock(ip, iolock);
+ }
ret = xfs_file_aio_write_checks(iocb, from, &iolock);
if (ret)
goto out;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 1aa3abd..84f981a 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1020,6 +1020,11 @@ xfs_file_iomap_begin(
if ((flags & IOMAP_REPORT) ||
(xfs_is_reflink_inode(ip) &&
(flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT))) {
+ /* Allocations due to reflinks */
+ if ((flags & IOMAP_NOWAIT) && !(flags & IOMAP_REPORT)) {
+ error = -EAGAIN;
+ goto out_unlock;
+ }
/* Trim the mapping to the nearest shared extent boundary. */
error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
&trimmed);
@@ -1049,6 +1054,10 @@ xfs_file_iomap_begin(
}

if ((flags & IOMAP_WRITE) && imap_needs_alloc(inode, &imap, nimaps)) {
+ if (flags & IOMAP_NOWAIT) {
+ error = -EAGAIN;
+ goto out_unlock;
+ }
/*
* We cap the maximum length we map here to MAX_WRITEBACK_PAGES
* pages to keep the chunks of work done where somewhat symmetric
--
2.10.2

2017-03-01 03:46:44

by Matthew Wilcox

[permalink] [raw]

Subject: Re: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

On Tue, Feb 28, 2017 at 05:36:05PM -0600, Goldwyn Rodrigues wrote:
> Find out if the write will trigger a wait due to writeback. If yes,
> return -EAGAIN.
>
> This introduces a new function filemap_range_has_page() which
> returns true if the file's mapping has a page within the range
> mentioned.

Ugh, this is pretty inefficient. If that's all you want to know, then
using the radix tree directly will be far more efficient than spinning
up all the pagevec machinery only to discard the pages found.

But what's going to kick these pages out of cache? Shouldn't we rather
find the pages, kick them out if clean, start writeback if not, and *then*
return -EAGAIN?

So maybe we want to spin up the pagevec machinery after all so we can
do that extra work?

2017-03-01 15:36:48

[permalink] [raw]

Subject: Re: [PATCH 1/8] nowait aio: Introduce IOCB_FLAG_NOWAIT

On Tue, Feb 28, 2017 at 05:36:03PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <[email protected]>
>
> This flag informs kernel to bail out if an AIO request will block
> for reasons such as file allocations, or a writeback triggered,
> or would block while allocating requests while performing
> direct I/O.
>
> IOCB_FLAG_NOWAIT is translated to IOCB_NOWAIT for
> iocb->ki_flags.

Given that we aren't validating aio_flags in older kernels we can't
just add this flag as it will be a no-op in older kernels. I think
we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that
properly validate all reserved fields or flags first.

Once we do that I'd really prefer to use the same flags values
as preadv2/pwritev2 so that we'll only need one set of flags over
sync/async read/write ops.

2017-03-01 15:37:07

[permalink] [raw]

Subject: Re: [PATCH 2/8] nowait aio: Return if cannot get hold of i_rwsem

Looks fine,

Reviewed-by: Christoph Hellwig <[email protected]>

2017-03-01 15:38:57

[permalink] [raw]

Subject: Re: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

On Tue, Feb 28, 2017 at 07:46:06PM -0800, Matthew Wilcox wrote:
> Ugh, this is pretty inefficient. If that's all you want to know, then
> using the radix tree directly will be far more efficient than spinning
> up all the pagevec machinery only to discard the pages found.
>
> But what's going to kick these pages out of cache? Shouldn't we rather
> find the pages, kick them out if clean, start writeback if not, and *then*
> return -EAGAIN?
>
> So maybe we want to spin up the pagevec machinery after all so we can
> do that extra work?

As pointed out in the last round of these patches I think we really
need to pass a flags argument to filemap_write_and_wait_range to
communicate the non-blocking nature and only return -EAGAIN if we'd
block. As a bonus that can indeed start to kick the pages out.

2017-03-01 15:40:58

[permalink] [raw]

Subject: Re: [PATCH 7/8] nowait aio: xfs

> @@ -528,12 +528,17 @@ xfs_file_dio_aio_write(
> ((iocb->ki_pos + count) & mp->m_blockmask)) {
> unaligned_io = 1;
> iolock = XFS_IOLOCK_EXCL;
> + if (iocb->ki_flags & IOCB_NOWAIT)
> + return -EAGAIN;

So all unaligned I/O will return -EAGAIN? Why? Also please explain
that reason in a comment right here.

> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 1aa3abd..84f981a 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1020,6 +1020,11 @@ xfs_file_iomap_begin(
> if ((flags & IOMAP_REPORT) ||
> (xfs_is_reflink_inode(ip) &&
> (flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT))) {
> + /* Allocations due to reflinks */
> + if ((flags & IOMAP_NOWAIT) && !(flags & IOMAP_REPORT)) {
> + error = -EAGAIN;
> + goto out_unlock;
> + }

FYI, this code looks very different in current Linus' tree - I think
you're on some old kernel base.

2017-03-01 15:56:52

[permalink] [raw]

Subject: Re: [PATCH 1/8] nowait aio: Introduce IOCB_FLAG_NOWAIT

On Wed, Mar 01, 2017 at 07:36:48AM -0800, Christoph Hellwig wrote:
> Given that we aren't validating aio_flags in older kernels we can't
> just add this flag as it will be a no-op in older kernels. I think
> we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that
> properly validate all reserved fields or flags first.
>
> Once we do that I'd really prefer to use the same flags values
> as preadv2/pwritev2 so that we'll only need one set of flags over
> sync/async read/write ops.

I just took another look and we do verify that
aio_reserved1/aio_reserved2 must be zero. So I think we can just
stick RWF_* into aio_reserved1 and fix that problem that way.

2017-03-01 16:57:17

[permalink] [raw]

Subject: Re: [PATCH 1/8] nowait aio: Introduce IOCB_FLAG_NOWAIT

On 03/01/2017 09:56 AM, Christoph Hellwig wrote:
> On Wed, Mar 01, 2017 at 07:36:48AM -0800, Christoph Hellwig wrote:
>> Given that we aren't validating aio_flags in older kernels we can't
>> just add this flag as it will be a no-op in older kernels. I think
>> we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that
>> properly validate all reserved fields or flags first.
>>
>> Once we do that I'd really prefer to use the same flags values
>> as preadv2/pwritev2 so that we'll only need one set of flags over
>> sync/async read/write ops.
>
> I just took another look and we do verify that
> aio_reserved1/aio_reserved2 must be zero. So I think we can just
> stick RWF_* into aio_reserved1 and fix that problem that way.
>

RWF_* ? Isn't that kernel space flags? Or did you intend to say
IOCB_FLAG_*? If yes, we maintain two flag fields? aio_reserved1 (perhaps
renamed to aio_flags2) and aio_flags?

aio_reserved1 is also used to return key for the purpose of io_cancel,
but we should be able to fetch the flags before putting the key value
there. Still I am not comfortable using the same field for it because it
will be overwritten when io_submit returns.

Which brings me to the next question: What is the purpose of aio_key?
Why is aio_key set to KIOCB_KEY (which is zero) every time? You are not
differentiating the request by setting all the iocb's key to zero.

--
Goldwyn

2017-03-01 22:44:21

[permalink] [raw]

Subject: Re: [PATCH 1/8] nowait aio: Introduce IOCB_FLAG_NOWAIT

On Wed, Mar 01, 2017 at 10:57:17AM -0600, Goldwyn Rodrigues wrote:
> RWF_* ? Isn't that kernel space flags? Or did you intend to say
> IOCB_FLAG_*?

No, they are the flags for preadv2/pwritev2.

> If yes, we maintain two flag fields? aio_reserved1 (perhaps
> renamed to aio_flags2) and aio_flags?

Yes - I'd call it aio_rw_flags or similar.

> aio_reserved1 is also used to return key for the purpose of io_cancel,
> but we should be able to fetch the flags before putting the key value
> there. Still I am not comfortable using the same field for it because it
> will be overwritten when io_submit returns.

It's not - the key is a separate field. It's just that the two are
defined using a very strange macro switching around their positions
based on the endiannes.

> Which brings me to the next question: What is the purpose of aio_key?
> Why is aio_key set to KIOCB_KEY (which is zero) every time? You are not
> differentiating the request by setting all the iocb's key to zero.

I don't know the history of this rather odd field.

2017-03-02 10:38:45

[permalink] [raw]

Subject: Re: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

On Wed 01-03-17 07:38:57, Christoph Hellwig wrote:
> On Tue, Feb 28, 2017 at 07:46:06PM -0800, Matthew Wilcox wrote:
> > Ugh, this is pretty inefficient. If that's all you want to know, then
> > using the radix tree directly will be far more efficient than spinning
> > up all the pagevec machinery only to discard the pages found.
> >
> > But what's going to kick these pages out of cache? Shouldn't we rather
> > find the pages, kick them out if clean, start writeback if not, and *then*
> > return -EAGAIN?
> >
> > So maybe we want to spin up the pagevec machinery after all so we can
> > do that extra work?
>
> As pointed out in the last round of these patches I think we really
> need to pass a flags argument to filemap_write_and_wait_range to
> communicate the non-blocking nature and only return -EAGAIN if we'd
> block. As a bonus that can indeed start to kick the pages out.

Aren't flags to filemap_write_and_wait_range() unnecessary complication?
Realistically, most users wanting performance from AIO DIO so badly that
they bother with this API won't have any pages to write / evict. If they do
by some bad accident, they can fall back to standard "blocking" AIO DIO.
So I don't see much value in teaching filemap_write_and_wait_range() about
a non-blocking mode...

Honza

--
Jan Kara <[email protected]>
SUSE Labs, CR

2017-03-02 14:12:45

by Matthew Wilcox

[permalink] [raw]

Subject: Re: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

On Thu, Mar 02, 2017 at 11:38:45AM +0100, Jan Kara wrote:
> On Wed 01-03-17 07:38:57, Christoph Hellwig wrote:
> > On Tue, Feb 28, 2017 at 07:46:06PM -0800, Matthew Wilcox wrote:
> > > But what's going to kick these pages out of cache? Shouldn't we rather
> > > find the pages, kick them out if clean, start writeback if not, and *then*
> > > return -EAGAIN?
> >
> > As pointed out in the last round of these patches I think we really
> > need to pass a flags argument to filemap_write_and_wait_range to
> > communicate the non-blocking nature and only return -EAGAIN if we'd
> > block. As a bonus that can indeed start to kick the pages out.
>
> Aren't flags to filemap_write_and_wait_range() unnecessary complication?
> Realistically, most users wanting performance from AIO DIO so badly that
> they bother with this API won't have any pages to write / evict. If they do
> by some bad accident, they can fall back to standard "blocking" AIO DIO.
> So I don't see much value in teaching filemap_write_and_wait_range() about
> a non-blocking mode...

That lets me execute a DoS against a user using this API. All I have
to do is open the file they're using read-only and read a byte from it.
Page goes into page-cache, and they'll only get -EAGAIN from calling
this syscall until the page ages out.

Also, I don't understand why this is a flag. Isn't the point of AIO to
be non-blocking? Why isn't this just a change to how we do AIO?

2017-03-02 15:46:39

[permalink] [raw]

Subject: Re: [PATCH 3/8] nowait aio: return if direct write will trigger writeback

On Thu 02-03-17 06:12:45, Matthew Wilcox wrote:
> On Thu, Mar 02, 2017 at 11:38:45AM +0100, Jan Kara wrote:
> > On Wed 01-03-17 07:38:57, Christoph Hellwig wrote:
> > > On Tue, Feb 28, 2017 at 07:46:06PM -0800, Matthew Wilcox wrote:
> > > > But what's going to kick these pages out of cache? Shouldn't we rather
> > > > find the pages, kick them out if clean, start writeback if not, and *then*
> > > > return -EAGAIN?
> > >
> > > As pointed out in the last round of these patches I think we really
> > > need to pass a flags argument to filemap_write_and_wait_range to
> > > communicate the non-blocking nature and only return -EAGAIN if we'd
> > > block. As a bonus that can indeed start to kick the pages out.
> >
> > Aren't flags to filemap_write_and_wait_range() unnecessary complication?
> > Realistically, most users wanting performance from AIO DIO so badly that
> > they bother with this API won't have any pages to write / evict. If they do
> > by some bad accident, they can fall back to standard "blocking" AIO DIO.
> > So I don't see much value in teaching filemap_write_and_wait_range() about
> > a non-blocking mode...
>
> That lets me execute a DoS against a user using this API. All I have
> to do is open the file they're using read-only and read a byte from it.
> Page goes into page-cache, and they'll only get -EAGAIN from calling
> this syscall until the page ages out.

It will not be a DoS. This non-blocking AIO can always return EAGAIN when
it feels like it and the caller is required to fall back to a blocking
version in that case if he wants to guarantee forward progress. It is just
a performance optimization which allows user (database) to submit IO from a
computation thread instead of having to offload it to an IO thread...

> Also, I don't understand why this is a flag. Isn't the point of AIO to
> be non-blocking? Why isn't this just a change to how we do AIO?

Because this is an API change and the caller has to implement some handling
to guarantee a forward progress of non-blocking IO...

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2017-03-05 14:56:21

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/01/2017 01:36 AM, Goldwyn Rodrigues wrote:
> This series adds nonblocking feature to asynchronous I/O writes.
> io_submit() can be delayed because of a number of reason:
> - Block allocation for files
> - Data writebacks for direct I/O
> - Sleeping because of waiting to acquire i_rwsem
> - Congested block device

We've been hit by a few of these so this change is very welcome.

>
> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
> any of these conditions are met. This way userspace can push most
> of the write()s to the kernel to the best of its ability to complete
> and if it returns -EAGAIN, can defer it to another thread.
>

Is it not possible to push the iocb to a workqueue? This will allow
existing userspace to work with the new functionality, unchanged. Any
userspace implementation would have to do the same thing, so it's not
like we're saving anything by pushing it there.

> In order to enable this, IOCB_FLAG_NOWAIT is introduced in
> uapi/linux/aio_abi.h which translates to IOCB_NOWAIT for struct iocb,
> BIO_NOWAIT for bio and IOMAP_NOWAIT for iomap.
>
> This feature is provided for direct I/O of asynchronous I/O only. I have
> tested it against xfs, ext4, and btrfs.
>
> Changes since v1:
> + Forwardported from 4.9.10
> + changed name from _NONBLOCKING to *_NOWAIT
> + filemap_range_has_page call moved to closer to (just before) calling filemap_write_and_wait_range().
> + BIO_NOWAIT limited to get_request()
> + XFS fixes
> - included reflink
> - use of xfs_ilock_nowait() instead of a XFS_IOLOCK_NONBLOCKING flag
> - Translate the flag through IOMAP_NOWAIT (iomap) to check for
> block allocation for the file.
> + ext4 coding style

2017-03-06 08:40:36

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 10:25 AM, Jan Kara wrote:
> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>> any of these conditions are met. This way userspace can push most
>>> of the write()s to the kernel to the best of its ability to complete
>>> and if it returns -EAGAIN, can defer it to another thread.
>>>
>> Is it not possible to push the iocb to a workqueue? This will allow
>> existing userspace to work with the new functionality, unchanged. Any
>> userspace implementation would have to do the same thing, so it's not like
>> we're saving anything by pushing it there.
> That is not easy because until IO is fully submitted, you need some parts
> of the context of the process which submits the IO (e.g. memory mappings,
> but possibly also other credentials). So you would need to somehow transfer
> this information to the workqueue.
>

It's at least possible to pass the mm_struct to the workqueue, and I
imagine other process attributes. But I appreciate the difficulty.

It would be quite annoying to have to keep a large number of worker
threads active, just in case aio is not working. Modern NVMes have
fairly deep queues, and at the worst case, you'll need one thread for
each I/O to keep everything busy.

2017-03-06 08:25:46

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On Sun 05-03-17 16:56:21, Avi Kivity wrote:
> >The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
> >any of these conditions are met. This way userspace can push most
> >of the write()s to the kernel to the best of its ability to complete
> >and if it returns -EAGAIN, can defer it to another thread.
> >
>
> Is it not possible to push the iocb to a workqueue? This will allow
> existing userspace to work with the new functionality, unchanged. Any
> userspace implementation would have to do the same thing, so it's not like
> we're saving anything by pushing it there.

That is not easy because until IO is fully submitted, you need some parts
of the context of the process which submits the IO (e.g. memory mappings,
but possibly also other credentials). So you would need to somehow transfer
this information to the workqueue.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2017-03-06 15:19:14

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 01:25 AM, Jan Kara wrote:
> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>> any of these conditions are met. This way userspace can push most
>>> of the write()s to the kernel to the best of its ability to complete
>>> and if it returns -EAGAIN, can defer it to another thread.
>>>
>>
>> Is it not possible to push the iocb to a workqueue? This will allow
>> existing userspace to work with the new functionality, unchanged. Any
>> userspace implementation would have to do the same thing, so it's not like
>> we're saving anything by pushing it there.
>
> That is not easy because until IO is fully submitted, you need some parts
> of the context of the process which submits the IO (e.g. memory mappings,
> but possibly also other credentials). So you would need to somehow transfer
> this information to the workqueue.

Outside of technical challenges, the API also needs to return EAGAIN or
start blocking at some point. We can't expose a direct connection to
queue work like that, and let any user potentially create millions of
pending work items (and IOs). That's why the current API is safe, even
though it does suck that it block seemingly randomly for users.

--
Jens Axboe

2017-03-06 15:29:41

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 05:19 PM, Jens Axboe wrote:
> On 03/06/2017 01:25 AM, Jan Kara wrote:
>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>> any of these conditions are met. This way userspace can push most
>>>> of the write()s to the kernel to the best of its ability to complete
>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>
>>> Is it not possible to push the iocb to a workqueue? This will allow
>>> existing userspace to work with the new functionality, unchanged. Any
>>> userspace implementation would have to do the same thing, so it's not like
>>> we're saving anything by pushing it there.
>> That is not easy because until IO is fully submitted, you need some parts
>> of the context of the process which submits the IO (e.g. memory mappings,
>> but possibly also other credentials). So you would need to somehow transfer
>> this information to the workqueue.
> Outside of technical challenges, the API also needs to return EAGAIN or
> start blocking at some point. We can't expose a direct connection to
> queue work like that, and let any user potentially create millions of
> pending work items (and IOs).

You wouldn't expect more concurrent events than the maxevents parameter
that was supplied to io_setup syscall; it should have reserved any
resources needed.

> That's why the current API is safe, even
> though it does suck that it block seemingly randomly for users.

2017-03-06 15:38:18

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 08:29 AM, Avi Kivity wrote:
>
>
> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>> any of these conditions are met. This way userspace can push most
>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>
>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>> existing userspace to work with the new functionality, unchanged. Any
>>>> userspace implementation would have to do the same thing, so it's not like
>>>> we're saving anything by pushing it there.
>>> That is not easy because until IO is fully submitted, you need some parts
>>> of the context of the process which submits the IO (e.g. memory mappings,
>>> but possibly also other credentials). So you would need to somehow transfer
>>> this information to the workqueue.
>> Outside of technical challenges, the API also needs to return EAGAIN or
>> start blocking at some point. We can't expose a direct connection to
>> queue work like that, and let any user potentially create millions of
>> pending work items (and IOs).
>
> You wouldn't expect more concurrent events than the maxevents parameter
> that was supplied to io_setup syscall; it should have reserved any
> resources needed.

Doesn't matter what limit you apply, my point still stands - at some
point you have to return EAGAIN, or block. Returning EAGAIN without
the caller having flagged support for that change of behavior would
be problematic.

And for this to really work, aio would need some serious help in
how it applies limits. It looks like a hot mess.

--
Jens Axboe

2017-03-06 15:59:26

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 05:38 PM, Jens Axboe wrote:
> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>
>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>> any of these conditions are met. This way userspace can push most
>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>
>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>> we're saving anything by pushing it there.
>>>> That is not easy because until IO is fully submitted, you need some parts
>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>> but possibly also other credentials). So you would need to somehow transfer
>>>> this information to the workqueue.
>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>> start blocking at some point. We can't expose a direct connection to
>>> queue work like that, and let any user potentially create millions of
>>> pending work items (and IOs).
>> You wouldn't expect more concurrent events than the maxevents parameter
>> that was supplied to io_setup syscall; it should have reserved any
>> resources needed.
> Doesn't matter what limit you apply, my point still stands - at some
> point you have to return EAGAIN, or block. Returning EAGAIN without
> the caller having flagged support for that change of behavior would
> be problematic.

Doesn't it already return EAGAIN (or some other error) if you exceed
maxevents?

> And for this to really work, aio would need some serious help in
> how it applies limits. It looks like a hot mess.

For sure. I think it would be a shame to create more user-facing
complexity.

2017-03-06 16:08:57

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 08:59 AM, Avi Kivity wrote:
> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>
>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>
>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>> we're saving anything by pushing it there.
>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>> this information to the workqueue.
>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>> start blocking at some point. We can't expose a direct connection to
>>>> queue work like that, and let any user potentially create millions of
>>>> pending work items (and IOs).
>>> You wouldn't expect more concurrent events than the maxevents parameter
>>> that was supplied to io_setup syscall; it should have reserved any
>>> resources needed.
>> Doesn't matter what limit you apply, my point still stands - at some
>> point you have to return EAGAIN, or block. Returning EAGAIN without
>> the caller having flagged support for that change of behavior would
>> be problematic.
>
> Doesn't it already return EAGAIN (or some other error) if you exceed
> maxevents?

It's a setup thing. We check these limits when someone creates an IO
context, and carve out the specified entries form our global pool. Then
we free those "resources" when the io context is freed.

Right now I can setup an IO context with 1000 entries on it, yet that
number has NO bearing on when io_submit() would potentially block or
return EAGAIN.

We can have a huge gap on the intent signaled by io context setup, and
the reality imposed by what actually happens on the IO submission side.

--
Jens Axboe

2017-03-06 17:06:43

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 09:59 AM, Avi Kivity wrote:
>
>
> On 03/06/2017 06:08 PM, Jens Axboe wrote:
>> On 03/06/2017 08:59 AM, Avi Kivity wrote:
>>> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>>>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>>>
>>>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>>>> we're saving anything by pushing it there.
>>>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>>>> this information to the workqueue.
>>>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>>>> start blocking at some point. We can't expose a direct connection to
>>>>>> queue work like that, and let any user potentially create millions of
>>>>>> pending work items (and IOs).
>>>>> You wouldn't expect more concurrent events than the maxevents parameter
>>>>> that was supplied to io_setup syscall; it should have reserved any
>>>>> resources needed.
>>>> Doesn't matter what limit you apply, my point still stands - at some
>>>> point you have to return EAGAIN, or block. Returning EAGAIN without
>>>> the caller having flagged support for that change of behavior would
>>>> be problematic.
>>> Doesn't it already return EAGAIN (or some other error) if you exceed
>>> maxevents?
>> It's a setup thing. We check these limits when someone creates an IO
>> context, and carve out the specified entries form our global pool. Then
>> we free those "resources" when the io context is freed.
>>
>> Right now I can setup an IO context with 1000 entries on it, yet that
>> number has NO bearing on when io_submit() would potentially block or
>> return EAGAIN.
>>
>> We can have a huge gap on the intent signaled by io context setup, and
>> the reality imposed by what actually happens on the IO submission side.
>
> Isn't that a bug? Shouldn't that 1001st incomplete io_submit() return
> EAGAIN?
>
> Just tested it, and maxevents is not respected for this:
>
> io_setup(1, [0x7fc64537f000]) = 0
> io_submit(0x7fc64537f000, 10, [{pread, fildes=3, buf=0x1eb4000,
> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread,
> fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3,
> buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000,
> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}]) = 10
>
> which is unexpected, to me.

ioctx_alloc()
{
[...]

/*
* We keep track of the number of available ringbuffer slots, to prevent
* overflow (reqs_available), and we also use percpu counters for this.
*
* So since up to half the slots might be on other cpu's percpu counters
* and unavailable, double nr_events so userspace sees what they
* expected: additionally, we move req_batch slots to/from percpu
* counters at a time, so make sure that isn't 0:
*/
nr_events = max(nr_events, num_possible_cpus() * 4);
nr_events *= 2;
}

--
Jens Axboe

2017-03-06 16:59:48

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 06:08 PM, Jens Axboe wrote:
> On 03/06/2017 08:59 AM, Avi Kivity wrote:
>> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>>
>>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>>> we're saving anything by pushing it there.
>>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>>> this information to the workqueue.
>>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>>> start blocking at some point. We can't expose a direct connection to
>>>>> queue work like that, and let any user potentially create millions of
>>>>> pending work items (and IOs).
>>>> You wouldn't expect more concurrent events than the maxevents parameter
>>>> that was supplied to io_setup syscall; it should have reserved any
>>>> resources needed.
>>> Doesn't matter what limit you apply, my point still stands - at some
>>> point you have to return EAGAIN, or block. Returning EAGAIN without
>>> the caller having flagged support for that change of behavior would
>>> be problematic.
>> Doesn't it already return EAGAIN (or some other error) if you exceed
>> maxevents?
> It's a setup thing. We check these limits when someone creates an IO
> context, and carve out the specified entries form our global pool. Then
> we free those "resources" when the io context is freed.
>
> Right now I can setup an IO context with 1000 entries on it, yet that
> number has NO bearing on when io_submit() would potentially block or
> return EAGAIN.
>
> We can have a huge gap on the intent signaled by io context setup, and
> the reality imposed by what actually happens on the IO submission side.

Isn't that a bug? Shouldn't that 1001st incomplete io_submit() return
EAGAIN?

Just tested it, and maxevents is not respected for this:

io_setup(1, [0x7fc64537f000]) = 0
io_submit(0x7fc64537f000, 10, [{pread, fildes=3, buf=0x1eb4000,
nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
{pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread,
fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3,
buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000,
nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
{pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}]) = 10

which is unexpected, to me.

2017-03-06 18:17:43

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 07:06 PM, Jens Axboe wrote:
> On 03/06/2017 09:59 AM, Avi Kivity wrote:
>>
>> On 03/06/2017 06:08 PM, Jens Axboe wrote:
>>> On 03/06/2017 08:59 AM, Avi Kivity wrote:
>>>> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>>>>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>>>>
>>>>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>>>>> we're saving anything by pushing it there.
>>>>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>>>>> this information to the workqueue.
>>>>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>>>>> start blocking at some point. We can't expose a direct connection to
>>>>>>> queue work like that, and let any user potentially create millions of
>>>>>>> pending work items (and IOs).
>>>>>> You wouldn't expect more concurrent events than the maxevents parameter
>>>>>> that was supplied to io_setup syscall; it should have reserved any
>>>>>> resources needed.
>>>>> Doesn't matter what limit you apply, my point still stands - at some
>>>>> point you have to return EAGAIN, or block. Returning EAGAIN without
>>>>> the caller having flagged support for that change of behavior would
>>>>> be problematic.
>>>> Doesn't it already return EAGAIN (or some other error) if you exceed
>>>> maxevents?
>>> It's a setup thing. We check these limits when someone creates an IO
>>> context, and carve out the specified entries form our global pool. Then
>>> we free those "resources" when the io context is freed.
>>>
>>> Right now I can setup an IO context with 1000 entries on it, yet that
>>> number has NO bearing on when io_submit() would potentially block or
>>> return EAGAIN.
>>>
>>> We can have a huge gap on the intent signaled by io context setup, and
>>> the reality imposed by what actually happens on the IO submission side.
>> Isn't that a bug? Shouldn't that 1001st incomplete io_submit() return
>> EAGAIN?
>>
>> Just tested it, and maxevents is not respected for this:
>>
>> io_setup(1, [0x7fc64537f000]) = 0
>> io_submit(0x7fc64537f000, 10, [{pread, fildes=3, buf=0x1eb4000,
>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread,
>> fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3,
>> buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000,
>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}]) = 10
>>
>> which is unexpected, to me.
> ioctx_alloc()
> {
> [...]
>
> /*
> * We keep track of the number of available ringbuffer slots, to prevent
> * overflow (reqs_available), and we also use percpu counters for this.
> *
> * So since up to half the slots might be on other cpu's percpu counters
> * and unavailable, double nr_events so userspace sees what they
> * expected: additionally, we move req_batch slots to/from percpu
> * counters at a time, so make sure that isn't 0:
> */
> nr_events = max(nr_events, num_possible_cpus() * 4);
> nr_events *= 2;
> }

On a 4-lcore desktop:

io_setup(1, [0x7fc210041000]) = 0
io_submit(0x7fc210041000, 10000, [big array]) = 126
io_submit(0x7fc210041000, 10000, [big array]) = -1 EAGAIN (Resource
temporarily unavailable)

so, the user should already expect EAGAIN from io_submit() due to
resource limits. I'm sure the check could be tightened so that if we do
have to use a workqueue, we respect the user's limit rather than some
inflated number.

2017-03-06 18:36:18

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 11:17 AM, Avi Kivity wrote:
>
>
> On 03/06/2017 07:06 PM, Jens Axboe wrote:
>> On 03/06/2017 09:59 AM, Avi Kivity wrote:
>>>
>>> On 03/06/2017 06:08 PM, Jens Axboe wrote:
>>>> On 03/06/2017 08:59 AM, Avi Kivity wrote:
>>>>> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>>>>>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>>>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>>>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>>>>>
>>>>>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>>>>>> we're saving anything by pushing it there.
>>>>>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>>>>>> this information to the workqueue.
>>>>>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>>>>>> start blocking at some point. We can't expose a direct connection to
>>>>>>>> queue work like that, and let any user potentially create millions of
>>>>>>>> pending work items (and IOs).
>>>>>>> You wouldn't expect more concurrent events than the maxevents parameter
>>>>>>> that was supplied to io_setup syscall; it should have reserved any
>>>>>>> resources needed.
>>>>>> Doesn't matter what limit you apply, my point still stands - at some
>>>>>> point you have to return EAGAIN, or block. Returning EAGAIN without
>>>>>> the caller having flagged support for that change of behavior would
>>>>>> be problematic.
>>>>> Doesn't it already return EAGAIN (or some other error) if you exceed
>>>>> maxevents?
>>>> It's a setup thing. We check these limits when someone creates an IO
>>>> context, and carve out the specified entries form our global pool. Then
>>>> we free those "resources" when the io context is freed.
>>>>
>>>> Right now I can setup an IO context with 1000 entries on it, yet that
>>>> number has NO bearing on when io_submit() would potentially block or
>>>> return EAGAIN.
>>>>
>>>> We can have a huge gap on the intent signaled by io context setup, and
>>>> the reality imposed by what actually happens on the IO submission side.
>>> Isn't that a bug? Shouldn't that 1001st incomplete io_submit() return
>>> EAGAIN?
>>>
>>> Just tested it, and maxevents is not respected for this:
>>>
>>> io_setup(1, [0x7fc64537f000]) = 0
>>> io_submit(0x7fc64537f000, 10, [{pread, fildes=3, buf=0x1eb4000,
>>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread,
>>> fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3,
>>> buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000,
>>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}]) = 10
>>>
>>> which is unexpected, to me.
>> ioctx_alloc()
>> {
>> [...]
>>
>> /*
>> * We keep track of the number of available ringbuffer slots, to prevent
>> * overflow (reqs_available), and we also use percpu counters for this.
>> *
>> * So since up to half the slots might be on other cpu's percpu counters
>> * and unavailable, double nr_events so userspace sees what they
>> * expected: additionally, we move req_batch slots to/from percpu
>> * counters at a time, so make sure that isn't 0:
>> */
>> nr_events = max(nr_events, num_possible_cpus() * 4);
>> nr_events *= 2;
>> }
>
> On a 4-lcore desktop:
>
> io_setup(1, [0x7fc210041000]) = 0
> io_submit(0x7fc210041000, 10000, [big array]) = 126
> io_submit(0x7fc210041000, 10000, [big array]) = -1 EAGAIN (Resource
> temporarily unavailable)
>
> so, the user should already expect EAGAIN from io_submit() due to
> resource limits. I'm sure the check could be tightened so that if we do
> have to use a workqueue, we respect the user's limit rather than some
> inflated number.

This is why I previously said that the 1000 requests you potentially
asks for when setting up your IO context has NOTHING to do with when you
will run into EAGAIN. Yes, returning EAGAIN if the app exceeds the
limit that it itself has set is existing behavior and it certainly makes
sense. And it's an easily handled condition, since the app can just
backoff and wait/reap completion events.

But if we allow EAGAIN to bubble up from block request submission, then
that's a change in behavior. This can happen without the app having any
pending IO against that IO context, hence we can return EAGAIN to the
app that then has no reasonable way to handle that condition.

--
Jens Axboe

2017-03-06 18:50:37

[permalink] [raw]

Subject: Re: [PATCH 0/8 v2] Non-blocking AIO

On 03/06/2017 08:27 PM, Jens Axboe wrote:
> On 03/06/2017 11:17 AM, Avi Kivity wrote:
>>
>> On 03/06/2017 07:06 PM, Jens Axboe wrote:
>>> On 03/06/2017 09:59 AM, Avi Kivity wrote:
>>>> On 03/06/2017 06:08 PM, Jens Axboe wrote:
>>>>> On 03/06/2017 08:59 AM, Avi Kivity wrote:
>>>>>> On 03/06/2017 05:38 PM, Jens Axboe wrote:
>>>>>>> On 03/06/2017 08:29 AM, Avi Kivity wrote:
>>>>>>>> On 03/06/2017 05:19 PM, Jens Axboe wrote:
>>>>>>>>> On 03/06/2017 01:25 AM, Jan Kara wrote:
>>>>>>>>>> On Sun 05-03-17 16:56:21, Avi Kivity wrote:
>>>>>>>>>>>> The goal of the patch series is to return -EAGAIN/-EWOULDBLOCK if
>>>>>>>>>>>> any of these conditions are met. This way userspace can push most
>>>>>>>>>>>> of the write()s to the kernel to the best of its ability to complete
>>>>>>>>>>>> and if it returns -EAGAIN, can defer it to another thread.
>>>>>>>>>>>>
>>>>>>>>>>> Is it not possible to push the iocb to a workqueue? This will allow
>>>>>>>>>>> existing userspace to work with the new functionality, unchanged. Any
>>>>>>>>>>> userspace implementation would have to do the same thing, so it's not like
>>>>>>>>>>> we're saving anything by pushing it there.
>>>>>>>>>> That is not easy because until IO is fully submitted, you need some parts
>>>>>>>>>> of the context of the process which submits the IO (e.g. memory mappings,
>>>>>>>>>> but possibly also other credentials). So you would need to somehow transfer
>>>>>>>>>> this information to the workqueue.
>>>>>>>>> Outside of technical challenges, the API also needs to return EAGAIN or
>>>>>>>>> start blocking at some point. We can't expose a direct connection to
>>>>>>>>> queue work like that, and let any user potentially create millions of
>>>>>>>>> pending work items (and IOs).
>>>>>>>> You wouldn't expect more concurrent events than the maxevents parameter
>>>>>>>> that was supplied to io_setup syscall; it should have reserved any
>>>>>>>> resources needed.
>>>>>>> Doesn't matter what limit you apply, my point still stands - at some
>>>>>>> point you have to return EAGAIN, or block. Returning EAGAIN without
>>>>>>> the caller having flagged support for that change of behavior would
>>>>>>> be problematic.
>>>>>> Doesn't it already return EAGAIN (or some other error) if you exceed
>>>>>> maxevents?
>>>>> It's a setup thing. We check these limits when someone creates an IO
>>>>> context, and carve out the specified entries form our global pool. Then
>>>>> we free those "resources" when the io context is freed.
>>>>>
>>>>> Right now I can setup an IO context with 1000 entries on it, yet that
>>>>> number has NO bearing on when io_submit() would potentially block or
>>>>> return EAGAIN.
>>>>>
>>>>> We can have a huge gap on the intent signaled by io context setup, and
>>>>> the reality imposed by what actually happens on the IO submission side.
>>>> Isn't that a bug? Shouldn't that 1001st incomplete io_submit() return
>>>> EAGAIN?
>>>>
>>>> Just tested it, and maxevents is not respected for this:
>>>>
>>>> io_setup(1, [0x7fc64537f000]) = 0
>>>> io_submit(0x7fc64537f000, 10, [{pread, fildes=3, buf=0x1eb4000,
>>>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>>>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>>>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread,
>>>> fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3,
>>>> buf=0x1eb4000, nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000,
>>>> nbytes=4096, offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096,
>>>> offset=0}, {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0},
>>>> {pread, fildes=3, buf=0x1eb4000, nbytes=4096, offset=0}]) = 10
>>>>
>>>> which is unexpected, to me.
>>> ioctx_alloc()
>>> {
>>> [...]
>>>
>>> /*
>>> * We keep track of the number of available ringbuffer slots, to prevent
>>> * overflow (reqs_available), and we also use percpu counters for this.
>>> *
>>> * So since up to half the slots might be on other cpu's percpu counters
>>> * and unavailable, double nr_events so userspace sees what they
>>> * expected: additionally, we move req_batch slots to/from percpu
>>> * counters at a time, so make sure that isn't 0:
>>> */
>>> nr_events = max(nr_events, num_possible_cpus() * 4);
>>> nr_events *= 2;
>>> }
>> On a 4-lcore desktop:
>>
>> io_setup(1, [0x7fc210041000]) = 0
>> io_submit(0x7fc210041000, 10000, [big array]) = 126
>> io_submit(0x7fc210041000, 10000, [big array]) = -1 EAGAIN (Resource
>> temporarily unavailable)
>>
>> so, the user should already expect EAGAIN from io_submit() due to
>> resource limits. I'm sure the check could be tightened so that if we do
>> have to use a workqueue, we respect the user's limit rather than some
>> inflated number.
> This is why I previously said that the 1000 requests you potentially
> asks for when setting up your IO context has NOTHING to do with when you
> will run into EAGAIN. Yes, returning EAGAIN if the app exceeds the
> limit that it itself has set is existing behavior and it certainly makes
> sense. And it's an easily handled condition, since the app can just
> backoff and wait/reap completion events.

Every time I used aio, I considered maxevents to be the maximum number
of in-flight requests for that queue, and observed this limit
religiously. It's possible others don't.

> But if we allow EAGAIN to bubble up from block request submission, then
> that's a change in behavior. This can happen without the app having any
> pending IO against that IO context, hence we can return EAGAIN to the
> app that then has no reasonable way to handle that condition.
>

For sure (and it's a different EAGAIN -- it's tied to the iocb, not
request submission). But we do have an upper bound for the number of
concurrent requests, even if inflated, so having the kernel convert a
blocking iocb into a workqueue item does not allow userspace to exploit
the kernel.

We could limit the number of workqueue submissions to maxevents, and
queue anything between maxevents and (maxevents * inflation_factor)
using a regular queue. So the intent of maxevents is respected, and
applications that overflow it are not regressed.

if iocb would overflow inflated maxevents:
io_submit returns EAGAIN
elseif iocb can be submitted asynchronously:
do that
elseif number of iocbs running in workqueues < maxevents:
push to a workqueue
else
queue somewhere, when a work_item completes it can pick up an
iocb from the queue

this enables aio for all filesystems, and doesn't require lots of idle
thread pools if the filesystem works or useless syscalls if it doesn't.
It's a lot more work for the kernel, but results in a tighter and
simpler interface.

2017-03-08 07:05:22

by Sagi Grimberg

[permalink] [raw]

Subject: Re: [PATCH 5/8] nowait aio: return on congested block device

> - if (likely(blk_queue_enter(q, false) == 0)) {
> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT)) == 0)) {
> ret = q->make_request_fn(q, bio);

I think that for ->make_request to not block we'd need to set
BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
allocation.

Something like the untested addition below:
--
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 09af8ff18719..40e78b57fb44 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -119,6 +119,9 @@ struct request *blk_mq_sched_get_request(struct
request_queue *q,
if (likely(!data->hctx))
data->hctx = blk_mq_map_queue(q, data->ctx->cpu);

+ if (likely(bio) && bio_flagged(bio, BIO_NOWAIT))
+ data->flags |= BLK_MQ_REQ_NOWAIT;
+
if (e) {
data->flags |= BLK_MQ_REQ_INTERNAL;
--

2017-03-08 15:04:15

[permalink] [raw]

Subject: Re: [PATCH 5/8] nowait aio: return on congested block device

On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
>
>> - if (likely(blk_queue_enter(q, false) == 0)) {
>> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
>> == 0)) {
>> ret = q->make_request_fn(q, bio);
>
> I think that for ->make_request to not block we'd need to set
> BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
> allocation.
>
> Something like the untested addition below:

I did that in the first series, but there are too many reasons to block
in blk-mq [1]. I dropped blk-mq work in v2.

[1] https://patchwork.kernel.org/patch/9571051/

--
Goldwyn

2017-03-08 15:28:06

[permalink] [raw]

Subject: Re: [PATCH 5/8] nowait aio: return on congested block device

On Wed 08-03-17 09:00:09, Goldwyn Rodrigues wrote:
>
>
> On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
> >
> >> - if (likely(blk_queue_enter(q, false) == 0)) {
> >> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
> >> == 0)) {
> >> ret = q->make_request_fn(q, bio);
> >
> > I think that for ->make_request to not block we'd need to set
> > BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
> > allocation.
> >
> > Something like the untested addition below:
>
> I did that in the first series, but there are too many reasons to block
> in blk-mq [1]. I dropped blk-mq work in v2.
>
> [1] https://patchwork.kernel.org/patch/9571051/

Well, that's not really good. If we cannot support this for both blk-mq and
legacy block layer the feature will not be usable. So please work on blk-mq
support as well.

Honza

--
Jan Kara <[email protected]>
SUSE Labs, CR

2017-03-08 15:52:07

[permalink] [raw]

Subject: Re: [PATCH 5/8] nowait aio: return on congested block device

On Wed, Mar 08, 2017 at 04:28:06PM +0100, Jan Kara wrote:
> Well, that's not really good. If we cannot support this for both blk-mq and
> legacy block layer the feature will not be usable. So please work on blk-mq
> support as well.

Exactly. In addition to that anything implementing a feature for the
legacy request request code but not blk-mq is grounds for an instant
NAK.

2017-03-08 16:17:04

[permalink] [raw]

Subject: Re: [PATCH 5/8] nowait aio: return on congested block device

On 03/08/2017 08:00 AM, Goldwyn Rodrigues wrote:
>
>
> On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
>>
>>> - if (likely(blk_queue_enter(q, false) == 0)) {
>>> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
>>> == 0)) {
>>> ret = q->make_request_fn(q, bio);
>>
>> I think that for ->make_request to not block we'd need to set
>> BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
>> allocation.
>>
>> Something like the untested addition below:
>
> I did that in the first series, but there are too many reasons to block
> in blk-mq [1]. I dropped blk-mq work in v2.

That's complete nonsense, there are no more places in blk-mq that will
block that in the legacy path. Most of the examples from your URL:

> [1] https://patchwork.kernel.org/patch/9571051/

are not blk-mq, but writeback throttling, and drivers that explicitly
hook into ->make_request_fn.

As others have mentioned, it's a total non-starter to focus on the
deprecated IO path and just ignore the new one. Back to the drawing
board.

--
Jens Axboe

2017-03-09 02:18:54