| Message ID | 149369654558.5146.13818001419050826182.stgit@noble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
> On 2 May 2017, at 05.42, NeilBrown <neilb@suse.com> wrote: > > pblk_submit_read() uses bio_clone_bioset() but doesn't change the > io_vec, so bio_clone_fast() is a better choice. > > It also uses fs_bio_set which is intended for filesystems. Using it > in a device driver can deadlock. > So allocate a new bioset, and and use bio_clone_fast(). > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > drivers/lightnvm/pblk-init.c | 12 +++++++++++- > drivers/lightnvm/pblk-read.c | 2 +- > drivers/lightnvm/pblk.h | 1 + > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > index b3fec8ec55b8..aaefbccce30e 100644 > --- a/drivers/lightnvm/pblk-init.c > +++ b/drivers/lightnvm/pblk-init.c > @@ -23,6 +23,7 @@ > static struct kmem_cache *pblk_blk_ws_cache, *pblk_rec_cache, *pblk_r_rq_cache, > *pblk_w_rq_cache, *pblk_line_meta_cache; > static DECLARE_RWSEM(pblk_lock); > +struct bio_set *pblk_bio_set; > > static int pblk_rw_io(struct request_queue *q, struct pblk *pblk, > struct bio *bio) > @@ -946,11 +947,20 @@ static struct nvm_tgt_type tt_pblk = { > > static int __init pblk_module_init(void) > { > - return nvm_register_tgt_type(&tt_pblk); > + int ret; > + > + pblk_bio_set = bioset_create(BIO_POOL_SIZE, 0, 0); > + if (!pblk_bio_set) > + return -ENOMEM; > + ret = nvm_register_tgt_type(&tt_pblk); > + if (ret) > + bioset_free(pblk_bio_set); > + return ret; > } > > static void pblk_module_exit(void) > { > + bioset_free(pblk_bio_set); > nvm_unregister_tgt_type(&tt_pblk); > } > > diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c > index 4a12f14d78c6..8ee0c50a7a71 100644 > --- a/drivers/lightnvm/pblk-read.c > +++ b/drivers/lightnvm/pblk-read.c > @@ -342,7 +342,7 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio) > struct pblk_r_ctx *r_ctx = nvm_rq_to_pdu(rqd); > > /* Clone read bio to deal with read errors internally */ > - int_bio = bio_clone_bioset(bio, GFP_KERNEL, fs_bio_set); > + int_bio = bio_clone_fast(bio, GFP_KERNEL, pblk_bio_set); > if (!int_bio) { > pr_err("pblk: could not clone read bio\n"); > return NVM_IO_ERR; > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h > index 99f3186b5288..95b665f23925 100644 > --- a/drivers/lightnvm/pblk.h > +++ b/drivers/lightnvm/pblk.h > @@ -702,6 +702,7 @@ void pblk_write_should_kick(struct pblk *pblk); > /* > * pblk read path > */ > +extern struct bio_set *pblk_bio_set; > int pblk_submit_read(struct pblk *pblk, struct bio *bio); > int pblk_submit_read_gc(struct pblk *pblk, u64 *lba_list, void *data, > unsigned int nr_secs, unsigned int *secs_to_gc, Hi Neil, Looks good. Thanks for fixing this. I did not know that bio_clone_bioset was not supposed to be used on drivers. Reviewed-by: Javier González <javier@cnexlabs.com> Tested-by: Javier González <javier@cnexlabs.com>
On Tue, May 02 2017, Javier González wrote: >> On 2 May 2017, at 05.42, NeilBrown <neilb@suse.com> wrote: >> >> pblk_submit_read() uses bio_clone_bioset() but doesn't change the >> io_vec, so bio_clone_fast() is a better choice. >> >> It also uses fs_bio_set which is intended for filesystems. Using it >> in a device driver can deadlock. >> So allocate a new bioset, and and use bio_clone_fast(). >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> drivers/lightnvm/pblk-init.c | 12 +++++++++++- >> drivers/lightnvm/pblk-read.c | 2 +- >> drivers/lightnvm/pblk.h | 1 + >> 3 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >> index b3fec8ec55b8..aaefbccce30e 100644 >> --- a/drivers/lightnvm/pblk-init.c >> +++ b/drivers/lightnvm/pblk-init.c >> @@ -23,6 +23,7 @@ >> static struct kmem_cache *pblk_blk_ws_cache, *pblk_rec_cache, *pblk_r_rq_cache, >> *pblk_w_rq_cache, *pblk_line_meta_cache; >> static DECLARE_RWSEM(pblk_lock); >> +struct bio_set *pblk_bio_set; >> >> static int pblk_rw_io(struct request_queue *q, struct pblk *pblk, >> struct bio *bio) >> @@ -946,11 +947,20 @@ static struct nvm_tgt_type tt_pblk = { >> >> static int __init pblk_module_init(void) >> { >> - return nvm_register_tgt_type(&tt_pblk); >> + int ret; >> + >> + pblk_bio_set = bioset_create(BIO_POOL_SIZE, 0, 0); >> + if (!pblk_bio_set) >> + return -ENOMEM; >> + ret = nvm_register_tgt_type(&tt_pblk); >> + if (ret) >> + bioset_free(pblk_bio_set); >> + return ret; >> } >> >> static void pblk_module_exit(void) >> { >> + bioset_free(pblk_bio_set); >> nvm_unregister_tgt_type(&tt_pblk); >> } >> >> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c >> index 4a12f14d78c6..8ee0c50a7a71 100644 >> --- a/drivers/lightnvm/pblk-read.c >> +++ b/drivers/lightnvm/pblk-read.c >> @@ -342,7 +342,7 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio) >> struct pblk_r_ctx *r_ctx = nvm_rq_to_pdu(rqd); >> >> /* Clone read bio to deal with read errors internally */ >> - int_bio = bio_clone_bioset(bio, GFP_KERNEL, fs_bio_set); >> + int_bio = bio_clone_fast(bio, GFP_KERNEL, pblk_bio_set); >> if (!int_bio) { >> pr_err("pblk: could not clone read bio\n"); >> return NVM_IO_ERR; >> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h >> index 99f3186b5288..95b665f23925 100644 >> --- a/drivers/lightnvm/pblk.h >> +++ b/drivers/lightnvm/pblk.h >> @@ -702,6 +702,7 @@ void pblk_write_should_kick(struct pblk *pblk); >> /* >> * pblk read path >> */ >> +extern struct bio_set *pblk_bio_set; >> int pblk_submit_read(struct pblk *pblk, struct bio *bio); >> int pblk_submit_read_gc(struct pblk *pblk, u64 *lba_list, void *data, >> unsigned int nr_secs, unsigned int *secs_to_gc, > > Hi Neil, > > Looks good. Thanks for fixing this. I did not know that bio_clone_bioset > was not supposed to be used on drivers. Prior to my patchset, using bio_clone_bioset() wasn't wrong in drivers, though it was a waste when bio_clone_fast() would to just as well as is more efficient. After my patchset, using it can be problematic. I'm wondering what I should do to encourage those problems to be more visible so that if people to us it, they'll get a warning or something. Thanks, NeilBrown > > Reviewed-by: Javier González <javier@cnexlabs.com> > Tested-by: Javier González <javier@cnexlabs.com>
> On 2 May 2017, at 23.51, NeilBrown <neilb@suse.com> wrote: > >> >> Hi Neil, >> >> Looks good. Thanks for fixing this. I did not know that bio_clone_bioset >> was not supposed to be used on drivers. > > Prior to my patchset, using bio_clone_bioset() wasn't wrong in drivers, > though it was a waste when bio_clone_fast() would to just as well as is > more efficient. > After my patchset, using it can be problematic. I'm wondering what I > should do to encourage those problems to be more visible so that if > people to us it, they'll get a warning or something. > > Thanks, > NeilBrown > Thanks for the explanation Neil. In my opinion a comment on top of bio_clone_bioset() would be hellful, as Ming suggested. But a warning might make sense too. Javier
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c index b3fec8ec55b8..aaefbccce30e 100644 --- a/drivers/lightnvm/pblk-init.c +++ b/drivers/lightnvm/pblk-init.c @@ -23,6 +23,7 @@ static struct kmem_cache *pblk_blk_ws_cache, *pblk_rec_cache, *pblk_r_rq_cache, *pblk_w_rq_cache, *pblk_line_meta_cache; static DECLARE_RWSEM(pblk_lock); +struct bio_set *pblk_bio_set; static int pblk_rw_io(struct request_queue *q, struct pblk *pblk, struct bio *bio) @@ -946,11 +947,20 @@ static struct nvm_tgt_type tt_pblk = { static int __init pblk_module_init(void) { - return nvm_register_tgt_type(&tt_pblk); + int ret; + + pblk_bio_set = bioset_create(BIO_POOL_SIZE, 0, 0); + if (!pblk_bio_set) + return -ENOMEM; + ret = nvm_register_tgt_type(&tt_pblk); + if (ret) + bioset_free(pblk_bio_set); + return ret; } static void pblk_module_exit(void) { + bioset_free(pblk_bio_set); nvm_unregister_tgt_type(&tt_pblk); } diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c index 4a12f14d78c6..8ee0c50a7a71 100644 --- a/drivers/lightnvm/pblk-read.c +++ b/drivers/lightnvm/pblk-read.c @@ -342,7 +342,7 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio) struct pblk_r_ctx *r_ctx = nvm_rq_to_pdu(rqd); /* Clone read bio to deal with read errors internally */ - int_bio = bio_clone_bioset(bio, GFP_KERNEL, fs_bio_set); + int_bio = bio_clone_fast(bio, GFP_KERNEL, pblk_bio_set); if (!int_bio) { pr_err("pblk: could not clone read bio\n"); return NVM_IO_ERR; diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h index 99f3186b5288..95b665f23925 100644 --- a/drivers/lightnvm/pblk.h +++ b/drivers/lightnvm/pblk.h @@ -702,6 +702,7 @@ void pblk_write_should_kick(struct pblk *pblk); /* * pblk read path */ +extern struct bio_set *pblk_bio_set; int pblk_submit_read(struct pblk *pblk, struct bio *bio); int pblk_submit_read_gc(struct pblk *pblk, u64 *lba_list, void *data, unsigned int nr_secs, unsigned int *secs_to_gc,
pblk_submit_read() uses bio_clone_bioset() but doesn't change the io_vec, so bio_clone_fast() is a better choice. It also uses fs_bio_set which is intended for filesystems. Using it in a device driver can deadlock. So allocate a new bioset, and and use bio_clone_fast(). Signed-off-by: NeilBrown <neilb@suse.com> --- drivers/lightnvm/pblk-init.c | 12 +++++++++++- drivers/lightnvm/pblk-read.c | 2 +- drivers/lightnvm/pblk.h | 1 + 3 files changed, 13 insertions(+), 2 deletions(-)