Re: [PATCH] block: avoid extra bio reference for async O_DIRECT

[Christoph Hellwig]

I like the idea, but I don't think it is correct as-is.  We can't just
move the blkdev_bio_dirty_release for the last bio into the caller, as
the completion order that decrements the refcount might have different
ordering.

Something like the simpler patch below might archive your goal, though:

---
>From 923ee8e2358de04037ba6f7269aaf321f7b2e173 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <h...@lst.de>
Date: Fri, 30 Nov 2018 09:23:48 +0100
Subject: block: avoid extra bio reference for async O_DIRECT

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.

Based on an earlier patch (and commit log) from Jens Axboe.

Signed-off-by: Christoph Hellwig <h...@lst.de>
---
 fs/block_dev.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index d233a59ea364..e1886cc7048f 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -302,7 +302,8 @@ static void blkdev_bio_end_io(struct bio *bio)
                        }
 
                        dio->iocb->ki_complete(iocb, ret, 0);
-                       bio_put(&dio->bio);
+                       if (dio->multi_bio)
+                               bio_put(&dio->bio);
                } else {
                        struct task_struct *waiter = dio->waiter;
 
@@ -343,14 +344,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 +402,13 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter 
*iter, int nr_pages)
                }
 
                if (!dio->multi_bio) {
+                       /*
+                        * AIO needs an extra reference to ensure the dio
+                        * structure which is embedded into the first bio
+                        * stays around.
+                        */
+                       if (!is_sync)
+                               bio_get(bio);
                        dio->multi_bio = true;
                        atomic_set(&dio->ref, 2);
                } else {
-- 
2.19.1