Re: [PATCH 1/4] block: introduce submit_bio_noacct_add_head

From: Danny Shih <dannyshih@synology.com>
To: John Stoffel <john@stoffel.org>
Cc: axboe@kernel.dk, agk@redhat.com, snitzer@redhat.com,
	dm-devel@redhat.com, song@kernel.org,
	linux-block@vger.kernel.org, linux-raid@vger.kernel.org
Subject: Re: [PATCH 1/4] block: introduce submit_bio_noacct_add_head
Date: Wed, 30 Dec 2020 17:51:29 +0800	[thread overview]
Message-ID: <abac671f-91f2-ca4e-7f77-8bb5da85a4cc@synology.com> (raw)
In-Reply-To: <24555.49943.411197.147225@quad.stoffel.home>

Hi, John,

Thank you for taking the time to write a review.

John Stoffel <john@stoffel.org> writes:
>>>>>> "dannyshih" == dannyshih  <dannyshih@synology.com> writes:
> dannyshih> From: Danny Shih <dannyshih@synology.com>
> 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.

Sorry for so many spelling errors.

The message should be

"Provide a way for stacking block device to re-submit

the bio which should be handled first."

I will fix it.

> dannyshih> Signed-off-by: Danny Shih <dannyshih@synology.com>
> dannyshih> Reviewed-by: Allen Peng <allenpeng@synology.com>
> dannyshih> Reviewed-by: Alex Wu <alexwu@synology.com>
> 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(&current->bio_list[0], bio);
> dannyshih> +		if (add_head)
> dannyshih> +			bio_list_add_head(&current->bio_list[0], bio);
> dannyshih> +		else
> dannyshih> +			bio_list_add(&current->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?

There is no logical change while using submit_bio_noacct() after my 
patch. So I didn't change

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.

Sorry for the grammar mistake. And I wrote the wrong function name here.

It should be replaced by submit_bio_noacct_add_head.

About the function name, the name of submit_bio_noacct_add_head()

is trying to let drivers know that this is just an alternative version of

submit_bio_noacct(). The only difference is that this function adds bio to

the head of current->bio_list, and submit_bio_noacct() adds it to the tail.

About the documentation, what if I change it like:

"submit_bio_noacct_add_head - re-submit a bio which needs to

be handled first to the block device layer for I/O, because it has

sequential relevance with the bio handling in current ->submit_bio.

Alternative submit_bio_noacct() adds bio to the head of

current->bio_list. To keep bio sequence, this function is used

when a block device splits bio and re-submits the remainder back

to itself. This makes sure that the re-submitted bio will be handle

just after the split part of the original bio."

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

Best Regards,

Danny Shih