Lukas Czerner <lczerner@xxxxxxxxxx> writes:
> On Fri, 4 Mar 2011, Jeff Moyer wrote:
>
>> Lukas Czerner <lczerner@xxxxxxxxxx> writes:
>>
>> > On Fri, 4 Mar 2011, Jeff Moyer wrote:
>> >> It seems to me like it might be better to just not complete anything
>> >> until the count is zero. Why issue a wakeup for every bio?
>> >> fs/direct-io does something similar, maybe take a look at the
>> >> dio_bio_end* routines and see if that would fit well here. With your
>> >> scheme, I worry about missing a completion, maybe because the first bio
>> >> completes before you are done submitting bios. Is that possible?
>> >
>> > I do not think it is possible. For every bio submitted there is
>> > wait_for_completion called. When bio complete()s completion->done is
>> > incremented (under the wait->lock). In wait_for_completion() we are
>> > waiting for single submitted bio to complete (completion->done > 0),
>> > then completion->done is decremented. It seems like simple
>> > synchronization.
>> >
>> > I am not sure what wakeup you have in mind, but thanks for the tip I'll
>> > look in fs/direct-io.
>>
>> Let's say you have several bios to submit, and the first bio is errored
>> immediately in submit_bio. Since you didn't add yourself to the
>> waitqueue yet, you might miss the wakeup and sleep forever.
>>
>> Cheers,
>> Jeff
>>
>
> (Adding Dimitry and Jens into CC)
>
> This can not happen. submit_bio does not return any value. The way how
> it does notify caller about its status is via ->bio_end_io (see comment
> for __generic_make_request()). Now the ->bio_end_io is in this case
> always set because it is the first thing we are doing, so for every bio
> submitted there will be appropriate complete() and wait_for_completition()
> call.
>
> The one thing can fail though, and it is bio_alloc() however when this
> fails we are jumping out of the loop immediately without touching
> "issued" at all, so if it is a first bio issued = 0, hence there is
> nothing to wait for.
>
> I do not see any problems here.
Yeah, somehow I actually missed the whole point of completions (the done
variable). Anyway, I agree with you.
> But, now I can see you point about calling wakeup() for every completed
> bio, which is not strictly needed and we should call complete() only
> once when the last bio is completed. So how about this ?
>
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 1a320d2..ccf5a40 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -109,7 +109,6 @@ struct bio_batch
> atomic_t done;
> unsigned long flags;
> struct completion *wait;
> - bio_end_io_t *end_io;
> };
>
> static void bio_batch_end_io(struct bio *bio, int err)
> @@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err)
> else
> clear_bit(BIO_UPTODATE, &bb->flags);
> }
> - if (bb) {
> - if (bb->end_io)
> - bb->end_io(bio, err);
> - atomic_inc(&bb->done);
> - complete(bb->wait);
> - }
> + if (bb)
> + if (atomic_dec_and_test(&bb->done))
> + complete(bb->wait);
I don't think we actually have to check for bb != NULL, do you?
> bio_put(bio);
> }
>
> @@ -150,13 +146,12 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
> int ret;
> struct bio *bio;
> struct bio_batch bb;
> - unsigned int sz, issued = 0;
> + unsigned int sz;
> DECLARE_COMPLETION_ONSTACK(wait);
>
> - atomic_set(&bb.done, 0);
> + atomic_set(&bb.done, 1);
> bb.flags = 1 << BIO_UPTODATE;
> bb.wait = &wait;
> - bb.end_io = NULL;
>
> submit:
> ret = 0;
> @@ -185,12 +180,12 @@ submit:
> break;
> }
> ret = 0;
> - issued++;
> + atomic_inc(&bb.done);
> submit_bio(WRITE, bio);
> }
>
> /* Wait for bios in-flight */
> - while (issued != atomic_read(&bb.done))
> + if (!atomic_dec_and_test(&bb.done))
> wait_for_completion(&wait);
>
> if (!test_bit(BIO_UPTODATE, &bb.flags))
Yep, this looks good to me. Thanks for fixing this up, Lukas!
Reviewed-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
