* [PATCH BUGFIX] block: add missing group association in bio_split @ 2016-05-06 20:45 Paolo Valente 2016-05-09 14:19 ` Jeff Moyer 2016-05-09 15:20 ` [PATCH BUGFIX] block: add missing group association in bio_split Tejun Heo 0 siblings, 2 replies; 29+ messages in thread From: Paolo Valente @ 2016-05-06 20:45 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, Paolo Valente When a bio is split, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the frequency of splits, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be then broken. This commit adds the missing association in bio_split. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bio.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/bio.c b/block/bio.c index 807d25e..c4a3834 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int sectors, bio_advance(bio, split->bi_iter.bi_size); +#ifdef CONFIG_BLK_CGROUP + if (bio->bi_css) + bio_associate_blkcg(split, bio->bi_css); +#endif + return split; } EXPORT_SYMBOL(bio_split); -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-06 20:45 [PATCH BUGFIX] block: add missing group association in bio_split Paolo Valente @ 2016-05-09 14:19 ` Jeff Moyer 2016-05-09 14:35 ` Jeff Moyer 2016-05-09 15:20 ` [PATCH BUGFIX] block: add missing group association in bio_split Tejun Heo 1 sibling, 1 reply; 29+ messages in thread From: Jeff Moyer @ 2016-05-09 14:19 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Paolo Valente <paolo.valente@linaro.org> writes: > @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int sectors, > > bio_advance(bio, split->bi_iter.bi_size); > > +#ifdef CONFIG_BLK_CGROUP > + if (bio->bi_css) > + bio_associate_blkcg(split, bio->bi_css); > +#endif > + > return split; > } > EXPORT_SYMBOL(bio_split); Get rid of the #ifdefery. This should be just: bio_associate_blkcg(split, bio->bi_css); Cheers, Jeff ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 14:19 ` Jeff Moyer @ 2016-05-09 14:35 ` Jeff Moyer 2016-05-09 14:39 ` Paolo 0 siblings, 1 reply; 29+ messages in thread From: Jeff Moyer @ 2016-05-09 14:35 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Jeff Moyer <jmoyer@redhat.com> writes: > Paolo Valente <paolo.valente@linaro.org> writes: > >> @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int sectors, >> >> bio_advance(bio, split->bi_iter.bi_size); >> >> +#ifdef CONFIG_BLK_CGROUP >> + if (bio->bi_css) >> + bio_associate_blkcg(split, bio->bi_css); >> +#endif >> + >> return split; >> } >> EXPORT_SYMBOL(bio_split); > > Get rid of the #ifdefery. This should be just: > > bio_associate_blkcg(split, bio->bi_css); Gah, I see that the bi_css member is only present for CONFIG_BLK_CGROUP. I guess we'll have to live with the ifdef. -Jeff ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 14:35 ` Jeff Moyer @ 2016-05-09 14:39 ` Paolo 2016-05-09 14:55 ` Jens Axboe 2016-05-09 14:56 ` Mark Brown 0 siblings, 2 replies; 29+ messages in thread From: Paolo @ 2016-05-09 14:39 UTC (permalink / raw) To: Jeff Moyer Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Il 09/05/2016 16:35, Jeff Moyer ha scritto: > Jeff Moyer <jmoyer@redhat.com> writes: > >> Paolo Valente <paolo.valente@linaro.org> writes: >> >>> @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int sectors, >>> >>> bio_advance(bio, split->bi_iter.bi_size); >>> >>> +#ifdef CONFIG_BLK_CGROUP >>> + if (bio->bi_css) >>> + bio_associate_blkcg(split, bio->bi_css); >>> +#endif >>> + >>> return split; >>> } >>> EXPORT_SYMBOL(bio_split); >> >> Get rid of the #ifdefery. This should be just: >> >> bio_associate_blkcg(split, bio->bi_css); > > Gah, I see that the bi_css member is only present for CONFIG_BLK_CGROUP. > I guess we'll have to live with the ifdef. > We have already tried to remove it, but it seems it would require other major changes. Thanks, Paolo > -Jeff > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 14:39 ` Paolo @ 2016-05-09 14:55 ` Jens Axboe 2016-05-09 14:56 ` Mark Brown 1 sibling, 0 replies; 29+ messages in thread From: Jens Axboe @ 2016-05-09 14:55 UTC (permalink / raw) To: Paolo, Jeff Moyer Cc: Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On 05/09/2016 08:39 AM, Paolo wrote: > Il 09/05/2016 16:35, Jeff Moyer ha scritto: >> Jeff Moyer <jmoyer@redhat.com> writes: >> >>> Paolo Valente <paolo.valente@linaro.org> writes: >>> >>>> @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int >>>> sectors, >>>> >>>> bio_advance(bio, split->bi_iter.bi_size); >>>> >>>> +#ifdef CONFIG_BLK_CGROUP >>>> + if (bio->bi_css) >>>> + bio_associate_blkcg(split, bio->bi_css); >>>> +#endif >>>> + >>>> return split; >>>> } >>>> EXPORT_SYMBOL(bio_split); >>> >>> Get rid of the #ifdefery. This should be just: >>> >>> bio_associate_blkcg(split, bio->bi_css); >> >> Gah, I see that the bi_css member is only present for CONFIG_BLK_CGROUP. >> I guess we'll have to live with the ifdef. >> > > We have already tried to remove it, but it seems it would require other > major changes. It'd be cleaner to have a bio_clone_associate_blkcg(split, bio); or similar, and then hide it in there. -- Jens Axboe ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 14:39 ` Paolo 2016-05-09 14:55 ` Jens Axboe @ 2016-05-09 14:56 ` Mark Brown 2016-05-09 17:58 ` Jens Axboe 1 sibling, 1 reply; 29+ messages in thread From: Mark Brown @ 2016-05-09 14:56 UTC (permalink / raw) To: Paolo Cc: Jeff Moyer, Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, stable [-- Attachment #1: Type: text/plain, Size: 380 bytes --] On Mon, May 09, 2016 at 04:39:04PM +0200, Paolo wrote: > Il 09/05/2016 16:35, Jeff Moyer ha scritto: > > Gah, I see that the bi_css member is only present for CONFIG_BLK_CGROUP. > > I guess we'll have to live with the ifdef. > We have already tried to remove it, but it seems it would require other > major changes. It probably should get fixed, but separately to the bug fix. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 14:56 ` Mark Brown @ 2016-05-09 17:58 ` Jens Axboe 2016-05-10 9:08 ` Paolo Valente 0 siblings, 1 reply; 29+ messages in thread From: Jens Axboe @ 2016-05-09 17:58 UTC (permalink / raw) To: Mark Brown, Paolo Cc: Jeff Moyer, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, stable On 05/09/2016 08:56 AM, Mark Brown wrote: > On Mon, May 09, 2016 at 04:39:04PM +0200, Paolo wrote: >> Il 09/05/2016 16:35, Jeff Moyer ha scritto: > >>> Gah, I see that the bi_css member is only present for CONFIG_BLK_CGROUP. >>> I guess we'll have to live with the ifdef. > >> We have already tried to remove it, but it seems it would require other >> major changes. > > It probably should get fixed, but separately to the bug fix. It's a minor tweak, might as well submit them together. Because if you don't, it has a tendency to be forgotten once the original issue is resolved. -- Jens Axboe ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-09 17:58 ` Jens Axboe @ 2016-05-10 9:08 ` Paolo Valente 2016-05-10 16:12 ` Jeff Moyer 0 siblings, 1 reply; 29+ messages in thread From: Paolo Valente @ 2016-05-10 9:08 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, Paolo Valente When a bio is split, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the frequency of splits, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be then broken. This commit adds the missing association in bio_split. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bio.c | 15 +++++++++++++++ include/linux/bio.h | 2 ++ 2 files changed, 17 insertions(+) diff --git a/block/bio.c b/block/bio.c index 807d25e..20795eb 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1811,6 +1811,8 @@ struct bio *bio_split(struct bio *bio, int sectors, bio_advance(bio, split->bi_iter.bi_size); + bio_clone_blkcg_association(split, bio); + return split; } EXPORT_SYMBOL(bio_split); @@ -2016,6 +2018,19 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + return bio_associate_blkcg(dst, src->bi_css); + + return 0; +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..8535f81 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,13 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +int bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-10 9:08 ` Paolo Valente @ 2016-05-10 16:12 ` Jeff Moyer 2016-05-10 17:08 ` Paolo 2016-05-10 20:23 ` [PATCH BUGFIX] block: add missing group association in bio-cloning functions Paolo Valente 0 siblings, 2 replies; 29+ messages in thread From: Jeff Moyer @ 2016-05-10 16:12 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie Paolo Valente <paolo.valente@linaro.org> writes: > When a bio is split, the newly created bio must be associated with the > same blkcg as the original bio (if BLK_CGROUP is enabled). If this > operation is not performed, then the new bio is not associated with > any group, and the group of the current task is returned when the > group of the bio is requested. > > Depending on the frequency of splits, this may cause a large > percentage of the bios belonging to a given group to be treated as if > belonging to other groups (in most cases as if belonging to the root > group). The expected group isolation may thereby be then broken. > > This commit adds the missing association in bio_split. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bio.c | 15 +++++++++++++++ > include/linux/bio.h | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/block/bio.c b/block/bio.c > index 807d25e..20795eb 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1811,6 +1811,8 @@ struct bio *bio_split(struct bio *bio, int sectors, > > bio_advance(bio, split->bi_iter.bi_size); > > + bio_clone_blkcg_association(split, bio); > + First, I think this should be done inside of bio_clone_fast and bio_clone_bioset instead of in bio_split. Otherwise you miss other places where bios are cloned, and I'm guessing that's bad. You could also then get rid of this code in btrfs/extent_io.c: #ifdef CONFIG_BLK_CGROUP /* FIXME, put this into bio_clone_bioset */ if (bio->bi_css) bio_associate_blkcg(new, bio->bi_css); #endif Next, you went to the trouble of propagating the return value from bio_associate_blkcg, and then it goes unchecked here in the only caller of your new function. Since bio_associate_blkcg should not fail when cloning (right?), I'd change bio_clone_blkcg_association to return void and to WARN_ON a failure return from bio_associate_blkcg. Cheers, Jeff > return split; > } > EXPORT_SYMBOL(bio_split); > @@ -2016,6 +2018,19 @@ void bio_disassociate_task(struct bio *bio) > } > } > > +/** > + * bio_clone_blkcg_association - clone blkcg association from src to dst bio > + * @dst: destination bio > + * @src: source bio > + */ > +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) > +{ > + if (src->bi_css) > + return bio_associate_blkcg(dst, src->bi_css); > + > + return 0; > +} > + > #endif /* CONFIG_BLK_CGROUP */ > > static void __init biovec_init_slabs(void) > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 6b7481f..8535f81 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -527,11 +527,13 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); > int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); > int bio_associate_current(struct bio *bio); > void bio_disassociate_task(struct bio *bio); > +int bio_clone_blkcg_association(struct bio *dst, struct bio *src); > #else /* CONFIG_BLK_CGROUP */ > static inline int bio_associate_blkcg(struct bio *bio, > struct cgroup_subsys_state *blkcg_css) { return 0; } > static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } > static inline void bio_disassociate_task(struct bio *bio) { } > +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) { return 0; } > #endif /* CONFIG_BLK_CGROUP */ > > #ifdef CONFIG_HIGHMEM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-10 16:12 ` Jeff Moyer @ 2016-05-10 17:08 ` Paolo 2016-05-10 20:23 ` [PATCH BUGFIX] block: add missing group association in bio-cloning functions Paolo Valente 1 sibling, 0 replies; 29+ messages in thread From: Paolo @ 2016-05-10 17:08 UTC (permalink / raw) To: Jeff Moyer Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie Il 10/05/2016 18:12, Jeff Moyer ha scritto: > Paolo Valente <paolo.valente@linaro.org> writes: > >> When a bio is split, the newly created bio must be associated with the >> same blkcg as the original bio (if BLK_CGROUP is enabled). If this >> operation is not performed, then the new bio is not associated with >> any group, and the group of the current task is returned when the >> group of the bio is requested. >> >> Depending on the frequency of splits, this may cause a large >> percentage of the bios belonging to a given group to be treated as if >> belonging to other groups (in most cases as if belonging to the root >> group). The expected group isolation may thereby be then broken. >> >> This commit adds the missing association in bio_split. >> >> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> >> --- >> block/bio.c | 15 +++++++++++++++ >> include/linux/bio.h | 2 ++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/block/bio.c b/block/bio.c >> index 807d25e..20795eb 100644 >> --- a/block/bio.c >> +++ b/block/bio.c >> @@ -1811,6 +1811,8 @@ struct bio *bio_split(struct bio *bio, int sectors, >> >> bio_advance(bio, split->bi_iter.bi_size); >> >> + bio_clone_blkcg_association(split, bio); >> + > > First, I think this should be done inside of bio_clone_fast and > bio_clone_bioset instead of in bio_split. Otherwise you miss other > places where bios are cloned, and I'm guessing that's bad. You could > also then get rid of this code in btrfs/extent_io.c: > > #ifdef CONFIG_BLK_CGROUP > /* FIXME, put this into bio_clone_bioset */ > if (bio->bi_css) > bio_associate_blkcg(new, bio->bi_css); > #endif > > Next, you went to the trouble of propagating the return value from > bio_associate_blkcg, and then it goes unchecked here in the only caller > of your new function. Since bio_associate_blkcg should not fail when > cloning (right?), I'd change bio_clone_blkcg_association to return void > and to WARN_ON a failure return from bio_associate_blkcg. > Changing as suggested. Thanks, Paolo > Cheers, > Jeff > >> return split; >> } >> EXPORT_SYMBOL(bio_split); >> @@ -2016,6 +2018,19 @@ void bio_disassociate_task(struct bio *bio) >> } >> } >> >> +/** >> + * bio_clone_blkcg_association - clone blkcg association from src to dst bio >> + * @dst: destination bio >> + * @src: source bio >> + */ >> +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) >> +{ >> + if (src->bi_css) >> + return bio_associate_blkcg(dst, src->bi_css); >> + >> + return 0; >> +} >> + >> #endif /* CONFIG_BLK_CGROUP */ >> >> static void __init biovec_init_slabs(void) >> diff --git a/include/linux/bio.h b/include/linux/bio.h >> index 6b7481f..8535f81 100644 >> --- a/include/linux/bio.h >> +++ b/include/linux/bio.h >> @@ -527,11 +527,13 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); >> int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); >> int bio_associate_current(struct bio *bio); >> void bio_disassociate_task(struct bio *bio); >> +int bio_clone_blkcg_association(struct bio *dst, struct bio *src); >> #else /* CONFIG_BLK_CGROUP */ >> static inline int bio_associate_blkcg(struct bio *bio, >> struct cgroup_subsys_state *blkcg_css) { return 0; } >> static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } >> static inline void bio_disassociate_task(struct bio *bio) { } >> +int bio_clone_blkcg_association(struct bio *dst, struct bio *src) { return 0; } >> #endif /* CONFIG_BLK_CGROUP */ >> >> #ifdef CONFIG_HIGHMEM ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX] block: add missing group association in bio-cloning functions 2016-05-10 16:12 ` Jeff Moyer 2016-05-10 17:08 ` Paolo @ 2016-05-10 20:23 ` Paolo Valente 2016-05-10 20:44 ` Tejun Heo 1 sibling, 1 reply; 29+ messages in thread From: Paolo Valente @ 2016-05-10 20:23 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bio.c | 17 +++++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 2 ++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 807d25e..e9b136a 100644 --- a/block/bio.c +++ b/block/bio.c @@ -622,6 +622,8 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs) } } + bio_clone_blkcg_association(b, bio); + return b; } EXPORT_SYMBOL(bio_clone_fast); @@ -695,6 +697,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -1811,6 +1815,8 @@ struct bio *bio_split(struct bio *bio, int sectors, bio_advance(bio, split->bi_iter.bi_size); + bio_clone_blkcg_association(split, bio); + return split; } EXPORT_SYMBOL(bio_split); @@ -2016,6 +2022,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index d247fc0..19f6739 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..a9ff116 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,13 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio-cloning functions 2016-05-10 20:23 ` [PATCH BUGFIX] block: add missing group association in bio-cloning functions Paolo Valente @ 2016-05-10 20:44 ` Tejun Heo 2016-05-10 21:02 ` [PATCH BUGFIX V2] " Paolo Valente 0 siblings, 1 reply; 29+ messages in thread From: Tejun Heo @ 2016-05-10 20:44 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On Tue, May 10, 2016 at 10:23:41PM +0200, Paolo Valente wrote: > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 6b7481f..a9ff116 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -527,11 +527,13 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); > int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); > int bio_associate_current(struct bio *bio); > void bio_disassociate_task(struct bio *bio); > +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); > #else /* CONFIG_BLK_CGROUP */ > static inline int bio_associate_blkcg(struct bio *bio, > struct cgroup_subsys_state *blkcg_css) { return 0; } > static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } > static inline void bio_disassociate_task(struct bio *bio) { } > +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) { return 0; } ^ static inline -- tejun ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions 2016-05-10 20:44 ` Tejun Heo @ 2016-05-10 21:02 ` Paolo Valente 2016-05-10 21:03 ` Tejun Heo 2016-05-10 21:34 ` Jeff Moyer 0 siblings, 2 replies; 29+ messages in thread From: Paolo Valente @ 2016-05-10 21:02 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bio.c | 17 +++++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 3 +++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 807d25e..e9b136a 100644 --- a/block/bio.c +++ b/block/bio.c @@ -622,6 +622,8 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs) } } + bio_clone_blkcg_association(b, bio); + return b; } EXPORT_SYMBOL(bio_clone_fast); @@ -695,6 +697,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -1811,6 +1815,8 @@ struct bio *bio_split(struct bio *bio, int sectors, bio_advance(bio, split->bi_iter.bi_size); + bio_clone_blkcg_association(split, bio); + return split; } EXPORT_SYMBOL(bio_split); @@ -2016,6 +2022,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index d247fc0..19f6739 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..16cbe59 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +static inline void bio_clone_blkcg_association(struct bio *dst, + struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions 2016-05-10 21:02 ` [PATCH BUGFIX V2] " Paolo Valente @ 2016-05-10 21:03 ` Tejun Heo 2016-05-10 21:34 ` Jeff Moyer 1 sibling, 0 replies; 29+ messages in thread From: Tejun Heo @ 2016-05-10 21:03 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On Tue, May 10, 2016 at 11:02:12PM +0200, Paolo Valente wrote: > When a bio is cloned, the newly created bio must be associated with > the same blkcg as the original bio (if BLK_CGROUP is enabled). If > this operation is not performed, then the new bio is not associated > with any group, and the group of the current task is returned when > the group of the bio is requested. > > Depending on the cloning frequency, this may cause a large > percentage of the bios belonging to a given group to be treated > as if belonging to other groups (in most cases as if belonging to > the root group). The expected group isolation may thereby be broken. > > This commit adds the missing association in bio-cloning functions. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions 2016-05-10 21:02 ` [PATCH BUGFIX V2] " Paolo Valente 2016-05-10 21:03 ` Tejun Heo @ 2016-05-10 21:34 ` Jeff Moyer 2016-05-10 21:44 ` Paolo 2016-05-10 22:01 ` [PATCH BUGFIX V3] " Paolo Valente 1 sibling, 2 replies; 29+ messages in thread From: Jeff Moyer @ 2016-05-10 21:34 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Paolo Valente <paolo.valente@linaro.org> writes: > diff --git a/block/bio.c b/block/bio.c > index 807d25e..e9b136a 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -622,6 +622,8 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs) > } > } > > + bio_clone_blkcg_association(b, bio); > + > return b; > } > EXPORT_SYMBOL(bio_clone_fast); > @@ -695,6 +697,8 @@ integrity_clone: > } > } > > + bio_clone_blkcg_association(bio, bio_src); > + > return bio; > } > EXPORT_SYMBOL(bio_clone_bioset); > @@ -1811,6 +1815,8 @@ struct bio *bio_split(struct bio *bio, int sectors, > > bio_advance(bio, split->bi_iter.bi_size); > > + bio_clone_blkcg_association(split, bio); > + Hi, Paolo, Did you test this? bio_split calls bio_clone_bioset or bio_clone_fast, so I'd be surprised if you didn't trigger that newly added warning. :-) Please remove the bio_split call site. Cheers, Jeff ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions 2016-05-10 21:34 ` Jeff Moyer @ 2016-05-10 21:44 ` Paolo 2016-05-10 22:01 ` [PATCH BUGFIX V3] " Paolo Valente 1 sibling, 0 replies; 29+ messages in thread From: Paolo @ 2016-05-10 21:44 UTC (permalink / raw) To: Jeff Moyer Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Il 10/05/2016 23:34, Jeff Moyer ha scritto: > Paolo Valente <paolo.valente@linaro.org> writes: > >> diff --git a/block/bio.c b/block/bio.c >> index 807d25e..e9b136a 100644 >> --- a/block/bio.c >> +++ b/block/bio.c >> @@ -622,6 +622,8 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs) >> } >> } >> >> + bio_clone_blkcg_association(b, bio); >> + >> return b; >> } >> EXPORT_SYMBOL(bio_clone_fast); >> @@ -695,6 +697,8 @@ integrity_clone: >> } >> } >> >> + bio_clone_blkcg_association(bio, bio_src); >> + >> return bio; >> } >> EXPORT_SYMBOL(bio_clone_bioset); >> @@ -1811,6 +1815,8 @@ struct bio *bio_split(struct bio *bio, int sectors, >> >> bio_advance(bio, split->bi_iter.bi_size); >> >> + bio_clone_blkcg_association(split, bio); >> + > > Hi, Paolo, > > Did you test this? bio_split calls bio_clone_bioset or bio_clone_fast, > so I'd be surprised if you didn't trigger that newly added warning. :-) Of course I didn't check the kernel log ... I hope next version will not introduce more bugs than it will fix. Thanks, Paolo > Please remove the bio_split call site. > > Cheers, > Jeff > ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions 2016-05-10 21:34 ` Jeff Moyer 2016-05-10 21:44 ` Paolo @ 2016-05-10 22:01 ` Paolo Valente 2016-05-11 6:38 ` Nikolay Borisov 1 sibling, 1 reply; 29+ messages in thread From: Paolo Valente @ 2016-05-10 22:01 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bio.c | 15 +++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 3 +++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 807d25e..5eaf0b5 100644 --- a/block/bio.c +++ b/block/bio.c @@ -622,6 +622,8 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs) } } + bio_clone_blkcg_association(b, bio); + return b; } EXPORT_SYMBOL(bio_clone_fast); @@ -695,6 +697,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -2016,6 +2020,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index d247fc0..19f6739 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..16cbe59 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +static inline void bio_clone_blkcg_association(struct bio *dst, + struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions 2016-05-10 22:01 ` [PATCH BUGFIX V3] " Paolo Valente @ 2016-05-11 6:38 ` Nikolay Borisov 2016-05-11 6:43 ` Nikolay Borisov 0 siblings, 1 reply; 29+ messages in thread From: Nikolay Borisov @ 2016-05-11 6:38 UTC (permalink / raw) To: Paolo Valente, Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On 05/11/2016 01:01 AM, Paolo Valente wrote: > When a bio is cloned, the newly created bio must be associated with > the same blkcg as the original bio (if BLK_CGROUP is enabled). If > this operation is not performed, then the new bio is not associated > with any group, and the group of the current task is returned when > the group of the bio is requested. > > Depending on the cloning frequency, this may cause a large > percentage of the bios belonging to a given group to be treated > as if belonging to other groups (in most cases as if belonging to > the root group). The expected group isolation may thereby be broken. > > This commit adds the missing association in bio-cloning functions. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bio.c | 15 +++++++++++++++ > fs/btrfs/extent_io.c | 6 ------ > include/linux/bio.h | 3 +++ > 3 files changed, 18 insertions(+), 6 deletions(-) > Just for reference something like that was already proposed (and tested) before, though it never got merged : https://www.redhat.com/archives/dm-devel/2016-March/msg00007.html So you might also want to patch __bio_clone_fast to also apply this for dm backed devices. Otherwise: Reviewed-by: Nikolay Borisov <kernel@kyup.com> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions 2016-05-11 6:38 ` Nikolay Borisov @ 2016-05-11 6:43 ` Nikolay Borisov 2016-05-11 9:06 ` Paolo 2016-05-11 9:28 ` [PATCH BUGFIX V4] " Paolo Valente 0 siblings, 2 replies; 29+ messages in thread From: Nikolay Borisov @ 2016-05-11 6:43 UTC (permalink / raw) To: Paolo Valente, Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On 05/11/2016 09:38 AM, Nikolay Borisov wrote: > > > On 05/11/2016 01:01 AM, Paolo Valente wrote: >> When a bio is cloned, the newly created bio must be associated with >> the same blkcg as the original bio (if BLK_CGROUP is enabled). If >> this operation is not performed, then the new bio is not associated >> with any group, and the group of the current task is returned when >> the group of the bio is requested. >> >> Depending on the cloning frequency, this may cause a large >> percentage of the bios belonging to a given group to be treated >> as if belonging to other groups (in most cases as if belonging to >> the root group). The expected group isolation may thereby be broken. >> >> This commit adds the missing association in bio-cloning functions. >> >> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> >> --- >> block/bio.c | 15 +++++++++++++++ >> fs/btrfs/extent_io.c | 6 ------ >> include/linux/bio.h | 3 +++ >> 3 files changed, 18 insertions(+), 6 deletions(-) >> > > Just for reference something like that was already proposed (and tested) > before, though it never got merged : > > https://www.redhat.com/archives/dm-devel/2016-March/msg00007.html > > So you might also want to patch __bio_clone_fast to also apply this for > dm backed devices. Right, to correct myself: You might want to move the association to __blk_clone_fast that way you are also covering dm devices as well as users of bio_clone_fast. > > Otherwise: > > Reviewed-by: Nikolay Borisov <kernel@kyup.com> > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions 2016-05-11 6:43 ` Nikolay Borisov @ 2016-05-11 9:06 ` Paolo 2016-05-11 9:28 ` [PATCH BUGFIX V4] " Paolo Valente 1 sibling, 0 replies; 29+ messages in thread From: Paolo @ 2016-05-11 9:06 UTC (permalink / raw) To: Nikolay Borisov, Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Il 11/05/2016 08:43, Nikolay Borisov ha scritto: > > > On 05/11/2016 09:38 AM, Nikolay Borisov wrote: >> >> >> On 05/11/2016 01:01 AM, Paolo Valente wrote: >>> When a bio is cloned, the newly created bio must be associated with >>> the same blkcg as the original bio (if BLK_CGROUP is enabled). If >>> this operation is not performed, then the new bio is not associated >>> with any group, and the group of the current task is returned when >>> the group of the bio is requested. >>> >>> Depending on the cloning frequency, this may cause a large >>> percentage of the bios belonging to a given group to be treated >>> as if belonging to other groups (in most cases as if belonging to >>> the root group). The expected group isolation may thereby be broken. >>> >>> This commit adds the missing association in bio-cloning functions. >>> >>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> >>> --- >>> block/bio.c | 15 +++++++++++++++ >>> fs/btrfs/extent_io.c | 6 ------ >>> include/linux/bio.h | 3 +++ >>> 3 files changed, 18 insertions(+), 6 deletions(-) >>> >> >> Just for reference something like that was already proposed (and tested) >> before, though it never got merged : >> >> https://www.redhat.com/archives/dm-devel/2016-March/msg00007.html >> >> So you might also want to patch __bio_clone_fast to also apply this for >> dm backed devices. > > Right, to correct myself: You might want to move the association to > __blk_clone_fast that way you are also covering dm devices as well as > users of bio_clone_fast. > Definitely (I didn't do that in the first place, because I worried only about doing the association when bio_clone_fast was certainly successful). I'm sending a fresh patch. Thanks, Paolo > >> >> Otherwise: >> >> Reviewed-by: Nikolay Borisov <kernel@kyup.com> >> ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-05-11 6:43 ` Nikolay Borisov 2016-05-11 9:06 ` Paolo @ 2016-05-11 9:28 ` Paolo Valente 2016-05-12 15:10 ` Tejun Heo 2016-05-13 20:42 ` Jeff Moyer 1 sibling, 2 replies; 29+ messages in thread From: Paolo Valente @ 2016-05-11 9:28 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Signed-off-by: Paolo Valente <paolo.valente@linaro.org> Reviewed-by: Nikolay Borisov <kernel@kyup.com> --- Tejun: I didn't add also your Ack, just because this version is slightly different from the one you acked. --- block/bio.c | 15 +++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 3 +++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 807d25e..89f517c 100644 --- a/block/bio.c +++ b/block/bio.c @@ -590,6 +590,8 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) bio->bi_rw = bio_src->bi_rw; bio->bi_iter = bio_src->bi_iter; bio->bi_io_vec = bio_src->bi_io_vec; + + bio_clone_blkcg_association(bio, bio_src); } EXPORT_SYMBOL(__bio_clone_fast); @@ -695,6 +697,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -2016,6 +2020,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index d247fc0..19f6739 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 6b7481f..16cbe59 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +static inline void bio_clone_blkcg_association(struct bio *dst, + struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-05-11 9:28 ` [PATCH BUGFIX V4] " Paolo Valente @ 2016-05-12 15:10 ` Tejun Heo 2016-05-13 20:42 ` Jeff Moyer 1 sibling, 0 replies; 29+ messages in thread From: Tejun Heo @ 2016-05-12 15:10 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On Wed, May 11, 2016 at 11:28:04AM +0200, Paolo Valente wrote: > When a bio is cloned, the newly created bio must be associated with > the same blkcg as the original bio (if BLK_CGROUP is enabled). If > this operation is not performed, then the new bio is not associated > with any group, and the group of the current task is returned when > the group of the bio is requested. > > Depending on the cloning frequency, this may cause a large > percentage of the bios belonging to a given group to be treated > as if belonging to other groups (in most cases as if belonging to > the root group). The expected group isolation may thereby be broken. > > This commit adds the missing association in bio-cloning functions. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > Reviewed-by: Nikolay Borisov <kernel@kyup.com> Acked-by: Tejun Heo <tj@kernel.org> It prolly should also have the following tags Fixes: da2f0f74cf7d ("Btrfs: add support for blkio controllers") Cc: stable@vger.kernel.org # v4.3+ Thanks. -- tejun ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-05-11 9:28 ` [PATCH BUGFIX V4] " Paolo Valente 2016-05-12 15:10 ` Tejun Heo @ 2016-05-13 20:42 ` Jeff Moyer 2016-06-07 7:09 ` Paolo 2016-07-26 16:14 ` Paolo Valente 1 sibling, 2 replies; 29+ messages in thread From: Jeff Moyer @ 2016-05-13 20:42 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Paolo Valente <paolo.valente@linaro.org> writes: > When a bio is cloned, the newly created bio must be associated with > the same blkcg as the original bio (if BLK_CGROUP is enabled). If > this operation is not performed, then the new bio is not associated > with any group, and the group of the current task is returned when > the group of the bio is requested. > > Depending on the cloning frequency, this may cause a large > percentage of the bios belonging to a given group to be treated > as if belonging to other groups (in most cases as if belonging to > the root group). The expected group isolation may thereby be broken. > > This commit adds the missing association in bio-cloning functions. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > Reviewed-by: Nikolay Borisov <kernel@kyup.com> I think this one's golden! Thanks, Paolo! Reviewed-by: Jeff Moyer <jmoyer@redhat.com> > --- > Tejun: I didn't add also your Ack, just because this version is slightly > different from the one you acked. > --- > block/bio.c | 15 +++++++++++++++ > fs/btrfs/extent_io.c | 6 ------ > include/linux/bio.h | 3 +++ > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 807d25e..89f517c 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -590,6 +590,8 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) > bio->bi_rw = bio_src->bi_rw; > bio->bi_iter = bio_src->bi_iter; > bio->bi_io_vec = bio_src->bi_io_vec; > + > + bio_clone_blkcg_association(bio, bio_src); > } > EXPORT_SYMBOL(__bio_clone_fast); > > @@ -695,6 +697,8 @@ integrity_clone: > } > } > > + bio_clone_blkcg_association(bio, bio_src); > + > return bio; > } > EXPORT_SYMBOL(bio_clone_bioset); > @@ -2016,6 +2020,17 @@ void bio_disassociate_task(struct bio *bio) > } > } > > +/** > + * bio_clone_blkcg_association - clone blkcg association from src to dst bio > + * @dst: destination bio > + * @src: source bio > + */ > +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) > +{ > + if (src->bi_css) > + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); > +} > + > #endif /* CONFIG_BLK_CGROUP */ > > static void __init biovec_init_slabs(void) > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index d247fc0..19f6739 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) > btrfs_bio->csum = NULL; > btrfs_bio->csum_allocated = NULL; > btrfs_bio->end_io = NULL; > - > -#ifdef CONFIG_BLK_CGROUP > - /* FIXME, put this into bio_clone_bioset */ > - if (bio->bi_css) > - bio_associate_blkcg(new, bio->bi_css); > -#endif > } > return new; > } > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 6b7481f..16cbe59 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); > int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); > int bio_associate_current(struct bio *bio); > void bio_disassociate_task(struct bio *bio); > +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); > #else /* CONFIG_BLK_CGROUP */ > static inline int bio_associate_blkcg(struct bio *bio, > struct cgroup_subsys_state *blkcg_css) { return 0; } > static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } > static inline void bio_disassociate_task(struct bio *bio) { } > +static inline void bio_clone_blkcg_association(struct bio *dst, > + struct bio *src) { return 0; } > #endif /* CONFIG_BLK_CGROUP */ > > #ifdef CONFIG_HIGHMEM ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-05-13 20:42 ` Jeff Moyer @ 2016-06-07 7:09 ` Paolo 2016-07-26 16:14 ` Paolo Valente 1 sibling, 0 replies; 29+ messages in thread From: Paolo @ 2016-06-07 7:09 UTC (permalink / raw) To: Jeff Moyer Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable Jens, are you picking this fix? Thanks, Paolo Il 13/05/2016 22:42, Jeff Moyer ha scritto: > Paolo Valente <paolo.valente@linaro.org> writes: > >> When a bio is cloned, the newly created bio must be associated with >> the same blkcg as the original bio (if BLK_CGROUP is enabled). If >> this operation is not performed, then the new bio is not associated >> with any group, and the group of the current task is returned when >> the group of the bio is requested. >> >> Depending on the cloning frequency, this may cause a large >> percentage of the bios belonging to a given group to be treated >> as if belonging to other groups (in most cases as if belonging to >> the root group). The expected group isolation may thereby be broken. >> >> This commit adds the missing association in bio-cloning functions. >> >> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> >> Reviewed-by: Nikolay Borisov <kernel@kyup.com> > > I think this one's golden! Thanks, Paolo! > > Reviewed-by: Jeff Moyer <jmoyer@redhat.com> > > >> --- >> Tejun: I didn't add also your Ack, just because this version is slightly >> different from the one you acked. >> --- >> block/bio.c | 15 +++++++++++++++ >> fs/btrfs/extent_io.c | 6 ------ >> include/linux/bio.h | 3 +++ >> 3 files changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/block/bio.c b/block/bio.c >> index 807d25e..89f517c 100644 >> --- a/block/bio.c >> +++ b/block/bio.c >> @@ -590,6 +590,8 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) >> bio->bi_rw = bio_src->bi_rw; >> bio->bi_iter = bio_src->bi_iter; >> bio->bi_io_vec = bio_src->bi_io_vec; >> + >> + bio_clone_blkcg_association(bio, bio_src); >> } >> EXPORT_SYMBOL(__bio_clone_fast); >> >> @@ -695,6 +697,8 @@ integrity_clone: >> } >> } >> >> + bio_clone_blkcg_association(bio, bio_src); >> + >> return bio; >> } >> EXPORT_SYMBOL(bio_clone_bioset); >> @@ -2016,6 +2020,17 @@ void bio_disassociate_task(struct bio *bio) >> } >> } >> >> +/** >> + * bio_clone_blkcg_association - clone blkcg association from src to dst bio >> + * @dst: destination bio >> + * @src: source bio >> + */ >> +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) >> +{ >> + if (src->bi_css) >> + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); >> +} >> + >> #endif /* CONFIG_BLK_CGROUP */ >> >> static void __init biovec_init_slabs(void) >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index d247fc0..19f6739 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -2686,12 +2686,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) >> btrfs_bio->csum = NULL; >> btrfs_bio->csum_allocated = NULL; >> btrfs_bio->end_io = NULL; >> - >> -#ifdef CONFIG_BLK_CGROUP >> - /* FIXME, put this into bio_clone_bioset */ >> - if (bio->bi_css) >> - bio_associate_blkcg(new, bio->bi_css); >> -#endif >> } >> return new; >> } >> diff --git a/include/linux/bio.h b/include/linux/bio.h >> index 6b7481f..16cbe59 100644 >> --- a/include/linux/bio.h >> +++ b/include/linux/bio.h >> @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); >> int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); >> int bio_associate_current(struct bio *bio); >> void bio_disassociate_task(struct bio *bio); >> +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); >> #else /* CONFIG_BLK_CGROUP */ >> static inline int bio_associate_blkcg(struct bio *bio, >> struct cgroup_subsys_state *blkcg_css) { return 0; } >> static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } >> static inline void bio_disassociate_task(struct bio *bio) { } >> +static inline void bio_clone_blkcg_association(struct bio *dst, >> + struct bio *src) { return 0; } >> #endif /* CONFIG_BLK_CGROUP */ >> >> #ifdef CONFIG_HIGHMEM ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-05-13 20:42 ` Jeff Moyer 2016-06-07 7:09 ` Paolo @ 2016-07-26 16:14 ` Paolo Valente 2016-07-26 17:01 ` kbuild test robot 2016-07-27 5:22 ` [PATCH BUGFIX V5] " Paolo Valente 1 sibling, 2 replies; 29+ messages in thread From: Paolo Valente @ 2016-07-26 16:14 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, jmoyer, Nikolay Borisov, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Fixes: da2f0f74cf7d ("Btrfs: add support for blkio controllers") Cc: stable@vger.kernel.org # v4.3+ Signed-off-by: Paolo Valente <paolo.valente@linaro.org> Reviewed-by: Nikolay Borisov <kernel@kyup.com> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> --- block/bio.c | 15 +++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 3 +++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 0e4aa42..4623869 100644 --- a/block/bio.c +++ b/block/bio.c @@ -579,6 +579,8 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) bio->bi_rw = bio_src->bi_rw; bio->bi_iter = bio_src->bi_iter; bio->bi_io_vec = bio_src->bi_io_vec; + + bio_clone_blkcg_association(bio, bio_src); } EXPORT_SYMBOL(__bio_clone_fast); @@ -684,6 +686,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -2005,6 +2009,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 75533ad..92fe3f8 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2696,12 +2696,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 9faebf7..c9403b7 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +static inline void bio_clone_blkcg_association(struct bio *dst, + struct bio *src) { return 0; } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions 2016-07-26 16:14 ` Paolo Valente @ 2016-07-26 17:01 ` kbuild test robot 2016-07-27 5:22 ` [PATCH BUGFIX V5] " Paolo Valente 1 sibling, 0 replies; 29+ messages in thread From: kbuild test robot @ 2016-07-26 17:01 UTC (permalink / raw) To: Paolo Valente Cc: kbuild-all, Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, jmoyer, Nikolay Borisov, Paolo Valente [-- Attachment #1: Type: text/plain, Size: 2594 bytes --] Hi, [auto build test WARNING on block/for-next] [also build test WARNING on v4.7 next-20160726] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Paolo-Valente/block-add-missing-group-association-in-bio-cloning-functions/20160727-005044 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next config: x86_64-randconfig-x015-201630 (attached as .config) compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from block/bio.c:20:0: include/linux/bio.h: In function 'bio_clone_blkcg_association': >> include/linux/bio.h:480:30: warning: 'return' with a value, in function returning void struct bio *src) { return 0; } ^ include/linux/bio.h:479:20: note: declared here static inline void bio_clone_blkcg_association(struct bio *dst, ^~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/return +480 include/linux/bio.h 464 void zero_fill_bio(struct bio *bio); 465 extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *); 466 extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int); 467 extern unsigned int bvec_nr_vecs(unsigned short idx); 468 469 #ifdef CONFIG_BLK_CGROUP 470 int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); 471 int bio_associate_current(struct bio *bio); 472 void bio_disassociate_task(struct bio *bio); 473 void bio_clone_blkcg_association(struct bio *dst, struct bio *src); 474 #else /* CONFIG_BLK_CGROUP */ 475 static inline int bio_associate_blkcg(struct bio *bio, 476 struct cgroup_subsys_state *blkcg_css) { return 0; } 477 static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } 478 static inline void bio_disassociate_task(struct bio *bio) { } 479 static inline void bio_clone_blkcg_association(struct bio *dst, > 480 struct bio *src) { return 0; } 481 #endif /* CONFIG_BLK_CGROUP */ 482 483 #ifdef CONFIG_HIGHMEM 484 /* 485 * remember never ever reenable interrupts between a bvec_kmap_irq and 486 * bvec_kunmap_irq! 487 */ 488 static inline char *bvec_kmap_irq(struct bio_vec *bvec, unsigned long *flags) --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/octet-stream, Size: 26155 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH BUGFIX V5] block: add missing group association in bio-cloning functions 2016-07-26 16:14 ` Paolo Valente 2016-07-26 17:01 ` kbuild test robot @ 2016-07-27 5:22 ` Paolo Valente 2016-07-27 14:45 ` Jens Axboe 1 sibling, 1 reply; 29+ messages in thread From: Paolo Valente @ 2016-07-27 5:22 UTC (permalink / raw) To: Jens Axboe, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, jmoyer, Nikolay Borisov, Paolo Valente When a bio is cloned, the newly created bio must be associated with the same blkcg as the original bio (if BLK_CGROUP is enabled). If this operation is not performed, then the new bio is not associated with any group, and the group of the current task is returned when the group of the bio is requested. Depending on the cloning frequency, this may cause a large percentage of the bios belonging to a given group to be treated as if belonging to other groups (in most cases as if belonging to the root group). The expected group isolation may thereby be broken. This commit adds the missing association in bio-cloning functions. Fixes: da2f0f74cf7d ("Btrfs: add support for blkio controllers") Cc: stable@vger.kernel.org # v4.3+ Signed-off-by: Paolo Valente <paolo.valente@linaro.org> Reviewed-by: Nikolay Borisov <kernel@kyup.com> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> --- Fixed a compilation issue reported by kbuild test robot --- block/bio.c | 15 +++++++++++++++ fs/btrfs/extent_io.c | 6 ------ include/linux/bio.h | 3 +++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/block/bio.c b/block/bio.c index 0e4aa42..4623869 100644 --- a/block/bio.c +++ b/block/bio.c @@ -579,6 +579,8 @@ void __bio_clone_fast(struct bio *bio, struct bio *bio_src) bio->bi_rw = bio_src->bi_rw; bio->bi_iter = bio_src->bi_iter; bio->bi_io_vec = bio_src->bi_io_vec; + + bio_clone_blkcg_association(bio, bio_src); } EXPORT_SYMBOL(__bio_clone_fast); @@ -684,6 +686,8 @@ integrity_clone: } } + bio_clone_blkcg_association(bio, bio_src); + return bio; } EXPORT_SYMBOL(bio_clone_bioset); @@ -2005,6 +2009,17 @@ void bio_disassociate_task(struct bio *bio) } } +/** + * bio_clone_blkcg_association - clone blkcg association from src to dst bio + * @dst: destination bio + * @src: source bio + */ +void bio_clone_blkcg_association(struct bio *dst, struct bio *src) +{ + if (src->bi_css) + WARN_ON(bio_associate_blkcg(dst, src->bi_css)); +} + #endif /* CONFIG_BLK_CGROUP */ static void __init biovec_init_slabs(void) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 75533ad..92fe3f8 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2696,12 +2696,6 @@ struct bio *btrfs_bio_clone(struct bio *bio, gfp_t gfp_mask) btrfs_bio->csum = NULL; btrfs_bio->csum_allocated = NULL; btrfs_bio->end_io = NULL; - -#ifdef CONFIG_BLK_CGROUP - /* FIXME, put this into bio_clone_bioset */ - if (bio->bi_css) - bio_associate_blkcg(new, bio->bi_css); -#endif } return new; } diff --git a/include/linux/bio.h b/include/linux/bio.h index 9faebf7..75fadd2 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -527,11 +527,14 @@ extern unsigned int bvec_nr_vecs(unsigned short idx); int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css); int bio_associate_current(struct bio *bio); void bio_disassociate_task(struct bio *bio); +void bio_clone_blkcg_association(struct bio *dst, struct bio *src); #else /* CONFIG_BLK_CGROUP */ static inline int bio_associate_blkcg(struct bio *bio, struct cgroup_subsys_state *blkcg_css) { return 0; } static inline int bio_associate_current(struct bio *bio) { return -ENOENT; } static inline void bio_disassociate_task(struct bio *bio) { } +static inline void bio_clone_blkcg_association(struct bio *dst, + struct bio *src) { } #endif /* CONFIG_BLK_CGROUP */ #ifdef CONFIG_HIGHMEM -- 1.9.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX V5] block: add missing group association in bio-cloning functions 2016-07-27 5:22 ` [PATCH BUGFIX V5] " Paolo Valente @ 2016-07-27 14:45 ` Jens Axboe 0 siblings, 0 replies; 29+ messages in thread From: Jens Axboe @ 2016-07-27 14:45 UTC (permalink / raw) To: Paolo Valente, Tejun Heo Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable, jmoyer, Nikolay Borisov On 07/26/2016 11:22 PM, Paolo Valente wrote: > When a bio is cloned, the newly created bio must be associated with > the same blkcg as the original bio (if BLK_CGROUP is enabled). If > this operation is not performed, then the new bio is not associated > with any group, and the group of the current task is returned when > the group of the bio is requested. > > Depending on the cloning frequency, this may cause a large > percentage of the bios belonging to a given group to be treated > as if belonging to other groups (in most cases as if belonging to > the root group). The expected group isolation may thereby be broken. > > This commit adds the missing association in bio-cloning functions. Added for 4.8, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH BUGFIX] block: add missing group association in bio_split 2016-05-06 20:45 [PATCH BUGFIX] block: add missing group association in bio_split Paolo Valente 2016-05-09 14:19 ` Jeff Moyer @ 2016-05-09 15:20 ` Tejun Heo 1 sibling, 0 replies; 29+ messages in thread From: Tejun Heo @ 2016-05-09 15:20 UTC (permalink / raw) To: Paolo Valente Cc: Jens Axboe, linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie, stable On Fri, May 06, 2016 at 10:45:12PM +0200, Paolo Valente wrote: > When a bio is split, the newly created bio must be associated with the > same blkcg as the original bio (if BLK_CGROUP is enabled). If this > operation is not performed, then the new bio is not associated with > any group, and the group of the current task is returned when the > group of the bio is requested. > > Depending on the frequency of splits, this may cause a large > percentage of the bios belonging to a given group to be treated as if > belonging to other groups (in most cases as if belonging to the root > group). The expected group isolation may thereby be then broken. > > This commit adds the missing association in bio_split. > > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> Acked-by: Tejun Heo <tj@kernel.org> > --- > block/bio.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/block/bio.c b/block/bio.c > index 807d25e..c4a3834 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1811,6 +1811,11 @@ struct bio *bio_split(struct bio *bio, int sectors, > > bio_advance(bio, split->bi_iter.bi_size); > > +#ifdef CONFIG_BLK_CGROUP > + if (bio->bi_css) > + bio_associate_blkcg(split, bio->bi_css); > +#endif And yeah, we need to encapsulate this better to avoid scattering CONFIG_BLK_CGROUP but that's for another patch. Thanks. -- tejun ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-07-27 14:45 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-06 20:45 [PATCH BUGFIX] block: add missing group association in bio_split Paolo Valente 2016-05-09 14:19 ` Jeff Moyer 2016-05-09 14:35 ` Jeff Moyer 2016-05-09 14:39 ` Paolo 2016-05-09 14:55 ` Jens Axboe 2016-05-09 14:56 ` Mark Brown 2016-05-09 17:58 ` Jens Axboe 2016-05-10 9:08 ` Paolo Valente 2016-05-10 16:12 ` Jeff Moyer 2016-05-10 17:08 ` Paolo 2016-05-10 20:23 ` [PATCH BUGFIX] block: add missing group association in bio-cloning functions Paolo Valente 2016-05-10 20:44 ` Tejun Heo 2016-05-10 21:02 ` [PATCH BUGFIX V2] " Paolo Valente 2016-05-10 21:03 ` Tejun Heo 2016-05-10 21:34 ` Jeff Moyer 2016-05-10 21:44 ` Paolo 2016-05-10 22:01 ` [PATCH BUGFIX V3] " Paolo Valente 2016-05-11 6:38 ` Nikolay Borisov 2016-05-11 6:43 ` Nikolay Borisov 2016-05-11 9:06 ` Paolo 2016-05-11 9:28 ` [PATCH BUGFIX V4] " Paolo Valente 2016-05-12 15:10 ` Tejun Heo 2016-05-13 20:42 ` Jeff Moyer 2016-06-07 7:09 ` Paolo 2016-07-26 16:14 ` Paolo Valente 2016-07-26 17:01 ` kbuild test robot 2016-07-27 5:22 ` [PATCH BUGFIX V5] " Paolo Valente 2016-07-27 14:45 ` Jens Axboe 2016-05-09 15:20 ` [PATCH BUGFIX] block: add missing group association in bio_split Tejun Heo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).