>>>>> "Danny" == Danny Shih <dannyshih@xxxxxxxxxxxx> writes:
Danny> Hi, John,
Danny> Thank you for taking the time to write a review.
Danny> John Stoffel <john@xxxxxxxxxxx> writes:
>>>>>>> "dannyshih" == dannyshih <dannyshih@xxxxxxxxxxxx> writes:
dannyshih> From: Danny Shih <dannyshih@xxxxxxxxxxxx>
dannyshih> Porvide a way for stacking block device to re-submit the bio
dannyshih> which sholud be handled firstly.
>>
>> You're spelling needs to be fixed in these messages.
Danny> Sorry for so many spelling errors.
Danny> The message should be
Danny> "Provide a way for stacking block device to re-submit
Danny> the bio which should be handled first."
Danny> I will fix it.
Great, though my second question is *why* it needs to be handled
first? What is the difference between stacked and un-stacked devices
and how could it be done in a way that doesn't require a seperate
function like this?
dannyshih> Signed-off-by: Danny Shih <dannyshih@xxxxxxxxxxxx>
dannyshih> Reviewed-by: Allen Peng <allenpeng@xxxxxxxxxxxx>
dannyshih> Reviewed-by: Alex Wu <alexwu@xxxxxxxxxxxx>
dannyshih> ---
dannyshih> block/blk-core.c | 44 +++++++++++++++++++++++++++++++++-----------
dannyshih> include/linux/blkdev.h | 1 +
dannyshih> 2 files changed, 34 insertions(+), 11 deletions(-)
>>
dannyshih> diff --git a/block/blk-core.c b/block/blk-core.c
dannyshih> index 96e5fcd..693dc83 100644
dannyshih> --- a/block/blk-core.c
dannyshih> +++ b/block/blk-core.c
dannyshih> @@ -1031,16 +1031,7 @@ static blk_qc_t __submit_bio_noacct_mq(struct bio *bio)
dannyshih> return ret;
dannyshih> }
>>
dannyshih> -/**
dannyshih> - * submit_bio_noacct - re-submit a bio to the block device layer for I/O
dannyshih> - * @bio: The bio describing the location in memory and on the device.
dannyshih> - *
dannyshih> - * This is a version of submit_bio() that shall only be used for I/O that is
dannyshih> - * resubmitted to lower level drivers by stacking block drivers. All file
dannyshih> - * systems and other upper level users of the block layer should use
dannyshih> - * submit_bio() instead.
dannyshih> - */
dannyshih> -blk_qc_t submit_bio_noacct(struct bio *bio)
dannyshih> +static blk_qc_t do_submit_bio_noacct(struct bio *bio, bool add_head)
dannyshih> {
dannyshih> if (!submit_bio_checks(bio))
dannyshih> return BLK_QC_T_NONE;
dannyshih> @@ -1052,7 +1043,10 @@ blk_qc_t submit_bio_noacct(struct bio *bio)
dannyshih> * it is active, and then process them after it returned.
dannyshih> */
dannyshih> if (current->bio_list) {
dannyshih> - bio_list_add(¤t->bio_list[0], bio);
dannyshih> + if (add_head)
dannyshih> + bio_list_add_head(¤t->bio_list[0], bio);
dannyshih> + else
dannyshih> + bio_list_add(¤t->bio_list[0], bio);
dannyshih> return BLK_QC_T_NONE;
dannyshih> }
>>
dannyshih> @@ -1060,9 +1054,37 @@ blk_qc_t submit_bio_noacct(struct bio *bio)
dannyshih> return __submit_bio_noacct_mq(bio);
dannyshih> return __submit_bio_noacct(bio);
dannyshih> }
dannyshih> +
dannyshih> +/**
dannyshih> + * submit_bio_noacct - re-submit a bio to the block device layer for I/O
dannyshih> + * @bio: The bio describing the location in memory and on the device.
dannyshih> + *
dannyshih> + * This is a version of submit_bio() that shall only be used for I/O that is
dannyshih> + * resubmitted to lower level drivers by stacking block drivers. All file
dannyshih> + * systems and other upper level users of the block layer should use
dannyshih> + * submit_bio() instead.
dannyshih> + */
dannyshih> +blk_qc_t submit_bio_noacct(struct bio *bio)
dannyshih> +{
dannyshih> + return do_submit_bio_noacct(bio, false);
dannyshih> +}
dannyshih> EXPORT_SYMBOL(submit_bio_noacct);
>>
>> So why is it named "submit_bio_noacct" when it's supposed to be only
>> used by layers submitting to lower level drivers. How can this be
>> figured out by drivers automatically, so the writed doesn't have to
>> know about this?
Danny> There is no logical change while using submit_bio_noacct() after my
Danny> patch. So I didn't change
Danny> the name and the documentation of submit_bio_noacct().
>>
dannyshih> /**
dannyshih> + * submit_bio_noacct - re-submit a bio, which needs to be handle firstly,
dannyshih> + * to the block device layer for I/O
dannyshih> + * @bio: The bio describing the location in memory and on the device.
dannyshih> + *
dannyshih> + * alternative submit_bio_noacct() which add bio to the head of
dannyshih> + * current->bio_list.
dannyshih> + */
>>
>> Firstly isn't proper english. Maybe something like:
>>
>> submit_bio_noacct - re-submit a bio which needs to be handled first
>> because <reasons> to the block device layer for I/O
>>
>> But the name still sucks, and the *reason* the bio needs to be handled
>> differently isn't well explained.
Danny> Sorry for the grammar mistake. And I wrote the wrong function name here.
Danny> It should be replaced by submit_bio_noacct_add_head.
Danny> About the function name, the name of submit_bio_noacct_add_head()
Danny> is trying to let drivers know that this is just an alternative version of
Danny> submit_bio_noacct(). The only difference is that this function adds bio to
Danny> the head of current->bio_list, and submit_bio_noacct() adds it to the tail.
Danny> About the documentation, what if I change it like:
Danny> "submit_bio_noacct_add_head - re-submit a bio which needs to
Danny> be handled first to the block device layer for I/O, because it has
Danny> sequential relevance with the bio handling in current ->submit_bio.
Danny> Alternative submit_bio_noacct() adds bio to the head of
current-> bio_list. To keep bio sequence, this function is used
Danny> when a block device splits bio and re-submits the remainder back
Danny> to itself. This makes sure that the re-submitted bio will be handle
Danny> just after the split part of the original bio."
Danny> Thanks for your suggestion.
dannyshih> +blk_qc_t submit_bio_noacct_add_head(struct bio *bio)
dannyshih> +{
dannyshih> + return do_submit_bio_noacct(bio, true);
dannyshih> +}
dannyshih> +EXPORT_SYMBOL(submit_bio_noacct_add_head);
dannyshih> +
dannyshih> +/**
dannyshih> * submit_bio - submit a bio to the block device layer for I/O
dannyshih> * @bio: The &struct bio which describes the I/O
dannyshih> *
dannyshih> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
dannyshih> index 070de09..b0080d0 100644
dannyshih> --- a/include/linux/blkdev.h
dannyshih> +++ b/include/linux/blkdev.h
dannyshih> @@ -905,6 +905,7 @@ static inline void rq_flush_dcache_pages(struct request *rq)
dannyshih> extern int blk_register_queue(struct gendisk *disk);
dannyshih> extern void blk_unregister_queue(struct gendisk *disk);
dannyshih> blk_qc_t submit_bio_noacct(struct bio *bio);
dannyshih> +blk_qc_t submit_bio_noacct_add_head(struct bio *bio);
dannyshih> extern void blk_rq_init(struct request_queue *q, struct request *rq);
dannyshih> extern void blk_put_request(struct request *);
dannyshih> extern struct request *blk_get_request(struct request_queue *, unsigned int op,
dannyshih> --
dannyshih> 2.7.4
Danny> Best Regards,
Danny> Danny Shih
