Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1371830 > unrolled thread

[PATCH] block: make sure big bio is splitted into at most 256 bvecs

Started byMing Lei <ming.lei@canonical.com>
First post2016-04-05 19:50 +0200
Last post2016-04-08 02:30 +0200
Articles 20 on this page of 33 — 4 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-05 19:50 +0200
    Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Shaohua Li <shli@fb.com> - 2016-04-05 20:30 +0200
      Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 02:30 +0200
        Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 02:40 +0200
          Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 02:50 +0200
            Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 03:10 +0200
            Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Shaohua Li <shli@fb.com> - 2016-04-06 03:10 +0200
          Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Shaohua Li <shli@fb.com> - 2016-04-06 02:50 +0200
        Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Shaohua Li <shli@fb.com> - 2016-04-06 02:40 +0200
      Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 02:50 +0200
        Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Shaohua Li <shli@fb.com> - 2016-04-06 03:10 +0200
          Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 03:20 +0200
    Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 02:40 +0200
      Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 03:00 +0200
        Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 03:20 +0200
          Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 03:30 +0200
            Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 03:30 +0200
              Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 04:00 +0200
                Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 04:30 +0200
                  Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 04:40 +0200
                    Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 04:50 +0200
                      Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 05:00 +0200
                      Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 05:00 +0200
                  Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Kent Overstreet <kent.overstreet@gmail.com> - 2016-04-06 04:40 +0200
                  Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 04:40 +0200
    Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-06 03:10 +0200
      Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-07 03:50 +0200
    Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-07 03:40 +0200
      Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-07 03:50 +0200
        Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-07 04:00 +0200
          Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-07 04:20 +0200
            Re: [PATCH] block: make sure big bio is splitted into at most 256  bvecs Eric Wheeler <bcache@lists.ewheeler.net> - 2016-04-08 01:30 +0200
              Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs Ming Lei <ming.lei@canonical.com> - 2016-04-08 02:30 +0200

Page 1 of 2  [1] 2  Next page →


