Re: [PATCH 11/11] block: don't check for BIO_MAX_PAGES in blk_bio_segment_split()

[NeilBrown]

On Fri, Apr 21 2017, Ming Lei wrote:

> On Fri, Apr 21, 2017 at 7:34 PM, Christoph Hellwig  wrote:
>> On Thu, Apr 20, 2017 at 03:38:50PM +1000, NeilBrown wrote:
>>> blk_bio_segment_split() makes sure bios have no more than
>>> BIO_MAX_PAGES entries in the bi_io_vec.
>>> This was done because bio_clone_bioset() (when given a
>>> mempool bioset) could not handle larger io_vecs.
>>>
>>> No driver uses bio_clone_bioset() any more, they all
>>> use bio_clone_fast() if anything, and bio_clone_fast()
>>> doesn't clone the bi_io_vec.
>>
>> Hmm.  From Jens tree:
>>
>> drivers/lightnvm/pblk-read.c:   int_bio = bio_clone_bioset(bio, 
>> GFP_KERNEL, fs_bio_set);
>> drivers/md/raid1.c: mbio = 
>> bio_clone_bioset_partial(bio, GFP_NOIO,
>> drivers/md/raid1.c: mbio = 
>> bio_clone_bioset_partial(bio, GFP_NOIO,
>
> Btrfs use bio_clone_bioset() too:
>
>  fs/btrfs/extent_io.c:2703:  new = bio_clone_bioset(bio,
> gfp_mask, btrfs_bioset);
>

btrfs is a filesystem, not a driver.  So it doesn't count.
The bios it uses were not yet processed by blk_bio_segment_split(),
so changes there cannot be relevant to btrfs.

Thanks,
NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH 11/11] block: don't check for BIO_MAX_PAGES in blk_bio_segment_split()

[NeilBrown]

On Fri, Apr 21 2017, Christoph Hellwig wrote:

> On Thu, Apr 20, 2017 at 03:38:50PM +1000, NeilBrown wrote:
>> blk_bio_segment_split() makes sure bios have no more than
>> BIO_MAX_PAGES entries in the bi_io_vec.
>> This was done because bio_clone_bioset() (when given a
>> mempool bioset) could not handle larger io_vecs.
>> 
>> No driver uses bio_clone_bioset() any more, they all
>> use bio_clone_fast() if anything, and bio_clone_fast()
>> doesn't clone the bi_io_vec.
>
> Hmm.  From Jens tree:
>
> drivers/lightnvm/pblk-read.c:   int_bio = bio_clone_bioset(bio, 
> GFP_KERNEL, fs_bio_set);

That is new and I missed it.
It should be bio_clone_fast(), and it shouldn't use fs_bio_set (as it is
not a filesystem).


> drivers/md/raid1.c: mbio = 
> bio_clone_bioset_partial(bio, GFP_NOIO,
> drivers/md/raid1.c: mbio = 
> bio_clone_bioset_partial(bio, GFP_NOIO,

These have since been changed to bio_clone_fast() or similar (in the md
tree) - and bio_clone_bioset_partial() is gone.

>
> I did not see your series touching either of those.  They probably
> don't matter for the code removed as the bios are controlled by the
> drivers, but the changelog still seems odd.

Depending one what base it ends up being applied to, it may or may not
be correct...  the md changes have been in -next since 28march.

Thanks,
NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH 11/11] block: don't check for BIO_MAX_PAGES in blk_bio_segment_split()

[Ming Lei]

On Fri, Apr 21, 2017 at 7:34 PM, Christoph Hellwig  wrote:
> On Thu, Apr 20, 2017 at 03:38:50PM +1000, NeilBrown wrote:
>> blk_bio_segment_split() makes sure bios have no more than
>> BIO_MAX_PAGES entries in the bi_io_vec.
>> This was done because bio_clone_bioset() (when given a
>> mempool bioset) could not handle larger io_vecs.
>>
>> No driver uses bio_clone_bioset() any more, they all
>> use bio_clone_fast() if anything, and bio_clone_fast()
>> doesn't clone the bi_io_vec.
>
> Hmm.  From Jens tree:
>
> drivers/lightnvm/pblk-read.c:   int_bio = bio_clone_bioset(bio, 
> GFP_KERNEL, fs_bio_set);
> drivers/md/raid1.c: mbio = 
> bio_clone_bioset_partial(bio, GFP_NOIO,
> drivers/md/raid1.c: mbio = 
> bio_clone_bioset_partial(bio, GFP_NOIO,

Btrfs use bio_clone_bioset() too:

 fs/btrfs/extent_io.c:2703:  new = bio_clone_bioset(bio,
gfp_mask, btrfs_bioset);

Thanks,
Ming Lei

Re: [PATCH 11/11] block: don't check for BIO_MAX_PAGES in blk_bio_segment_split()

[Christoph Hellwig]

On Thu, Apr 20, 2017 at 03:38:50PM +1000, NeilBrown wrote:
> blk_bio_segment_split() makes sure bios have no more than
> BIO_MAX_PAGES entries in the bi_io_vec.
> This was done because bio_clone_bioset() (when given a
> mempool bioset) could not handle larger io_vecs.
> 
> No driver uses bio_clone_bioset() any more, they all
> use bio_clone_fast() if anything, and bio_clone_fast()
> doesn't clone the bi_io_vec.

Hmm.  From Jens tree:

drivers/lightnvm/pblk-read.c:   int_bio = bio_clone_bioset(bio, 
GFP_KERNEL, fs_bio_set);
drivers/md/raid1.c: mbio = 
bio_clone_bioset_partial(bio, GFP_NOIO,
drivers/md/raid1.c: mbio = 
bio_clone_bioset_partial(bio, GFP_NOIO,

I did not see your series touching either of those.  They probably
don't matter for the code removed as the bios are controlled by the
drivers, but the changelog still seems odd.

[PATCH 11/11] block: don't check for BIO_MAX_PAGES in blk_bio_segment_split()

[NeilBrown]

blk_bio_segment_split() makes sure bios have no more than
BIO_MAX_PAGES entries in the bi_io_vec.
This was done because bio_clone_bioset() (when given a
mempool bioset) could not handle larger io_vecs.

No driver uses bio_clone_bioset() any more, they all
use bio_clone_fast() if anything, and bio_clone_fast()
doesn't clone the bi_io_vec.

The main user of of bio_clone_bioset() at this level
is bounce.c, and bouncing now happens before blk_bio_segment_split(),
so that is not of concern.

So remove the big helpful comment and the code.

Signed-off-by: NeilBrown 
---
 block/blk-merge.c |   16 
 1 file changed, 16 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index e7862e9dcc39..cea544ec5d96 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -108,25 +108,9 @@ static struct bio *blk_bio_segment_split(struct 
request_queue *q,
bool do_split = true;
struct bio *new = NULL;
const unsigned max_sectors = get_max_io_size(q, bio);
-   unsigned bvecs = 0;
 
bio_for_each_segment(bv, bio, iter) {
/*
-* With arbitrary bio size, the incoming bio may be very
-* big. We have to split the bio into small bios so that
-* each holds at most BIO_MAX_PAGES bvecs because
-* bio_clone_bioset() can fail to allocate big bvecs.
-*
-* Those drivers which will need to use bio_clone_bioset()
-* should tell us in some way.  For now, impose the
-* BIO_MAX_PAGES limit on all queues.
-*
-* TODO: handle users of bio_clone_bioset() differently.
-*/
-   if (bvecs++ >= BIO_MAX_PAGES)
-   goto split;
-
-   /*
 * If the queue doesn't support SG gaps and adding this
 * offset would create a gap, disallow it.
 */