Re: [PATCH v2 1/1] Don't update group weights when on service tree.

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Feb 11, 2011 at 01:09:03PM -0800, Justin TerAvest wrote:
> For version 2, I took Vivek's advice and made sure we update the group
> weight from cfq_group_service_tree_add().
> 
> If a weight was updated while a group is on the service tree, the
> calculation for the total weight of the service tree can be adjusted
> improperly, which either leads to bad service tree weights, or
> potentially crashes (if total_weight becomes 0).
> 
> This patch defers updates to the weight until a group is off the service
> tree.
> 
> Signed-off-by: Justin TerAvest <teravest@google.com>
> ---

Thanks Justin. Looks good to me.

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

>  block/cfq-iosched.c |   57 ++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 501ffdf..868f9cf 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -179,6 +179,8 @@ struct cfq_group {
>  	/* group service_tree key */
>  	u64 vdisktime;
>  	unsigned int weight;
> +	unsigned int new_weight;
> +	bool needs_update;
>  
>  	/* number of cfqq currently on this group */
>  	int nr_cfqq;
> @@ -863,7 +865,27 @@ __cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
>  }
>  
>  static void
> -cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
> +cfq_update_group_weight(struct cfq_group *cfqg)
> +{
> +	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
> +	if (cfqg->needs_update) {
> +		cfqg->weight = cfqg->new_weight;
> +		cfqg->needs_update = false;
> +	}
> +}
> +
> +static void
> +cfq_group_service_tree_add(struct cfq_rb_root *st, struct cfq_group *cfqg)
> +{
> +	BUG_ON(!RB_EMPTY_NODE(&cfqg->rb_node));
> +
> +	cfq_update_group_weight(cfqg);
> +	__cfq_group_service_tree_add(st, cfqg);
> +	st->total_weight += cfqg->weight;
> +}
> +
> +static void
> +cfq_group_notify_queue_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  {
>  	struct cfq_rb_root *st = &cfqd->grp_service_tree;
>  	struct cfq_group *__cfqg;
> @@ -884,13 +906,19 @@ cfq_group_service_tree_add(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  		cfqg->vdisktime = __cfqg->vdisktime + CFQ_IDLE_DELAY;
>  	} else
>  		cfqg->vdisktime = st->min_vdisktime;
> +	cfq_group_service_tree_add(st, cfqg);
> +}
>  
> -	__cfq_group_service_tree_add(st, cfqg);
> -	st->total_weight += cfqg->weight;
> +static void
> +cfq_group_service_tree_del(struct cfq_rb_root *st, struct cfq_group *cfqg)
> +{
> +	st->total_weight -= cfqg->weight;
> +	if (!RB_EMPTY_NODE(&cfqg->rb_node))
> +		cfq_rb_erase(&cfqg->rb_node, st);
>  }
>  
>  static void
> -cfq_group_service_tree_del(struct cfq_data *cfqd, struct cfq_group *cfqg)
> +cfq_group_notify_queue_del(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  {
>  	struct cfq_rb_root *st = &cfqd->grp_service_tree;
>  
> @@ -902,9 +930,7 @@ cfq_group_service_tree_del(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  		return;
>  
>  	cfq_log_cfqg(cfqd, cfqg, "del_from_rr group");
> -	st->total_weight -= cfqg->weight;
> -	if (!RB_EMPTY_NODE(&cfqg->rb_node))
> -		cfq_rb_erase(&cfqg->rb_node, st);
> +	cfq_group_service_tree_del(st, cfqg);
>  	cfqg->saved_workload_slice = 0;
>  	cfq_blkiocg_update_dequeue_stats(&cfqg->blkg, 1);
>  }
> @@ -952,9 +978,10 @@ static void cfq_group_served(struct cfq_data *cfqd, struct cfq_group *cfqg,
>  		charge = cfqq->allocated_slice;
>  
>  	/* Can't update vdisktime while group is on service tree */
> -	cfq_rb_erase(&cfqg->rb_node, st);
> +	cfq_group_service_tree_del(st, cfqg);
>  	cfqg->vdisktime += cfq_scale_slice(charge, cfqg);
> -	__cfq_group_service_tree_add(st, cfqg);
> +	/* If a new weight was requested, update now, off tree */
> +	cfq_group_service_tree_add(st, cfqg);
>  
>  	/* This group is being expired. Save the context */
>  	if (time_after(cfqd->workload_expires, jiffies)) {
> @@ -985,7 +1012,9 @@ static inline struct cfq_group *cfqg_of_blkg(struct blkio_group *blkg)
>  void cfq_update_blkio_group_weight(void *key, struct blkio_group *blkg,
>  					unsigned int weight)
>  {
> -	cfqg_of_blkg(blkg)->weight = weight;
> +	struct cfq_group *cfqg = cfqg_of_blkg(blkg);
> +	cfqg->new_weight = weight;
> +	cfqg->needs_update = true;
>  }
>  
>  static struct cfq_group *
> @@ -1194,7 +1223,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>  		/* Move this cfq to root group */
>  		cfq_log_cfqq(cfqd, cfqq, "moving to root group");
>  		if (!RB_EMPTY_NODE(&cfqq->rb_node))
> -			cfq_group_service_tree_del(cfqd, cfqq->cfqg);
> +			cfq_group_notify_queue_del(cfqd, cfqq->cfqg);
>  		cfqq->orig_cfqg = cfqq->cfqg;
>  		cfqq->cfqg = &cfqd->root_group;
>  		cfqd->root_group.ref++;
> @@ -1204,7 +1233,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>  		/* cfqq is sequential now needs to go to its original group */
>  		BUG_ON(cfqq->cfqg != &cfqd->root_group);
>  		if (!RB_EMPTY_NODE(&cfqq->rb_node))
> -			cfq_group_service_tree_del(cfqd, cfqq->cfqg);
> +			cfq_group_notify_queue_del(cfqd, cfqq->cfqg);
>  		cfq_put_cfqg(cfqq->cfqg);
>  		cfqq->cfqg = cfqq->orig_cfqg;
>  		cfqq->orig_cfqg = NULL;
> @@ -1284,7 +1313,7 @@ static void cfq_service_tree_add(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>  	service_tree->count++;
>  	if ((add_front || !new_cfqq) && !group_changed)
>  		return;
> -	cfq_group_service_tree_add(cfqd, cfqq->cfqg);
> +	cfq_group_notify_queue_add(cfqd, cfqq->cfqg);
>  }
>  
>  static struct cfq_queue *
> @@ -1395,7 +1424,7 @@ static void cfq_del_cfqq_rr(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>  		cfqq->p_root = NULL;
>  	}
>  
> -	cfq_group_service_tree_del(cfqd, cfqq->cfqg);
> +	cfq_group_notify_queue_del(cfqd, cfqq->cfqg);
>  	BUG_ON(!cfqd->busy_queues);
>  	cfqd->busy_queues--;
>  }
> -- 
> 1.7.3.1