#1371830 — [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromMing Lei <ming.lei@canonical.com>
Date2016-04-05 19:50 +0200
Subject[PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkDh0-7Hy-13@gated-at.bofh.it>
After arbitrary bio size is supported, 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 for safety reason, such
as bio_clone().

This patch fixes the following kernel crash:

> [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000028
> [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
> [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
> [  172.660399] Oops: 0000 [#1] SMP
> [...]
> [  172.664780] Call Trace:
> [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
> [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
> [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
> [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
> [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
> [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
> [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]

Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
Cc: stable@vger.kernel.org (4.2+)
Cc: Shaohua Li <shli@fb.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
I can reproduce the issue and verify the fix by the following approach:
	- create one raid1 over two virtio-blk 
	- build bcache device over the above raid1 and another cache device.
	- set cache mode as writeback
	- run random write over ext4 on the bcache device
	- then the crash can be triggered

 block/blk-merge.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 2613531..9a8651f 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -79,6 +79,18 @@ static inline unsigned get_max_io_size(struct request_queue *q,
 	/* aligned to logical block size */
 	sectors &= ~(mask >> 9);
 
+	/*
+	 * 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 for safety reason, such as
+	 * bio_clone().
+	 *
+	 * In the future, the limit might be converted into per-queue
+	 * flag.
+	 */
+	sectors = min_t(unsigned, sectors, BIO_MAX_PAGES <<
+			(PAGE_CACHE_SHIFT - 9));
+
 	return sectors;
 }
 
-- 
1.9.1

[toc] | [next] | [standalone]


#1371877 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromShaohua Li <shli@fb.com>
Date2016-04-05 20:30 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkDTJ-8d1-39@gated-at.bofh.it>
In reply to#1371830
On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> After arbitrary bio size is supported, 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 for safety reason, such
> as bio_clone().
> 
> This patch fixes the following kernel crash:
> 
> > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000028
> > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
> > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
> > [  172.660399] Oops: 0000 [#1] SMP
> > [...]
> > [  172.664780] Call Trace:
> > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
> > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
> > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
> > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
> > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
> > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
> > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
> 
> Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)

this bug is introduced by d2be537c3ba
> Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
> Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
> Cc: stable@vger.kernel.org (4.2+)
> Cc: Shaohua Li <shli@fb.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> I can reproduce the issue and verify the fix by the following approach:
> 	- create one raid1 over two virtio-blk 
> 	- build bcache device over the above raid1 and another cache device.
> 	- set cache mode as writeback
> 	- run random write over ext4 on the bcache device
> 	- then the crash can be triggered

can you explain why this is better than my original patch?

[toc] | [prev] | [next] | [standalone]


#1372091 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 02:30 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJw5-3LU-3@gated-at.bofh.it>
In reply to#1371877
On Tue, Apr 05, 2016 at 11:27:21AM -0700, Shaohua Li wrote:
> On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> > After arbitrary bio size is supported, 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 for safety reason, such
> > as bio_clone().
> > 
> > This patch fixes the following kernel crash:
> > 
> > > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
> > > 0000000000000028
> > > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
> > > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
> > > [  172.660399] Oops: 0000 [#1] SMP
> > > [...]
> > > [  172.664780] Call Trace:
> > > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
> > > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
> > > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
> > > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
> > > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
> > > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
> > > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
> > 
> > Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
> 
> this bug is introduced by d2be537c3ba
> > Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
> > Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
> > Cc: stable@vger.kernel.org (4.2+)
> > Cc: Shaohua Li <shli@fb.com>
> > Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > ---
> > I can reproduce the issue and verify the fix by the following approach:
> > 	- create one raid1 over two virtio-blk 
> > 	- build bcache device over the above raid1 and another cache device.
> > 	- set cache mode as writeback
> > 	- run random write over ext4 on the bcache device
> > 	- then the crash can be triggered
> 
> can you explain why this is better than my original patch?

Shaohua, what was your original patch? I'm sorry, I know I saw it at one point
but I can't remember what it was.

I didn't see Jeff's patch that introduced this bug until your email just now.
Argh.

Jeff, "block: bump BLK_DEF_MAX_SECTORS to 2560" doesn't make much sense as far as
I can tell without changing the BIO_MAX_PAGES too - that's probably why you
weren't seeing much performance increase from that patch...

But BLK_DEF_MAX_SECTORS should not have been enforcing the BIO_MAX_PAGES limit
so that patch was not at fault.

[toc] | [prev] | [next] | [standalone]


#1372097 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 02:40 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJFM-3PD-15@gated-at.bofh.it>
In reply to#1372091
On Tue, Apr 05, 2016 at 05:30:07PM -0700, Shaohua Li wrote:
> this one:
> http://marc.info/?l=linux-kernel&m=145926976808760&w=2

Ah. that patch won't actually fix the bug, since md isn't using
blk_default_limits, it's using blk_set_stacking_limits().

[toc] | [prev] | [next] | [standalone]


#1372102 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 02:50 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJPs-3T2-3@gated-at.bofh.it>
In reply to#1372097
On Tue, Apr 05, 2016 at 05:41:47PM -0700, Shaohua Li wrote:
> On Tue, Apr 05, 2016 at 04:36:04PM -0800, Kent Overstreet wrote:
> > On Tue, Apr 05, 2016 at 05:30:07PM -0700, Shaohua Li wrote:
> > > this one:
> > > http://marc.info/?l=linux-kernel&m=145926976808760&w=2
> > 
> > Ah. that patch won't actually fix the bug, since md isn't using
> > blk_default_limits, it's using blk_set_stacking_limits().
> 
> Not really, the limit is set by under layer disk not md, otherwise it
> should be BLK_SAFE_MAX_SECTORS, but the reported bio has 2560 sectors.
> blk_set_stacking_limits() will use it.

What? Well, that could should just be deleted, there's no reason anymore for md
to care about the queue limits of the devices underneath it.

Regardless, using BLK_DEF_MAX_SECTORS to limit # of pages in the biovec is
_crazy_. Why would you even do that? We have a separate field in queue limits
for # max segments, use it.

[toc] | [prev] | [next] | [standalone]


#1372116

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 03:10 +0200
Message-ID<rkK8O-4jp-7@gated-at.bofh.it>
In reply to#1372102
On Wed, Apr 6, 2016 at 8:59 AM, Shaohua Li <shli@fb.com> wrote:
> On Tue, Apr 05, 2016 at 04:45:55PM -0800, Kent Overstreet wrote:
>> On Tue, Apr 05, 2016 at 05:41:47PM -0700, Shaohua Li wrote:
>> > On Tue, Apr 05, 2016 at 04:36:04PM -0800, Kent Overstreet wrote:
>> > > On Tue, Apr 05, 2016 at 05:30:07PM -0700, Shaohua Li wrote:
>> > > > this one:
>> > > > http://marc.info/?l=linux-kernel&m=145926976808760&w=2
>> > >
>> > > Ah. that patch won't actually fix the bug, since md isn't using
>> > > blk_default_limits, it's using blk_set_stacking_limits().
>> >
>> > Not really, the limit is set by under layer disk not md, otherwise it
>> > should be BLK_SAFE_MAX_SECTORS, but the reported bio has 2560 sectors.
>> > blk_set_stacking_limits() will use it.
>>
>> What? Well, that could should just be deleted, there's no reason anymore for md
>> to care about the queue limits of the devices underneath it.
>>
>> Regardless, using BLK_DEF_MAX_SECTORS to limit # of pages in the biovec is
>> _crazy_. Why would you even do that? We have a separate field in queue limits
>> for # max segments, use it.
>
> We don't limit the max segments in blk_queue_max_segments(), but we can
> add. On the other hand, limit max segments to 256 could be a problem,
> because bvec page isn't always 4k, this might make some bio smaller.

It is nothing with max segments limit, it is about max sectors limit:
think about one big bio which includes 2Mbytes, then 512 bvecs are required,
but the 2M buffer can be continuous physically.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


#1372117 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromShaohua Li <shli@fb.com>
Date2016-04-06 03:10 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkK8O-4jp-9@gated-at.bofh.it>
In reply to#1372102
On Tue, Apr 05, 2016 at 04:45:55PM -0800, Kent Overstreet wrote:
> On Tue, Apr 05, 2016 at 05:41:47PM -0700, Shaohua Li wrote:
> > On Tue, Apr 05, 2016 at 04:36:04PM -0800, Kent Overstreet wrote:
> > > On Tue, Apr 05, 2016 at 05:30:07PM -0700, Shaohua Li wrote:
> > > > this one:
> > > > http://marc.info/?l=linux-kernel&m=145926976808760&w=2
> > > 
> > > Ah. that patch won't actually fix the bug, since md isn't using
> > > blk_default_limits, it's using blk_set_stacking_limits().
> > 
> > Not really, the limit is set by under layer disk not md, otherwise it
> > should be BLK_SAFE_MAX_SECTORS, but the reported bio has 2560 sectors.
> > blk_set_stacking_limits() will use it.
> 
> What? Well, that could should just be deleted, there's no reason anymore for md
> to care about the queue limits of the devices underneath it.
> 
> Regardless, using BLK_DEF_MAX_SECTORS to limit # of pages in the biovec is
> _crazy_. Why would you even do that? We have a separate field in queue limits
> for # max segments, use it.

We don't limit the max segments in blk_queue_max_segments(), but we can
add. On the other hand, limit max segments to 256 could be a problem,
because bvec page isn't always 4k, this might make some bio smaller.

[toc] | [prev] | [next] | [standalone]


#1372104 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromShaohua Li <shli@fb.com>
Date2016-04-06 02:50 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJPs-3T2-5@gated-at.bofh.it>
In reply to#1372097
On Tue, Apr 05, 2016 at 04:36:04PM -0800, Kent Overstreet wrote:
> On Tue, Apr 05, 2016 at 05:30:07PM -0700, Shaohua Li wrote:
> > this one:
> > http://marc.info/?l=linux-kernel&m=145926976808760&w=2
> 
> Ah. that patch won't actually fix the bug, since md isn't using
> blk_default_limits, it's using blk_set_stacking_limits().

Not really, the limit is set by under layer disk not md, otherwise it
should be BLK_SAFE_MAX_SECTORS, but the reported bio has 2560 sectors.
blk_set_stacking_limits() will use it.

[toc] | [prev] | [next] | [standalone]


#1372100 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromShaohua Li <shli@fb.com>
Date2016-04-06 02:40 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJFM-3PD-17@gated-at.bofh.it>
In reply to#1372091
On Tue, Apr 05, 2016 at 04:27:33PM -0800, Kent Overstreet wrote:
> On Tue, Apr 05, 2016 at 11:27:21AM -0700, Shaohua Li wrote:
> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> > > After arbitrary bio size is supported, 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 for safety reason, such
> > > as bio_clone().
> > > 
> > > This patch fixes the following kernel crash:
> > > 
> > > > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
> > > > 0000000000000028
> > > > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
> > > > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
> > > > [  172.660399] Oops: 0000 [#1] SMP
> > > > [...]
> > > > [  172.664780] Call Trace:
> > > > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
> > > > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
> > > > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
> > > > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
> > > > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
> > > > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
> > > > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
> > > 
> > > Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
> > 
> > this bug is introduced by d2be537c3ba
> > > Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
> > > Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
> > > Cc: stable@vger.kernel.org (4.2+)
> > > Cc: Shaohua Li <shli@fb.com>
> > > Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > > ---
> > > I can reproduce the issue and verify the fix by the following approach:
> > > 	- create one raid1 over two virtio-blk 
> > > 	- build bcache device over the above raid1 and another cache device.
> > > 	- set cache mode as writeback
> > > 	- run random write over ext4 on the bcache device
> > > 	- then the crash can be triggered
> > 
> > can you explain why this is better than my original patch?
> 
> Shaohua, what was your original patch? I'm sorry, I know I saw it at one point
> but I can't remember what it was.

this one:
http://marc.info/?l=linux-kernel&m=145926976808760&w=2
 

[toc] | [prev] | [next] | [standalone]


#1372103

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 02:50 +0200
Message-ID<rkJPs-3T2-7@gated-at.bofh.it>
In reply to#1371877
On Wed, Apr 6, 2016 at 2:27 AM, Shaohua Li <shli@fb.com> wrote:
> On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
>> After arbitrary bio size is supported, 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 for safety reason, such
>> as bio_clone().
>>
>> This patch fixes the following kernel crash:
>>
>> > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
>> > 0000000000000028
>> > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
>> > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
>> > [  172.660399] Oops: 0000 [#1] SMP
>> > [...]
>> > [  172.664780] Call Trace:
>> > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
>> > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
>> > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
>> > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
>> > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
>> > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
>> > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
>>
>> Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
>
> this bug is introduced by d2be537c3ba
>> Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
>> Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
>> Cc: stable@vger.kernel.org (4.2+)
>> Cc: Shaohua Li <shli@fb.com>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>> I can reproduce the issue and verify the fix by the following approach:
>>       - create one raid1 over two virtio-blk
>>       - build bcache device over the above raid1 and another cache device.
>>       - set cache mode as writeback
>>       - run random write over ext4 on the bcache device
>>       - then the crash can be triggered
>
> can you explain why this is better than my original patch?

Shaohua, your patch is wrong, please see the following link:

    https://lkml.org/lkml/2016/3/30/893

Thanks,

> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


#1372115 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromShaohua Li <shli@fb.com>
Date2016-04-06 03:10 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkK8O-4jp-5@gated-at.bofh.it>
In reply to#1372103
On Wed, Apr 06, 2016 at 08:47:56AM +0800, Ming Lei wrote:
> On Wed, Apr 6, 2016 at 2:27 AM, Shaohua Li <shli@fb.com> wrote:
> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> >> After arbitrary bio size is supported, 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 for safety reason, such
> >> as bio_clone().
> >>
> >> This patch fixes the following kernel crash:
> >>
> >> > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
> >> > 0000000000000028
> >> > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
> >> > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
> >> > [  172.660399] Oops: 0000 [#1] SMP
> >> > [...]
> >> > [  172.664780] Call Trace:
> >> > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
> >> > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
> >> > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
> >> > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
> >> > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
> >> > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
> >> > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
> >>
> >> Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
> >
> > this bug is introduced by d2be537c3ba
> >> Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
> >> Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
> >> Cc: stable@vger.kernel.org (4.2+)
> >> Cc: Shaohua Li <shli@fb.com>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >> I can reproduce the issue and verify the fix by the following approach:
> >>       - create one raid1 over two virtio-blk
> >>       - build bcache device over the above raid1 and another cache device.
> >>       - set cache mode as writeback
> >>       - run random write over ext4 on the bcache device
> >>       - then the crash can be triggered
> >
> > can you explain why this is better than my original patch?
> 
> Shaohua, your patch is wrong, please see the following link:
> 
>     https://lkml.org/lkml/2016/3/30/893

I don't see why, really, except it declares you are right :)

why it's 2560 instead of 2048?

[toc] | [prev] | [next] | [standalone]


#1372121

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 03:20 +0200
Message-ID<rkKiu-4nJ-7@gated-at.bofh.it>
In reply to#1372115
On Wed, Apr 6, 2016 at 9:04 AM, Shaohua Li <shli@fb.com> wrote:
> On Wed, Apr 06, 2016 at 08:47:56AM +0800, Ming Lei wrote:
>> On Wed, Apr 6, 2016 at 2:27 AM, Shaohua Li <shli@fb.com> wrote:
>> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
>> >> After arbitrary bio size is supported, 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 for safety reason, such
>> >> as bio_clone().
>> >>
>> >> This patch fixes the following kernel crash:
>> >>
>> >> > [  172.660142] BUG: unable to handle kernel NULL pointer dereference at
>> >> > 0000000000000028
>> >> > [  172.660229] IP: [<ffffffff811e53b4>] bio_trim+0xf/0x2a
>> >> > [  172.660289] PGD 7faf3e067 PUD 7f9279067 PMD 0
>> >> > [  172.660399] Oops: 0000 [#1] SMP
>> >> > [...]
>> >> > [  172.664780] Call Trace:
>> >> > [  172.664813]  [<ffffffffa007f3be>] ? raid1_make_request+0x2e8/0xad7 [raid1]
>> >> > [  172.664846]  [<ffffffff811f07da>] ? blk_queue_split+0x377/0x3d4
>> >> > [  172.664880]  [<ffffffffa005fb5f>] ? md_make_request+0xf6/0x1e9 [md_mod]
>> >> > [  172.664912]  [<ffffffff811eb860>] ? generic_make_request+0xb5/0x155
>> >> > [  172.664947]  [<ffffffffa0445c89>] ? prio_io+0x85/0x95 [bcache]
>> >> > [  172.664981]  [<ffffffffa0448252>] ? register_cache_set+0x355/0x8d0 [bcache]
>> >> > [  172.665016]  [<ffffffffa04497d3>] ? register_bcache+0x1006/0x1174 [bcache]
>> >>
>> >> Fixes: 54efd50(block: make generic_make_request handle arbitrarily sized bios)
>> >
>> > this bug is introduced by d2be537c3ba
>> >> Reported-by: Sebastian Roesner <sroesner-kernelorg@roesner-online.de>
>> >> Reported-by: Eric Wheeler <bcache@lists.ewheeler.net>
>> >> Cc: stable@vger.kernel.org (4.2+)
>> >> Cc: Shaohua Li <shli@fb.com>
>> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> >> ---
>> >> I can reproduce the issue and verify the fix by the following approach:
>> >>       - create one raid1 over two virtio-blk
>> >>       - build bcache device over the above raid1 and another cache device.
>> >>       - set cache mode as writeback
>> >>       - run random write over ext4 on the bcache device
>> >>       - then the crash can be triggered
>> >
>> > can you explain why this is better than my original patch?
>>
>> Shaohua, your patch is wrong, please see the following link:
>>
>>     https://lkml.org/lkml/2016/3/30/893
>
> I don't see why, really, except it declares you are right :)

Never mind, I post it again, and maybe cause my poor english, :-)

blk_rq_get_max_sectors() which uses max sectors limit is used for
merging bios/reqs, and that means limits.max_sectors is for limitting
max sectors in one request or transfer. One request may include lots of
bios. Now this patch decreases the limit just for single bio's 256
bvec's limitation.
Is that correct? That is the reason why I suggest to change get_max_io_size()
for bio's 256 bvecs limit.

On the contrary, the default max sectors should have been increased
since hardware is becoming quicker, and we should send more to drive
in one request, IMO.

>
> why it's 2560 instead of 2048?

I don't know the exact reason why Jeff takes 2560, but I feel it can be
bigger because the hardware is becoming quicker.

Thanks,
Ming

> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


#1372098 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 02:40 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkJFM-3PD-11@gated-at.bofh.it>
In reply to#1371830
On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> After arbitrary bio size is supported, 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 for safety reason, such
> as bio_clone().
> 
> This patch fixes the following kernel crash:

Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
instead, md has its own queue limits that it ought to be setting up correctly.

[toc] | [prev] | [next] | [standalone]


#1372106

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 03:00 +0200
Message-ID<rkJZ7-3Wx-1@gated-at.bofh.it>
In reply to#1372098
On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
>> After arbitrary bio size is supported, 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 for safety reason, such
>> as bio_clone().
>>
>> This patch fixes the following kernel crash:
>
> Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
> instead, md has its own queue limits that it ought to be setting up correctly.

Except for md, there are also several usages of bio_clone:

         - drbd
         - osdblk
         - pktcdvd
         - xen-blkfront
         - verify code of bcache

I don't like bio_clone() too, which can cause trouble to multipage bvecs.

How about fixing the issue by this simple patch first? Then once we limits
all above queues by max sectors, the global limit can be removed as
mentioned by the comment.

thanks,
Ming

> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


#1372118 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 03:20 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkKit-4nJ-1@gated-at.bofh.it>
In reply to#1372106
On Wed, Apr 06, 2016 at 08:59:31AM +0800, Ming Lei wrote:
> On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
> <kent.overstreet@gmail.com> wrote:
> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> >> After arbitrary bio size is supported, 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 for safety reason, such
> >> as bio_clone().
> >>
> >> This patch fixes the following kernel crash:
> >
> > Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
> > instead, md has its own queue limits that it ought to be setting up correctly.
> 
> Except for md, there are also several usages of bio_clone:
> 
>          - drbd
>          - osdblk
>          - pktcdvd
>          - xen-blkfront
>          - verify code of bcache
> 
> I don't like bio_clone() too, which can cause trouble to multipage bvecs.
> 
> How about fixing the issue by this simple patch first? Then once we limits
> all above queues by max sectors, the global limit can be removed as
> mentioned by the comment.

just do this:

void blk_set_limit_clonable(struct queue_limits *lim)
{
	lim->max_segments = min(lim->max_segments, BIO_MAX_PAGES);
}

and then call that from the appropriate drivers. It should be like 20 minutes of
work.

My issue is that your approach of just enforcing a global limit is a step in the
wrong direction - we want to get _away_ from that and move towards drivers
specifying _directly_ what their limits are: more straightforward, less opaque.

Also, your patch is wrong, as it'll break if there's bvecs that aren't full
pages.

[toc] | [prev] | [next] | [standalone]


#1372123

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 03:30 +0200
Message-ID<rkKsa-4u3-3@gated-at.bofh.it>
In reply to#1372118
On Wed, Apr 6, 2016 at 9:10 AM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 08:59:31AM +0800, Ming Lei wrote:
>> On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
>> <kent.overstreet@gmail.com> wrote:
>> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
>> >> After arbitrary bio size is supported, 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 for safety reason, such
>> >> as bio_clone().
>> >>
>> >> This patch fixes the following kernel crash:
>> >
>> > Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
>> > instead, md has its own queue limits that it ought to be setting up correctly.
>>
>> Except for md, there are also several usages of bio_clone:
>>
>>          - drbd
>>          - osdblk
>>          - pktcdvd
>>          - xen-blkfront
>>          - verify code of bcache
>>
>> I don't like bio_clone() too, which can cause trouble to multipage bvecs.
>>
>> How about fixing the issue by this simple patch first? Then once we limits
>> all above queues by max sectors, the global limit can be removed as
>> mentioned by the comment.
>
> just do this:
>
> void blk_set_limit_clonable(struct queue_limits *lim)
> {
>         lim->max_segments = min(lim->max_segments, BIO_MAX_PAGES);
> }

As I memtioned it is __not__ correct to use max_segments, and the issue is
related with max sectors, please see the code of bio_clone_bioset():

      bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);

bio_segments() returns pages actually.

>
> and then call that from the appropriate drivers. It should be like 20 minutes of
> work.
>
> My issue is that your approach of just enforcing a global limit is a step in the
> wrong direction - we want to get _away_ from that and move towards drivers
> specifying _directly_ what their limits are: more straightforward, less opaque.
>
> Also, your patch is wrong, as it'll break if there's bvecs that aren't full
> pages.

I don't understand why my patch is wrong, since we can split anywhere
in a bio, could you explain it a bit?

Thanks,
Ming

> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


#1372126 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 03:30 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkKsa-4u3-7@gated-at.bofh.it>
In reply to#1372123
On Wed, Apr 06, 2016 at 09:20:59AM +0800, Ming Lei wrote:
> On Wed, Apr 6, 2016 at 9:10 AM, Kent Overstreet
> <kent.overstreet@gmail.com> wrote:
> > On Wed, Apr 06, 2016 at 08:59:31AM +0800, Ming Lei wrote:
> >> On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
> >> <kent.overstreet@gmail.com> wrote:
> >> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> >> >> After arbitrary bio size is supported, 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 for safety reason, such
> >> >> as bio_clone().
> >> >>
> >> >> This patch fixes the following kernel crash:
> >> >
> >> > Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
> >> > instead, md has its own queue limits that it ought to be setting up correctly.
> >>
> >> Except for md, there are also several usages of bio_clone:
> >>
> >>          - drbd
> >>          - osdblk
> >>          - pktcdvd
> >>          - xen-blkfront
> >>          - verify code of bcache
> >>
> >> I don't like bio_clone() too, which can cause trouble to multipage bvecs.
> >>
> >> How about fixing the issue by this simple patch first? Then once we limits
> >> all above queues by max sectors, the global limit can be removed as
> >> mentioned by the comment.
> >
> > just do this:
> >
> > void blk_set_limit_clonable(struct queue_limits *lim)
> > {
> >         lim->max_segments = min(lim->max_segments, BIO_MAX_PAGES);
> > }
> 
> As I memtioned it is __not__ correct to use max_segments, and the issue is
> related with max sectors, please see the code of bio_clone_bioset():

I know how bio_clone_bioset() works but I'm not seeing how that has anything to
do with max sectors. The way it copies the biovec is not going to merge
segments, if the original bio had non full page segments then so is the clone.

>       bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
> 
> bio_segments() returns pages actually.
> 
> >
> > and then call that from the appropriate drivers. It should be like 20 minutes of
> > work.
> >
> > My issue is that your approach of just enforcing a global limit is a step in the
> > wrong direction - we want to get _away_ from that and move towards drivers
> > specifying _directly_ what their limits are: more straightforward, less opaque.
> >
> > Also, your patch is wrong, as it'll break if there's bvecs that aren't full
> > pages.
> 
> I don't understand why my patch is wrong, since we can split anywhere
> in a bio, could you explain it a bit?

If you have a bio that has > BIO_MAX_PAGES segments, but all the segments are a
single sector (not a full page!) - then think about what'll happen...

It can happen with userspace issuing direct IOs

[toc] | [prev] | [next] | [standalone]


#1372131

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 04:00 +0200
Message-ID<rkKVb-4Hp-1@gated-at.bofh.it>
In reply to#1372126
On Wed, Apr 6, 2016 at 9:28 AM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 09:20:59AM +0800, Ming Lei wrote:
>> On Wed, Apr 6, 2016 at 9:10 AM, Kent Overstreet
>> <kent.overstreet@gmail.com> wrote:
>> > On Wed, Apr 06, 2016 at 08:59:31AM +0800, Ming Lei wrote:
>> >> On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
>> >> <kent.overstreet@gmail.com> wrote:
>> >> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
>> >> >> After arbitrary bio size is supported, 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 for safety reason, such
>> >> >> as bio_clone().
>> >> >>
>> >> >> This patch fixes the following kernel crash:
>> >> >
>> >> > Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
>> >> > instead, md has its own queue limits that it ought to be setting up correctly.
>> >>
>> >> Except for md, there are also several usages of bio_clone:
>> >>
>> >>          - drbd
>> >>          - osdblk
>> >>          - pktcdvd
>> >>          - xen-blkfront
>> >>          - verify code of bcache
>> >>
>> >> I don't like bio_clone() too, which can cause trouble to multipage bvecs.
>> >>
>> >> How about fixing the issue by this simple patch first? Then once we limits
>> >> all above queues by max sectors, the global limit can be removed as
>> >> mentioned by the comment.
>> >
>> > just do this:
>> >
>> > void blk_set_limit_clonable(struct queue_limits *lim)
>> > {
>> >         lim->max_segments = min(lim->max_segments, BIO_MAX_PAGES);
>> > }
>>
>> As I memtioned it is __not__ correct to use max_segments, and the issue is
>> related with max sectors, please see the code of bio_clone_bioset():
>
> I know how bio_clone_bioset() works but I'm not seeing how that has anything to
> do with max sectors. The way it copies the biovec is not going to merge
> segments, if the original bio had non full page segments then so is the clone.

OK, I see, now it is a totally new limit, and no current queue limit can fit
the purpose.

Looks we need to introduce the new limit of io_max_vecs, which can be
applied into blk_bio_segment_split().

But a queue flag should be better than queue limit since it is a 'limit' from
software/driver.

>
>>       bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
>>
>> bio_segments() returns pages actually.
>>
>> >
>> > and then call that from the appropriate drivers. It should be like 20 minutes of
>> > work.
>> >
>> > My issue is that your approach of just enforcing a global limit is a step in the
>> > wrong direction - we want to get _away_ from that and move towards drivers
>> > specifying _directly_ what their limits are: more straightforward, less opaque.
>> >
>> > Also, your patch is wrong, as it'll break if there's bvecs that aren't full
>> > pages.
>>
>> I don't understand why my patch is wrong, since we can split anywhere
>> in a bio, could you explain it a bit?
>
> If you have a bio that has > BIO_MAX_PAGES segments, but all the segments are a
> single sector (not a full page!) - then think about what'll happen...
>
> It can happen with userspace issuing direct IOs

Yeah, I will cook a patch for review.

Thanks,
Ming

[toc] | [prev] | [next] | [standalone]


#1372147 — Re: [PATCH] block: make sure big bio is splitted into at most 256 bvecs

FromKent Overstreet <kent.overstreet@gmail.com>
Date2016-04-06 04:30 +0200
SubjectRe: [PATCH] block: make sure big bio is splitted into at most 256 bvecs
Message-ID<rkLoe-5lE-5@gated-at.bofh.it>
In reply to#1372131
On Wed, Apr 06, 2016 at 09:51:02AM +0800, Ming Lei wrote:
> On Wed, Apr 6, 2016 at 9:28 AM, Kent Overstreet
> <kent.overstreet@gmail.com> wrote:
> > On Wed, Apr 06, 2016 at 09:20:59AM +0800, Ming Lei wrote:
> >> On Wed, Apr 6, 2016 at 9:10 AM, Kent Overstreet
> >> <kent.overstreet@gmail.com> wrote:
> >> > On Wed, Apr 06, 2016 at 08:59:31AM +0800, Ming Lei wrote:
> >> >> On Wed, Apr 6, 2016 at 8:30 AM, Kent Overstreet
> >> >> <kent.overstreet@gmail.com> wrote:
> >> >> > On Wed, Apr 06, 2016 at 01:44:06AM +0800, Ming Lei wrote:
> >> >> >> After arbitrary bio size is supported, 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 for safety reason, such
> >> >> >> as bio_clone().
> >> >> >>
> >> >> >> This patch fixes the following kernel crash:
> >> >> >
> >> >> > Ming, let's not do it this way; drivers that don't clone biovecs are the norm -
> >> >> > instead, md has its own queue limits that it ought to be setting up correctly.
> >> >>
> >> >> Except for md, there are also several usages of bio_clone:
> >> >>
> >> >>          - drbd
> >> >>          - osdblk
> >> >>          - pktcdvd
> >> >>          - xen-blkfront
> >> >>          - verify code of bcache
> >> >>
> >> >> I don't like bio_clone() too, which can cause trouble to multipage bvecs.
> >> >>
> >> >> How about fixing the issue by this simple patch first? Then once we limits
> >> >> all above queues by max sectors, the global limit can be removed as
> >> >> mentioned by the comment.
> >> >
> >> > just do this:
> >> >
> >> > void blk_set_limit_clonable(struct queue_limits *lim)
> >> > {
> >> >         lim->max_segments = min(lim->max_segments, BIO_MAX_PAGES);
> >> > }
> >>
> >> As I memtioned it is __not__ correct to use max_segments, and the issue is
> >> related with max sectors, please see the code of bio_clone_bioset():
> >
> > I know how bio_clone_bioset() works but I'm not seeing how that has anything to
> > do with max sectors. The way it copies the biovec is not going to merge
> > segments, if the original bio had non full page segments then so is the clone.
> 
> OK, I see, now it is a totally new limit, and no current queue limit can fit
> the purpose.
> 
> Looks we need to introduce the new limit of io_max_vecs, which can be
> applied into blk_bio_segment_split().
> 
> But a queue flag should be better than queue limit since it is a 'limit' from
> software/driver.

Why is max_segments not appropriate?

[toc] | [prev] | [next] | [standalone]


#1372148

FromMing Lei <ming.lei@canonical.com>
Date2016-04-06 04:40 +0200
Message-ID<rkLxU-5qB-3@gated-at.bofh.it>
In reply to#1372147
On Wed, Apr 6, 2016 at 10:34 AM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 10:30:22AM +0800, Ming Lei wrote:
>> Now limit.max_segments is for limiting segments from hw view, one this
>> segment may include lots of pages/bvecs.
>>
>> The current bio_clone() issue is that we can't clone from one bio which
>> includes more than 256 bvecs, maybe all these 256 bvecs belong to
>> one same hw segment.
>
> So the distinction is purely a post multipage bvec thing?

Even after multipage bvec is applied, the limit for max bvecs is still needed
for some cases like bio bounce, in which bio_clone() need to
clone single page bvec.

Thanks,


> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]


Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel


csiph-web