* [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@vger.kernel.org; +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@vger.kernel.org, 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@vger.kernel.org, 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@vger.kernel.org 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@vger.kernel.org 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox