Re: [PATCH 01/33] block: add a lower-level bio_add_page interface

On 5/16/2018 11:35 PM, Christoph Hellwig wrote:

On Wed, May 16, 2018 at 10:36:14AM +0530, Ritesh Harjani wrote:

quoted

1. if bio_full is true that means no space in bio->bio_io_vec[] no?
Than how come we are still proceeding ahead with only warning?
While originally in bio_add_page we used to return after checking
bio_full. Callers can still call __bio_add_page directly right.

I you don't know if the bio is full or not don't use __bio_add_page,
keep using bio_add_page.  The WARN_ON is just a debug tool to catch
cases where the developer did use it incorrectly.

quoted

2. IF above is correct why don't we set the bio->bi_max_vecs to the size
of the slab instead of keeeping it to nr_iovecs which user requested?
(in bio_alloc_bioset)

Because we limit the user to the number that the user requested.  Not
that this patch changes anything about that.

quoted

3. Could you please help understand why for cloned bio we still allow
__bio_add_page to work? why not WARN and return like in original code?

It doesn't work, and I have now added the WARN_ON to deal with any
incorrect usage.

quoted

quoted

-	if (bio->bi_vcnt >= bio->bi_max_vecs)
-		return 0;

Originally here we were supposed to return and not proceed further.
Should __bio_add_page not have similar checks to safeguard crossing
the bio_io_vec[] boundary?

No, __bio_add_page is the "I know what I am doing" interface.

Thanks for explaining. I guess I missed reading the comment made on top of function __bio_add_page.
"The caller must ensure that @bio has space for another bvec"

This discussion helped me understand a bit about bios & bio_vec.

Thanks!!
Ritesh


-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.