| Message ID | 1462567512-3007-1-git-send-email-paolo.valente@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 > -- 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
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.
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.
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.
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.
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);
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(+)