Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1396115 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [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] 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 1 of 2  [1] 2  Next page →

#1396115 — [PATCH BUGFIX] block: add missing group association in 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(+)

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

[toc] | [next] | [standalone]

#1397084

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

[toc] | [prev] | [next] | [standalone]

#1397095

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

[toc] | [prev] | [next] | [standalone]

#1397101

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
>

[toc] | [prev] | [next] | [standalone]

#1397117

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1397235

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

[toc] | [prev] | [next] | [standalone]

#1397872

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

[toc] | [prev] | [next] | [standalone]

#1398295

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

[toc] | [prev] | [next] | [standalone]

#1398331

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

[toc] | [prev] | [next] | [standalone]

#1398492 — [PATCH BUGFIX] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398514 — Re: [PATCH BUGFIX] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398527 — Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398531 — [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398536 — Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398546 — Re: [PATCH BUGFIX V2] block: add missing group association in bio-cloning functions

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
>

[toc] | [prev] | [next] | [standalone]

#1398551 — [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398714 — Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions

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>

[toc] | [prev] | [next] | [standalone]

#1398715 — Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398808 — Re: [PATCH BUGFIX V3] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

#1398841 — [PATCH BUGFIX V4] block: add missing group association in bio-cloning functions

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web