On 2/3/2020 12:34 AM, Christoph Hellwig wrote:

On Fri, Jan 31, 2020 at 06:08:16PM +0000, Bijan Mottahedeh wrote:

quoted

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);
         }

That actualy is before the submit_bio call, which is just below that
code.

quoted

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.

But we never access the bio after submit_bio returns for the single
bio async case, so I still don't understand the problem.

Ok, I see your point.

My concern is with the code below for the single bio async case:

                            qc = submit_bio(bio);

                            if (polled)
                                    WRITE_ONCE(iocb->ki_cookie, qc);

The bio/dio can be freed before the the cookie is written which is what 
I'm seeing, and I thought this may lead to a scenario where that iocb 
request could be completed, freed, reallocated, and resubmitted in 
io_uring layer; i.e., I thought the cookie could be written into the 
wrong iocb.

So I have two questions:

Could we ever update a wrong iocb?

Is the additional bio_get() the right way to mitigate that?  I see that 
this might not be true since end_io() calls ki_complete() regardless.

Thanks.

--bijan