All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] block: setup bounce bio_sets properly
@ 2018-10-18 21:24 Jens Axboe
  2018-10-19  9:10 ` Ming Lei
  0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2018-10-18 21:24 UTC (permalink / raw)
  To: linux-block; +Cc: Ondrej Zary

We're only setting up the bounce bio sets if we happen
to need bouncing for regular HIGHMEM, not if we only need
it for ISA devices.

Reported-by: Ondrej Zary <linux@rainbow-software.org>
Tested-by: Ondrej Zary <linux@rainbow-software.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/bounce.c b/block/bounce.c
index b30071ac4ec6..1356a2f4aae2 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -31,6 +31,24 @@
 static struct bio_set bounce_bio_set, bounce_bio_split;
 static mempool_t page_pool, isa_page_pool;
 
+static void init_bounce_bioset(void)
+{
+	static bool bounce_bs_setup;
+	int ret;
+
+	if (bounce_bs_setup)
+		return;
+
+	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
+	BUG_ON(ret);
+	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
+		BUG_ON(1);
+
+	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
+	BUG_ON(ret);
+	bounce_bs_setup = true;
+}
+
 #if defined(CONFIG_HIGHMEM)
 static __init int init_emergency_pool(void)
 {
@@ -44,14 +62,7 @@ static __init int init_emergency_pool(void)
 	BUG_ON(ret);
 	pr_info("pool size: %d pages\n", POOL_SIZE);
 
-	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
-	BUG_ON(ret);
-	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
-		BUG_ON(1);
-
-	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
-	BUG_ON(ret);
-
+	init_bounce_bioset();
 	return 0;
 }
 
@@ -102,6 +113,7 @@ int init_emergency_isa_pool(void)
 	BUG_ON(ret);
 
 	pr_info("isa pool size: %d pages\n", ISA_POOL_SIZE);
+	init_bounce_bioset();
 	return 0;
 }
 
-- 
Jens Axboe

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: setup bounce bio_sets properly
  2018-10-18 21:24 [PATCH] block: setup bounce bio_sets properly Jens Axboe
@ 2018-10-19  9:10 ` Ming Lei
  2018-10-19 16:35   ` Jens Axboe
  0 siblings, 1 reply; 5+ messages in thread
From: Ming Lei @ 2018-10-19  9:10 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Ondrej Zary

On Thu, Oct 18, 2018 at 03:24:47PM -0600, Jens Axboe wrote:
> We're only setting up the bounce bio sets if we happen
> to need bouncing for regular HIGHMEM, not if we only need
> it for ISA devices.
> 
> Reported-by: Ondrej Zary <linux@rainbow-software.org>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> diff --git a/block/bounce.c b/block/bounce.c
> index b30071ac4ec6..1356a2f4aae2 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -31,6 +31,24 @@
>  static struct bio_set bounce_bio_set, bounce_bio_split;
>  static mempool_t page_pool, isa_page_pool;
>  
> +static void init_bounce_bioset(void)
> +{
> +	static bool bounce_bs_setup;
> +	int ret;
> +
> +	if (bounce_bs_setup)
> +		return;
> +
> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> +	BUG_ON(ret);
> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
> +		BUG_ON(1);
> +
> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
> +	BUG_ON(ret);
> +	bounce_bs_setup = true;

When initcall is run from do_basic_setup(), all CPUs and scheduler have been up,
it is likely that init_emergency_pool() is run when one driver is calling
blk_queue_bounce_limit() to start init_emergency_isa_pool().

So the above code might be run twice.

Thanks,
Ming

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: setup bounce bio_sets properly
  2018-10-19  9:10 ` Ming Lei
@ 2018-10-19 16:35   ` Jens Axboe
  2018-10-21 12:17     ` Ondrej Zary
  0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2018-10-19 16:35 UTC (permalink / raw)
  To: Ming Lei; +Cc: linux-block, Ondrej Zary

