Re: [PATCH] block: Document the bio splitting functions

[Hannes Reinecke <hare@suse.de>]

On 7/1/19 6:23 PM, Bart Van Assche wrote:
> Since what the bio splitting functions do is nontrivial, document these
> functions.
> 
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/blk-merge.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 1ea00da12ca3..038eaee4438a 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -192,6 +192,23 @@ static bool bvec_split_segs(const struct request_queue *q,
>  	return !!len;
>  }
>  
> +/**
> + * blk_bio_segment_split - split a bio in two bios
> + * @q:    [in] request queue pointer
> + * @bio:  [in] bio to be split
> + * @bs:	  [in] bio set to allocate the clone from
> + * @segs: [out] number of segments in the bio with the first half of the sectors
> + *
> + * Clones @bio, updates the bi_iter of the clone to represent the first sectors
> + * of @bio and updates @bio->bi_iter to represent the remaining sectors. The
> + * following is guaranteed for the cloned bio:
> + * - That it has at most get_max_io_size(@q, @bio) sectors.
> + * - That it has at most queue_max_segments(@q) segments.
> + *
> + * Except for discard requests the cloned bio will point at the bi_io_vec of
> + * the original bio. It is the responsibility of the caller to ensure that the
> + * original bio is not freed before the cloned bio.
> + */
>  static struct bio *blk_bio_segment_split(struct request_queue *q,
>  					 struct bio *bio,
>  					 struct bio_set *bs,
> @@ -248,6 +265,16 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
>  	return bio_split(bio, sectors, GFP_NOIO, bs);
>  }
>  
> +/**
> + * __blk_queue_split - split a bio and submit the second half
> + * @q:       [in] request queue pointer
> + * @bio:     [in, out] bio to be split
> + * @nr_segs: [out] number of segments in the first bio
> + *
> + * Splits a bio into two bios, chains the two bios, submits the second half
> + * and stores a pointer to the first half in *@bio. If the second bio is still
> + * too big it will be split by a recursive call to this function.
> + */
>  void __blk_queue_split(struct request_queue *q, struct bio **bio,
>  		unsigned int *nr_segs)
>  {
> @@ -292,6 +319,14 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
>  	}
>  }
>  
> +/**
> + * blk_queue_split - split a bio and submit the second half
> + * @q:   [in] request queue pointer
> + * @bio: [in, out] bio to be split
> + *
> + * Splits a bio into two bios, chains the two bios, submits the second half
> + * and stores a pointer to the first half in *@bio.
> + */
>  void blk_queue_split(struct request_queue *q, struct bio **bio)
>  {
>  	unsigned int nr_segs;
> 
Could you add a reference to the bio_set in those descriptions, too?
It's really non-obvious that 'split' will reference the bio_set of the
queue, and hence the queue _must not_ go away while the bio is allocated.
This becomes especially tricky if the bio is remapped to another queue
later on, as then we'll lose the reference to the original queue, and
have no idea that suddenly we have a bio with references to _two_
request queues.

Maybe we should even add a warning somewhere...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)