| Message ID | 1462958884-9457-1-git-send-email-paolo.valente@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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