On 10/19/18 3:10 AM, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 03:24:47PM -0600, Jens Axboe wrote:
>> We're only setting up the bounce bio sets if we happen
>> to need bouncing for regular HIGHMEM, not if we only need
>> it for ISA devices.
>>
>> Reported-by: Ondrej Zary <linux@rainbow-software.org>
>> Tested-by: Ondrej Zary <linux@rainbow-software.org>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> diff --git a/block/bounce.c b/block/bounce.c
>> index b30071ac4ec6..1356a2f4aae2 100644
>> --- a/block/bounce.c
>> +++ b/block/bounce.c
>> @@ -31,6 +31,24 @@
>>  static struct bio_set bounce_bio_set, bounce_bio_split;
>>  static mempool_t page_pool, isa_page_pool;
>>  
>> +static void init_bounce_bioset(void)
>> +{
>> +	static bool bounce_bs_setup;
>> +	int ret;
>> +
>> +	if (bounce_bs_setup)
>> +		return;
>> +
>> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
>> +	BUG_ON(ret);
>> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
>> +		BUG_ON(1);
>> +
>> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
>> +	BUG_ON(ret);
>> +	bounce_bs_setup = true;
> 
> When initcall is run from do_basic_setup(), all CPUs and scheduler have been up,
> it is likely that init_emergency_pool() is run when one driver is calling
> blk_queue_bounce_limit() to start init_emergency_isa_pool().
> 
> So the above code might be run twice.

Good point, it's actually not even safe right now with the mempool
setup. How about the below?


diff --git a/block/bounce.c b/block/bounce.c
index b30071ac4ec6..ec0d99995f5f 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -31,6 +31,24 @@
 static struct bio_set bounce_bio_set, bounce_bio_split;
 static mempool_t page_pool, isa_page_pool;
 
+static void init_bounce_bioset(void)
+{
+	static bool bounce_bs_setup;
+	int ret;
+
+	if (bounce_bs_setup)
+		return;
+
+	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
+	BUG_ON(ret);
+	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
+		BUG_ON(1);
+
+	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
+	BUG_ON(ret);
+	bounce_bs_setup = true;
+}
+
 #if defined(CONFIG_HIGHMEM)
 static __init int init_emergency_pool(void)
 {
@@ -44,14 +62,7 @@ static __init int init_emergency_pool(void)
 	BUG_ON(ret);
 	pr_info("pool size: %d pages\n", POOL_SIZE);
 
-	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
-	BUG_ON(ret);
-	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
-		BUG_ON(1);
-
-	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
-	BUG_ON(ret);
-
+	init_bounce_bioset();
 	return 0;
 }
 
@@ -86,6 +97,8 @@ static void *mempool_alloc_pages_isa(gfp_t gfp_mask, void *data)
 	return mempool_alloc_pages(gfp_mask | GFP_DMA, data);
 }
 
