Re: [PATCH 1/1] block: Manage bio references so the bio persists until necessary

[Bijan Mottahedeh <bijan.mottahedeh@oracle.com>]

On 1/30/2020 10:42 PM, Christoph Hellwig wrote:
> On Thu, Jan 30, 2020 at 07:23:42PM -0800, Bijan Mottahedeh wrote:
>> Get a reference to a bio, so it won't be freed if end_io() gets to
>> it before submit_io() returns.  Defer the release of the first bio
>> in a mult-bio request until the last end_io() since the first bio is
>> embedded in the dio structure and must therefore persist through an
>> entire multi-bio request.
> Can you explain the issue a little more?
>
> The initial bio is embedded into the dio, and will have a reference
> until the bio_put call at the end of the function, so we can't have
> a race for that one and won't ever need the refcount for the single
> bio case.  Avoiding the atomic is pretty important for aio/uring
> performance.
I think the problem is that in the async case, bio_get() is not called 
for the initial bio *before* the submit_bio() call for that bio:

     if (dio->is_sync) {
         dio->waiter = current;
         bio_get(bio);
     } else {
         dio->iocb = iocb;
     }


The bio_get() call for the async case happens too late, after the 
submit_bio() call:

         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 {
             atomic_inc(&dio->ref);
         }

See my previous message on the mailing list titled "io_uring: acquire 
ctx->uring_lock before calling io_issue_sqe()" for the more details but 
basically blkdev_bio_end_io() can be called before submit_bio() returns 
and therefore free the initial bio.  I think it is the unconditional 
bio_put() at the end that does it.