2011-05-11 15:58:17

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH] blk-throttle: fix a typo in blk_throtl_init()

s/td/tg/

Signed-off-by: Namhyung Kim <[email protected]>
Cc: Vivek Goyal <[email protected]>
---
block/blk-throttle.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 0475a22a420d..e7986b8a0085 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1072,7 +1072,7 @@ int blk_throtl_init(struct request_queue *q)
/* Practically unlimited BW */
tg->bps[0] = tg->bps[1] = -1;
tg->iops[0] = tg->iops[1] = -1;
- td->limits_changed = false;
+ tg->limits_changed = false;

/*
* Set root group reference to 2. One reference will be dropped when
--
1.7.5


2011-05-11 15:38:18

by Vivek Goyal

[permalink] [raw]
Subject: Re: [PATCH] blk-throttle: fix a typo in blk_throtl_init()

On Wed, May 11, 2011 at 10:01:02PM +0900, Namhyung Kim wrote:
> s/td/tg/
>
> Signed-off-by: Namhyung Kim <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> ---
> block/blk-throttle.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 0475a22a420d..e7986b8a0085 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -1072,7 +1072,7 @@ int blk_throtl_init(struct request_queue *q)
> /* Practically unlimited BW */
> tg->bps[0] = tg->bps[1] = -1;
> tg->iops[0] = tg->iops[1] = -1;
> - td->limits_changed = false;
> + tg->limits_changed = false;
>

Thanks Namhyung. I noticed it recently too. There is one more such typo
in throtl_find_alloc_tg(). Can you fix that too and repost the patch.

Thanks
Vivek

2011-05-11 16:16:27

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH] blk-throttle: fix typos on struct throtl_grp init code

s/td/tg/

Signed-off-by: Namhyung Kim <[email protected]>
Cc: Vivek Goyal <[email protected]>
---
block/blk-throttle.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 0475a22a420d..d1e317e0fc34 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -201,7 +201,7 @@ static struct throtl_grp * throtl_find_alloc_tg(struct throtl_data *td,
RB_CLEAR_NODE(&tg->rb_node);
bio_list_init(&tg->bio_lists[0]);
bio_list_init(&tg->bio_lists[1]);
- td->limits_changed = false;
+ tg->limits_changed = false;

/*
* Take the initial reference that will be released on destroy
@@ -1072,7 +1072,7 @@ int blk_throtl_init(struct request_queue *q)
/* Practically unlimited BW */
tg->bps[0] = tg->bps[1] = -1;
tg->iops[0] = tg->iops[1] = -1;
- td->limits_changed = false;
+ tg->limits_changed = false;

/*
* Set root group reference to 2. One reference will be dropped when
--
1.7.5

2011-05-11 16:17:13

by Vivek Goyal

[permalink] [raw]
Subject: Re: [PATCH] blk-throttle: fix typos on struct throtl_grp init code

On Wed, May 11, 2011 at 10:20:45PM +0900, Namhyung Kim wrote:
> s/td/tg/

I think jens would like to have little more changelog than that.

Otherwise looks good to me.

Acked-by: Vivek Goyal <[email protected]>

Vivek

>
> Signed-off-by: Namhyung Kim <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> ---
> block/blk-throttle.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 0475a22a420d..d1e317e0fc34 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -201,7 +201,7 @@ static struct throtl_grp * throtl_find_alloc_tg(struct throtl_data *td,
> RB_CLEAR_NODE(&tg->rb_node);
> bio_list_init(&tg->bio_lists[0]);
> bio_list_init(&tg->bio_lists[1]);
> - td->limits_changed = false;
> + tg->limits_changed = false;
>
> /*
> * Take the initial reference that will be released on destroy
> @@ -1072,7 +1072,7 @@ int blk_throtl_init(struct request_queue *q)
> /* Practically unlimited BW */
> tg->bps[0] = tg->bps[1] = -1;
> tg->iops[0] = tg->iops[1] = -1;
> - td->limits_changed = false;
> + tg->limits_changed = false;
>
> /*
> * Set root group reference to 2. One reference will be dropped when
> --
> 1.7.5

2011-05-11 16:03:55

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH] blk-throttle: fix typos on struct throtl_grp init code

2011-05-11 (수), 09:30 -0400, Vivek Goyal:
> On Wed, May 11, 2011 at 10:20:45PM +0900, Namhyung Kim wrote:
> > s/td/tg/
>
> I think jens would like to have little more changelog than that.
>

OK, let me clarify this first: My first patch was trivial but second one
seems not. AFAICS it could affect when the bps/iops bandwidth change
applies - maybe delayed to next @tg->disptime? - if there are concurrent
cgroup init and limit change tasks, right? Or do you have something need
to be included in the changelog?


> Otherwise looks good to me.
>
> Acked-by: Vivek Goyal <[email protected]>
>
> Vivek

Thank you for the review and the comment.


--
Regards,
Namhyung Kim

2011-05-11 16:23:52

by Vivek Goyal

[permalink] [raw]
Subject: Re: [PATCH] blk-throttle: fix typos on struct throtl_grp init code

On Wed, May 11, 2011 at 11:14:02PM +0900, Namhyung Kim wrote:
> 2011-05-11 (수), 09:30 -0400, Vivek Goyal:
> > On Wed, May 11, 2011 at 10:20:45PM +0900, Namhyung Kim wrote:
> > > s/td/tg/
> >
> > I think jens would like to have little more changelog than that.
> >
>
> OK, let me clarify this first: My first patch was trivial but second one
> seems not. AFAICS it could affect when the bps/iops bandwidth change
> applies - maybe delayed to next @tg->disptime? - if there are concurrent
> cgroup init and limit change tasks, right? Or do you have something need
> to be included in the changelog?

Just say that when a new group is initialized, set tg->limits_changed =
false instead of setting td->limits_changed to false.

We do memset 0 on all newly allocated objects to this might not be
required at all.

So how about just getting rid of td->limits_changed assignments and
not do explicit tg->limits_changed as that is implicit in zeroing of
newly allocated object.

Thanks
Vivek


>
>
> > Otherwise looks good to me.
> >
> > Acked-by: Vivek Goyal <[email protected]>
> >
> > Vivek
>
> Thank you for the review and the comment.
>
>
> --
> Regards,
> Namhyung Kim
>

2011-05-11 15:38:48

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH] blk-throttle: fix typos on struct throtl_grp init code

2011-05-11 (수), 10:28 -0400, Vivek Goyal:
> On Wed, May 11, 2011 at 11:14:02PM +0900, Namhyung Kim wrote:
> > 2011-05-11 (수), 09:30 -0400, Vivek Goyal:
> > > On Wed, May 11, 2011 at 10:20:45PM +0900, Namhyung Kim wrote:
> > > > s/td/tg/
> > >
> > > I think jens would like to have little more changelog than that.
> > >
> >
> > OK, let me clarify this first: My first patch was trivial but second one
> > seems not. AFAICS it could affect when the bps/iops bandwidth change
> > applies - maybe delayed to next @tg->disptime? - if there are concurrent
> > cgroup init and limit change tasks, right? Or do you have something need
> > to be included in the changelog?
>
> Just say that when a new group is initialized, set tg->limits_changed =
> false instead of setting td->limits_changed to false.
>
> We do memset 0 on all newly allocated objects to this might not be
> required at all.
>
> So how about just getting rid of td->limits_changed assignments and
> not do explicit tg->limits_changed as that is implicit in zeroing of
> newly allocated object.
>
> Thanks
> Vivek
>

Sounds good to me. I'll cook a new patch :)

Thanks.


--
Regards,
Namhyung Kim