Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, May 01 2017, Jens Axboe wrote: > On 04/30/2017 11:00 PM, NeilBrown wrote: >> On Mon, Apr 24 2017, Christoph Hellwig wrote: >> >>> On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: I was following the existing practice exemplified by bioset_create_nobvec(). >>> >>> Which is pretty ugly to start with.. >> >> That is a matter of personal taste. >> As such, it is up to the maintainer to change it if they want it >> changed. >> >>> By not changing the signature of the function, I can avoid touching quite a few places where it is called. >>> >>> There are 13 callers of bioset_create and one caller of >>> bioset_create_nobvec, and your series touches many of those. >>> >>> So just adding a flags argument to bioset_create and passing >>> BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem >>> to much of an effort, and it's going to create a much nicer and easier >>> to extend interface. >> >> If someone else submitted a patch to discard bioset_create_nobvec in >> favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my >> series on that. As it is, I'm basing my patches on the style currently >> present in the tree. >> >> Of course, if Jens says he'll only take my patches if I change to style >> to match your preference, I'll do that. > > I generally tend to prefer tree wide cleanups to improve our APIs, even > if it does cause an extra bit of pain. Would you mind doing that as a > prep patch? OK, will do. I have rebased and fixed up a couple of issues. Will repost shortly. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, May 01 2017, Jens Axboe wrote: > On 04/30/2017 11:00 PM, NeilBrown wrote: >> On Mon, Apr 24 2017, Christoph Hellwig wrote: >> >>> On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: I was following the existing practice exemplified by bioset_create_nobvec(). >>> >>> Which is pretty ugly to start with.. >> >> That is a matter of personal taste. >> As such, it is up to the maintainer to change it if they want it >> changed. >> >>> By not changing the signature of the function, I can avoid touching quite a few places where it is called. >>> >>> There are 13 callers of bioset_create and one caller of >>> bioset_create_nobvec, and your series touches many of those. >>> >>> So just adding a flags argument to bioset_create and passing >>> BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem >>> to much of an effort, and it's going to create a much nicer and easier >>> to extend interface. >> >> If someone else submitted a patch to discard bioset_create_nobvec in >> favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my >> series on that. As it is, I'm basing my patches on the style currently >> present in the tree. >> >> Of course, if Jens says he'll only take my patches if I change to style >> to match your preference, I'll do that. > > I generally tend to prefer tree wide cleanups to improve our APIs, even > if it does cause an extra bit of pain. Would you mind doing that as a > prep patch? OK, will do. I have rebased and fixed up a couple of issues. Will repost shortly. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On 04/30/2017 11:00 PM, NeilBrown wrote: > On Mon, Apr 24 2017, Christoph Hellwig wrote: > >> On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: >>> >>> I was following the existing practice exemplified by >>> bioset_create_nobvec(). >> >> Which is pretty ugly to start with.. > > That is a matter of personal taste. > As such, it is up to the maintainer to change it if they want it > changed. > >> >>> By not changing the signature of the function, I can avoid touching >>> quite a few places where it is called. >> >> There are 13 callers of bioset_create and one caller of >> bioset_create_nobvec, and your series touches many of those. >> >> So just adding a flags argument to bioset_create and passing >> BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem >> to much of an effort, and it's going to create a much nicer and easier >> to extend interface. > > If someone else submitted a patch to discard bioset_create_nobvec in > favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my > series on that. As it is, I'm basing my patches on the style currently > present in the tree. > > Of course, if Jens says he'll only take my patches if I change to style > to match your preference, I'll do that. I generally tend to prefer tree wide cleanups to improve our APIs, even if it does cause an extra bit of pain. Would you mind doing that as a prep patch? -- Jens Axboe
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On 04/30/2017 11:00 PM, NeilBrown wrote: > On Mon, Apr 24 2017, Christoph Hellwig wrote: > >> On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: >>> >>> I was following the existing practice exemplified by >>> bioset_create_nobvec(). >> >> Which is pretty ugly to start with.. > > That is a matter of personal taste. > As such, it is up to the maintainer to change it if they want it > changed. > >> >>> By not changing the signature of the function, I can avoid touching >>> quite a few places where it is called. >> >> There are 13 callers of bioset_create and one caller of >> bioset_create_nobvec, and your series touches many of those. >> >> So just adding a flags argument to bioset_create and passing >> BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem >> to much of an effort, and it's going to create a much nicer and easier >> to extend interface. > > If someone else submitted a patch to discard bioset_create_nobvec in > favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my > series on that. As it is, I'm basing my patches on the style currently > present in the tree. > > Of course, if Jens says he'll only take my patches if I change to style > to match your preference, I'll do that. I generally tend to prefer tree wide cleanups to improve our APIs, even if it does cause an extra bit of pain. Would you mind doing that as a prep patch? -- Jens Axboe
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, Apr 24 2017, Christoph Hellwig wrote: > On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: >> >> I was following the existing practice exemplified by >> bioset_create_nobvec(). > > Which is pretty ugly to start with.. That is a matter of personal taste. As such, it is up to the maintainer to change it if they want it changed. > >> By not changing the signature of the function, I can avoid touching >> quite a few places where it is called. > > There are 13 callers of bioset_create and one caller of > bioset_create_nobvec, and your series touches many of those. > > So just adding a flags argument to bioset_create and passing > BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem > to much of an effort, and it's going to create a much nicer and easier > to extend interface. If someone else submitted a patch to discard bioset_create_nobvec in favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my series on that. As it is, I'm basing my patches on the style currently present in the tree. Of course, if Jens says he'll only take my patches if I change to style to match your preference, I'll do that. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, Apr 24 2017, Christoph Hellwig wrote: > On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: >> >> I was following the existing practice exemplified by >> bioset_create_nobvec(). > > Which is pretty ugly to start with.. That is a matter of personal taste. As such, it is up to the maintainer to change it if they want it changed. > >> By not changing the signature of the function, I can avoid touching >> quite a few places where it is called. > > There are 13 callers of bioset_create and one caller of > bioset_create_nobvec, and your series touches many of those. > > So just adding a flags argument to bioset_create and passing > BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem > to much of an effort, and it's going to create a much nicer and easier > to extend interface. If someone else submitted a patch to discard bioset_create_nobvec in favour of BIOSET_NEED_BVECS and got it accepted, then I would rebase my series on that. As it is, I'm basing my patches on the style currently present in the tree. Of course, if Jens says he'll only take my patches if I change to style to match your preference, I'll do that. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: > > I was following the existing practice exemplified by > bioset_create_nobvec(). Which is pretty ugly to start with.. > By not changing the signature of the function, I can avoid touching > quite a few places where it is called. There are 13 callers of bioset_create and one caller of bioset_create_nobvec, and your series touches many of those. So just adding a flags argument to bioset_create and passing BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem to much of an effort, and it's going to create a much nicer and easier to extend interface.
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Mon, Apr 24, 2017 at 11:51:01AM +1000, NeilBrown wrote: > > I was following the existing practice exemplified by > bioset_create_nobvec(). Which is pretty ugly to start with.. > By not changing the signature of the function, I can avoid touching > quite a few places where it is called. There are 13 callers of bioset_create and one caller of bioset_create_nobvec, and your series touches many of those. So just adding a flags argument to bioset_create and passing BIOSET_NEED_BVECS and BIOSET_NEED_RESUER flags to it doesn't seem to much of an effort, and it's going to create a much nicer and easier to extend interface.
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Fri, Apr 21 2017, Christoph Hellwig wrote: > On Thu, Apr 20, 2017 at 03:38:48PM +1000, NeilBrown wrote: >> This patch converts bioset_create() and >> bioset_create_nobvec() to not create a workqueue so >> alloctions will never trigger punt_bios_to_rescuer(). It >> also introduces bioset_create_rescued() and >> bioset_create_nobvec_rescued() which preserve the old >> behaviour. > > Why these super-early line breaks in the committ message? They make > the text much more awkware compared to say: I usually set a smaller wrap-margin for git comments because of the extra space that git inserts on the left. Maybe I over-do it. > > This patch converts bioset_create() and bioset_create_nobvec() to not > create a workqueue so alloctions will never trigger > punt_bios_to_rescuer(). It also introduces bioset_create_rescued() and > bioset_create_nobvec_rescued() which preserve the old behaviour. > >> static struct bio_set *__bioset_create(unsigned int pool_size, >> unsigned int front_pad, >> - bool create_bvec_pool) >> + bool create_bvec_pool, >> + bool create_rescue_workqueue) > > I'd much prefer a single new bioset_create with a bunch of flags > arguments over the number of new functions and these bool arguments. I was following the existing practice exemplified by bioset_create_nobvec(). By not changing the signature of the function, I can avoid touching quite a few places where it is called. I hope to get rid of the _rescued() versions eventually. That is easier if there are a separate function rather than an extra arg that needs to be removed everywhere. NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Fri, Apr 21 2017, Christoph Hellwig wrote: > On Thu, Apr 20, 2017 at 03:38:48PM +1000, NeilBrown wrote: >> This patch converts bioset_create() and >> bioset_create_nobvec() to not create a workqueue so >> alloctions will never trigger punt_bios_to_rescuer(). It >> also introduces bioset_create_rescued() and >> bioset_create_nobvec_rescued() which preserve the old >> behaviour. > > Why these super-early line breaks in the committ message? They make > the text much more awkware compared to say: I usually set a smaller wrap-margin for git comments because of the extra space that git inserts on the left. Maybe I over-do it. > > This patch converts bioset_create() and bioset_create_nobvec() to not > create a workqueue so alloctions will never trigger > punt_bios_to_rescuer(). It also introduces bioset_create_rescued() and > bioset_create_nobvec_rescued() which preserve the old behaviour. > >> static struct bio_set *__bioset_create(unsigned int pool_size, >> unsigned int front_pad, >> - bool create_bvec_pool) >> + bool create_bvec_pool, >> + bool create_rescue_workqueue) > > I'd much prefer a single new bioset_create with a bunch of flags > arguments over the number of new functions and these bool arguments. I was following the existing practice exemplified by bioset_create_nobvec(). By not changing the signature of the function, I can avoid touching quite a few places where it is called. I hope to get rid of the _rescued() versions eventually. That is easier if there are a separate function rather than an extra arg that needs to be removed everywhere. NeilBrown signature.asc Description: PGP signature
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Thu, Apr 20, 2017 at 03:38:48PM +1000, NeilBrown wrote: > This patch converts bioset_create() and > bioset_create_nobvec() to not create a workqueue so > alloctions will never trigger punt_bios_to_rescuer(). It > also introduces bioset_create_rescued() and > bioset_create_nobvec_rescued() which preserve the old > behaviour. Why these super-early line breaks in the committ message? They make the text much more awkware compared to say: This patch converts bioset_create() and bioset_create_nobvec() to not create a workqueue so alloctions will never trigger punt_bios_to_rescuer(). It also introduces bioset_create_rescued() and bioset_create_nobvec_rescued() which preserve the old behaviour. > static struct bio_set *__bioset_create(unsigned int pool_size, > unsigned int front_pad, > -bool create_bvec_pool) > +bool create_bvec_pool, > +bool create_rescue_workqueue) I'd much prefer a single new bioset_create with a bunch of flags arguments over the number of new functions and these bool arguments.
Re: [PATCH 02/11] blk: make the bioset rescue_workqueue optional.
On Thu, Apr 20, 2017 at 03:38:48PM +1000, NeilBrown wrote: > This patch converts bioset_create() and > bioset_create_nobvec() to not create a workqueue so > alloctions will never trigger punt_bios_to_rescuer(). It > also introduces bioset_create_rescued() and > bioset_create_nobvec_rescued() which preserve the old > behaviour. Why these super-early line breaks in the committ message? They make the text much more awkware compared to say: This patch converts bioset_create() and bioset_create_nobvec() to not create a workqueue so alloctions will never trigger punt_bios_to_rescuer(). It also introduces bioset_create_rescued() and bioset_create_nobvec_rescued() which preserve the old behaviour. > static struct bio_set *__bioset_create(unsigned int pool_size, > unsigned int front_pad, > -bool create_bvec_pool) > +bool create_bvec_pool, > +bool create_rescue_workqueue) I'd much prefer a single new bioset_create with a bunch of flags arguments over the number of new functions and these bool arguments.
