Re: [PATCH] block: avoid extra bio reference for async O_DIRECT
From: Jens Axboe <axboe@kernel.dk>
Date: 2018-11-29 22:58:30
Subsystem:
filesystems (vfs and infrastructure), the rest · Maintainers:
Alexander Viro, Christian Brauner, Linus Torvalds
On 11/29/18 3:55 PM, Jens Axboe wrote:
The bio referencing has a trick that doesn't do any actual atomic inc/dec on the reference count until we have to elevator to > 1. For the async IO O_DIRECT case, we can't use the simple DIO variants, so we use __blkdev_direct_IO(). It always grabs an extra reference to the bio after allocation, which means we then enter the slower path of actually having to do atomic_inc/dec on the count. We don't need to do that for the async case, unless we end up going multi-bio, in which case we're already doing huge amounts of IO. For the smaller IO case (< BIO_MAX_PAGES), we can do without the extra ref.
I accidentally generated this against the aio-poll branch, here's a version that is not.
diff --git a/fs/block_dev.c b/fs/block_dev.c
index d233a59ea364..5c0a224bf9cb 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c@@ -281,10 +281,25 @@ struct blkdev_dio { static struct bio_set blkdev_dio_pool; +static void blkdev_bio_dirty_release(struct bio *bio, bool should_dirty) +{ + if (should_dirty) { + bio_check_pages_dirty(bio); + } else { + struct bio_vec *bvec; + int i; + + bio_for_each_segment_all(bvec, bio, i) + put_page(bvec->bv_page); + bio_put(bio); + } +} + static void blkdev_bio_end_io(struct bio *bio) { struct blkdev_dio *dio = bio->bi_private; bool should_dirty = dio->should_dirty; + bool should_free = false; if (dio->multi_bio && !atomic_dec_and_test(&dio->ref)) { if (bio->bi_status && !dio->bio.bi_status)
@@ -302,25 +317,20 @@ static void blkdev_bio_end_io(struct bio *bio) } dio->iocb->ki_complete(iocb, ret, 0); - bio_put(&dio->bio); } else { struct task_struct *waiter = dio->waiter; WRITE_ONCE(dio->waiter, NULL); blk_wake_io_task(waiter); } + /* last bio is done, now we can free the parent holding dio */ + should_free = dio->multi_bio; } - if (should_dirty) { - bio_check_pages_dirty(bio); - } else { - struct bio_vec *bvec; - int i; - - bio_for_each_segment_all(bvec, bio, i) - put_page(bvec->bv_page); - bio_put(bio); - } + if (!dio->is_sync) + blkdev_bio_dirty_release(bio, should_dirty); + if (should_free) + bio_put(&dio->bio); } static ssize_t
@@ -343,14 +353,15 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) return -EINVAL; bio = bio_alloc_bioset(GFP_KERNEL, nr_pages, &blkdev_dio_pool); - bio_get(bio); /* extra ref for the completion handler */ dio = container_of(bio, struct blkdev_dio, bio); dio->is_sync = is_sync = is_sync_kiocb(iocb); - if (dio->is_sync) + if (dio->is_sync) { dio->waiter = current; - else + bio_get(bio); + } else { dio->iocb = iocb; + } dio->size = 0; dio->multi_bio = false;
@@ -400,6 +411,13 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) } if (!dio->multi_bio) { + /* + * async needs an extra reference if we are goign + * multi-bio only, so we can ensure that 'dio' + * sticks around. + */ + if (!is_sync) + bio_get(bio); dio->multi_bio = true; atomic_set(&dio->ref, 2); } else {
@@ -433,6 +451,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) if (likely(!ret)) ret = dio->size; + blkdev_bio_dirty_release(bio, dio->should_dirty); bio_put(&dio->bio); return ret; }
--
Jens Axboe