Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1396115 > unrolled thread
| Started by | Paolo Valente <paolo.valente@linaro.org> |
|---|---|
| First post | 2016-05-06 23:30 +0200 |
| Last post | 2016-05-09 17:30 +0200 |
| Articles | 4 on this page of 24 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH BUGFIX] block: add missing group association in bio_split Paolo Valente <paolo.valente@linaro.org> - 2016-05-06 23:30 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Jeff Moyer <jmoyer@redhat.com> - 2016-05-09 16:20 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Jeff Moyer <jmoyer@redhat.com> - 2016-05-09 16:40 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Paolo <paolo.valente@linaro.org> - 2016-05-09 16:40 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Mark Brown <broonie@kernel.org> - 2016-05-09 17:00 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Jens Axboe <axboe@kernel.dk> - 2016-05-09 20:00 +0200
[PATCH BUGFIX] block: add missing group association in bio_split Paolo Valente <paolo.valente@linaro.org> - 2016-05-10 11:10 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Jeff Moyer <jmoyer@redhat.com> - 2016-05-10 18:20 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Paolo <paolo.valente@linaro.org> - 2016-05-10 19:10 +0200
[PATCH BUGFIX] block: add missing group association in bio-cloning functions Paolo Valente <paolo.valente@linaro.org> - 2016-05-10 22:30 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio-cloning functions Tejun Heo <tj@kernel.org> - 2016-05-10 22:50 +0200
Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions Tejun Heo <tj@kernel.org> - 2016-05-10 23:10 +0200
[PATCH BUGFIX V2] block: add missing group association in bio-cloning functions Paolo Valente <paolo.valente@linaro.org> - 2016-05-10 23:10 +0200
Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions Jeff Moyer <jmoyer@redhat.com> - 2016-05-10 23:40 +0200
Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions Paolo <paolo.valente@linaro.org> - 2016-05-10 23:50 +0200
[PATCH BUGFIX V3] block: add missing group association in bio-cloning functions Paolo Valente <paolo.valente@linaro.org> - 2016-05-11 00:10 +0200
Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions Nikolay Borisov <kernel@kyup.com> - 2016-05-11 08:40 +0200
Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions Nikolay Borisov <kernel@kyup.com> - 2016-05-11 08:50 +0200
Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions Paolo <paolo.valente@linaro.org> - 2016-05-11 11:10 +0200
[PATCH BUGFIX V4] block: add missing group association in bio-cloning functions Paolo Valente <paolo.valente@linaro.org> - 2016-05-11 11:30 +0200
Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions Tejun Heo <tj@kernel.org> - 2016-05-12 17:30 +0200
Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions Jeff Moyer <jmoyer@redhat.com> - 2016-05-13 22:50 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Jens Axboe <axboe@kernel.dk> - 2016-05-09 17:00 +0200
Re: [PATCH BUGFIX] block: add missing group association in bio_split Tejun Heo <tj@kernel.org> - 2016-05-09 17:30 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-05-12 17:30 +0200 |
| Subject | Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions |
| Message-ID | <ry0IN-eN-3@gated-at.bofh.it> |
| In reply to | #1398841 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jeff Moyer <jmoyer@redhat.com> |
|---|---|
| Date | 2016-05-13 22:50 +0200 |
| Subject | Re: [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions |
| Message-ID | <rysc3-2oB-15@gated-at.bofh.it> |
| In reply to | #1398841 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jens Axboe <axboe@kernel.dk> |
|---|---|
| Date | 2016-05-09 17:00 +0200 |
| Message-ID | <rwUP9-7Fi-17@gated-at.bofh.it> |
| In reply to | #1397101 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2016-05-09 17:30 +0200 |
| Message-ID | <rwVi9-8if-1@gated-at.bofh.it> |
| In reply to | #1396115 |
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
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web