+static DEFINE_MUTEX(isa_mutex);
+
 /*
  * gets called "every" time someone init's a queue with BLK_BOUNCE_ISA
  * as the max address, so check if the pool has already been created.
@@ -94,14 +107,20 @@ int init_emergency_isa_pool(void)
 {
 	int ret;
 
-	if (mempool_initialized(&isa_page_pool))
+	mutex_lock(&isa_mutex);
+
+	if (mempool_initialized(&isa_page_pool)) {
+		mutex_unlock(&isa_mutex);
 		return 0;
+	}
 
 	ret = mempool_init(&isa_page_pool, ISA_POOL_SIZE, mempool_alloc_pages_isa,
 			   mempool_free_pages, (void *) 0);
 	BUG_ON(ret);
 
 	pr_info("isa pool size: %d pages\n", ISA_POOL_SIZE);
+	init_bounce_bioset();
+	mutex_unlock(&isa_mutex);
 	return 0;
 }
 

-- 
Jens Axboe

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: setup bounce bio_sets properly
  2018-10-19 16:35   ` Jens Axboe
@ 2018-10-21 12:17     ` Ondrej Zary
  2018-10-21 18:02       ` Jens Axboe
  0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Zary @ 2018-10-21 12:17 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Ming Lei, linux-block

On Friday 19 October 2018 18:35:00 Jens Axboe wrote:
> On 10/19/18 3:10 AM, Ming Lei wrote:
> > On Thu, Oct 18, 2018 at 03:24:47PM -0600, Jens Axboe wrote:
> >> We're only setting up the bounce bio sets if we happen
> >> to need bouncing for regular HIGHMEM, not if we only need
> >> it for ISA devices.
> >>
> >> Reported-by: Ondrej Zary <linux@rainbow-software.org>
> >> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>
> >> diff --git a/block/bounce.c b/block/bounce.c
> >> index b30071ac4ec6..1356a2f4aae2 100644
> >> --- a/block/bounce.c
> >> +++ b/block/bounce.c
> >> @@ -31,6 +31,24 @@
> >>  static struct bio_set bounce_bio_set, bounce_bio_split;
> >>  static mempool_t page_pool, isa_page_pool;
> >>  
> >> +static void init_bounce_bioset(void)
> >> +{
> >> +	static bool bounce_bs_setup;
> >> +	int ret;
> >> +
> >> +	if (bounce_bs_setup)
> >> +		return;
> >> +
> >> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> >> +	BUG_ON(ret);
> >> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
> >> +		BUG_ON(1);
> >> +
> >> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
> >> +	BUG_ON(ret);
> >> +	bounce_bs_setup = true;
> > 
> > When initcall is run from do_basic_setup(), all CPUs and scheduler have been up,
> > it is likely that init_emergency_pool() is run when one driver is calling
> > blk_queue_bounce_limit() to start init_emergency_isa_pool().
> > 
> > So the above code might be run twice.
> 
> Good point, it's actually not even safe right now with the mempool
> setup. How about the below?
> 
> 
> diff --git a/block/bounce.c b/block/bounce.c
> index b30071ac4ec6..ec0d99995f5f 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -31,6 +31,24 @@
>  static struct bio_set bounce_bio_set, bounce_bio_split;
>  static mempool_t page_pool, isa_page_pool;
>  
> +static void init_bounce_bioset(void)
> +{
> +	static bool bounce_bs_setup;
> +	int ret;
> +
> +	if (bounce_bs_setup)
> +		return;
> +
> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> +	BUG_ON(ret);
> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
> +		BUG_ON(1);
> +
> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
> +	BUG_ON(ret);
> +	bounce_bs_setup = true;
> +}
> +
>  #if defined(CONFIG_HIGHMEM)
>  static __init int init_emergency_pool(void)
>  {
> @@ -44,14 +62,7 @@ static __init int init_emergency_pool(void)
>  	BUG_ON(ret);
>  	pr_info("pool size: %d pages\n", POOL_SIZE);
>  
> -	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
> -	BUG_ON(ret);
> -	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
> -		BUG_ON(1);
> -
> -	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
> -	BUG_ON(ret);
> -
> +	init_bounce_bioset();
>  	return 0;
>  }
>  
> @@ -86,6 +97,8 @@ static void *mempool_alloc_pages_isa(gfp_t gfp_mask, void *data)
>  	return mempool_alloc_pages(gfp_mask | GFP_DMA, data);
>  }
>  
> +static DEFINE_MUTEX(isa_mutex);
> +
>  /*
>   * gets called "every" time someone init's a queue with BLK_BOUNCE_ISA
>   * as the max address, so check if the pool has already been created.
> @@ -94,14 +107,20 @@ int init_emergency_isa_pool(void)
>  {
>  	int ret;
>  
> -	if (mempool_initialized(&isa_page_pool))
> +	mutex_lock(&isa_mutex);
> +
> +	if (mempool_initialized(&isa_page_pool)) {
> +		mutex_unlock(&isa_mutex);
>  		return 0;
> +	}
>  
>  	ret = mempool_init(&isa_page_pool, ISA_POOL_SIZE, mempool_alloc_pages_isa,
>  			   mempool_free_pages, (void *) 0);
>  	BUG_ON(ret);
>  
>  	pr_info("isa pool size: %d pages\n", ISA_POOL_SIZE);
> +	init_bounce_bioset();
> +	mutex_unlock(&isa_mutex);
>  	return 0;
>  }
>  

Works for me.

-- 
Ondrej Zary

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] block: setup bounce bio_sets properly
  2018-10-21 12:17     ` Ondrej Zary
@ 2018-10-21 18:02       ` Jens Axboe
  0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2018-10-21 18:02 UTC (permalink / raw)
  To: Ondrej Zary; +Cc: Ming Lei, linux-block

On 10/21/18 6:17 AM, Ondrej Zary wrote:
> On Friday 19 October 2018 18:35:00 Jens Axboe wrote:
>> On 10/19/18 3:10 AM, Ming Lei wrote:
>>> On Thu, Oct 18, 2018 at 03:24:47PM -0600, Jens Axboe wrote:
>>>> We're only setting up the bounce bio sets if we happen
>>>> to need bouncing for regular HIGHMEM, not if we only need
>>>> it for ISA devices.
>>>>
>>>> Reported-by: Ondrej Zary <linux@rainbow-software.org>
>>>> Tested-by: Ondrej Zary <linux@rainbow-software.org>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> diff --git a/block/bounce.c b/block/bounce.c
>>>> index b30071ac4ec6..1356a2f4aae2 100644
>>>> --- a/block/bounce.c
>>>> +++ b/block/bounce.c
>>>> @@ -31,6 +31,24 @@
>>>>  static struct bio_set bounce_bio_set, bounce_bio_split;
>>>>  static mempool_t page_pool, isa_page_pool;
>>>>  
>>>> +static void init_bounce_bioset(void)
>>>> +{
>>>> +	static bool bounce_bs_setup;
>>>> +	int ret;
>>>> +
>>>> +	if (bounce_bs_setup)
>>>> +		return;
>>>> +
>>>> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
>>>> +	BUG_ON(ret);
>>>> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
>>>> +		BUG_ON(1);
>>>> +
>>>> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
>>>> +	BUG_ON(ret);
>>>> +	bounce_bs_setup = true;
>>>
>>> When initcall is run from do_basic_setup(), all CPUs and scheduler have been up,
>>> it is likely that init_emergency_pool() is run when one driver is calling
>>> blk_queue_bounce_limit() to start init_emergency_isa_pool().
>>>
>>> So the above code might be run twice.
>>
>> Good point, it's actually not even safe right now with the mempool
>> setup. How about the below?
>>
>>
>> diff --git a/block/bounce.c b/block/bounce.c
>> index b30071ac4ec6..ec0d99995f5f 100644
>> --- a/block/bounce.c
>> +++ b/block/bounce.c
>> @@ -31,6 +31,24 @@
>>  static struct bio_set bounce_bio_set, bounce_bio_split;
>>  static mempool_t page_pool, isa_page_pool;
>>  
>> +static void init_bounce_bioset(void)
>> +{
>> +	static bool bounce_bs_setup;
>> +	int ret;
>> +
>> +	if (bounce_bs_setup)
>> +		return;
>> +
>> +	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
>> +	BUG_ON(ret);
>> +	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
>> +		BUG_ON(1);
>> +
>> +	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
>> +	BUG_ON(ret);
>> +	bounce_bs_setup = true;
>> +}
>> +
>>  #if defined(CONFIG_HIGHMEM)
>>  static __init int init_emergency_pool(void)
>>  {
>> @@ -44,14 +62,7 @@ static __init int init_emergency_pool(void)
>>  	BUG_ON(ret);
>>  	pr_info("pool size: %d pages\n", POOL_SIZE);
>>  
>> -	ret = bioset_init(&bounce_bio_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS);
>> -	BUG_ON(ret);
>> -	if (bioset_integrity_create(&bounce_bio_set, BIO_POOL_SIZE))
>> -		BUG_ON(1);
>> -
>> -	ret = bioset_init(&bounce_bio_split, BIO_POOL_SIZE, 0, 0);
>> -	BUG_ON(ret);
>> -
>> +	init_bounce_bioset();
>>  	return 0;
>>  }
>>  
>> @@ -86,6 +97,8 @@ static void *mempool_alloc_pages_isa(gfp_t gfp_mask, void *data)
>>  	return mempool_alloc_pages(gfp_mask | GFP_DMA, data);
>>  }
>>  
>> +static DEFINE_MUTEX(isa_mutex);
>> +
>>  /*
>>   * gets called "every" time someone init's a queue with BLK_BOUNCE_ISA
>>   * as the max address, so check if the pool has already been created.
>> @@ -94,14 +107,20 @@ int init_emergency_isa_pool(void)
>>  {
>>  	int ret;
>>  
>> -	if (mempool_initialized(&isa_page_pool))
>> +	mutex_lock(&isa_mutex);
>> +
>> +	if (mempool_initialized(&isa_page_pool)) {
>> +		mutex_unlock(&isa_mutex);
>>  		return 0;
>> +	}
>>  
>>  	ret = mempool_init(&isa_page_pool, ISA_POOL_SIZE, mempool_alloc_pages_isa,
>>  			   mempool_free_pages, (void *) 0);
>>  	BUG_ON(ret);
>>  
>>  	pr_info("isa pool size: %d pages\n", ISA_POOL_SIZE);
>> +	init_bounce_bioset();
>> +	mutex_unlock(&isa_mutex);
>>  	return 0;
>>  }
>>  
> 
> Works for me.

Thanks Ondrej, I'm going to queue it up for 4.20 and mark it for stable.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-10-21 18:02 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-18 21:24 [PATCH] block: setup bounce bio_sets properly Jens Axboe
2018-10-19  9:10 ` Ming Lei
2018-10-19 16:35   ` Jens Axboe
2018-10-21 12:17     ` Ondrej Zary
2018-10-21 18:02       ` Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.