[PATCH BUGFIX] block: add missing group association in bio_split

[PATCH BUGFIX] block: add missing group association in bio_split

[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

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Jeff Moyer]

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

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Jeff Moyer]

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

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Paolo]

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
>

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Jens Axboe]

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

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Mark Brown]

[-- 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 --]

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Jens Axboe]

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

Re: [PATCH BUGFIX] block: add missing group association in bio_split

[Tejun Heo]

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