Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 31 Jan 2020 11:20:59 +0100
From:   Oleksandr Natalenko <oleksandr@natalenko.name>
To:     Paolo Valente <paolo.valente@linaro.org>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, bfq-iosched@googlegroups.com,
        patdung100@gmail.com, cevich@redhat.com
Subject: Re: [PATCH BUGFIX 3/6] block, bfq: get extra ref to prevent a queue
 from being freed during a group move
In-Reply-To: <20200131092409.10867-4-paolo.valente@linaro.org>
References: <20200131092409.10867-1-paolo.valente@linaro.org>
 <20200131092409.10867-4-paolo.valente@linaro.org>
User-Agent: Roundcube Webmail/1.4.2
Message-ID: <784c55c0f37a1a448c31e73e28bef6f8@natalenko.name>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello.

On 31.01.2020 10:24, Paolo Valente wrote:
> In bfq_bfqq_move(), the bfq_queue, say Q, to be moved to a new group
> may happen to be deactivated in the scheduling data structures of the
> source group (and then activated in the destination group). If Q is
> referred only by the data structures in the source group when the
> deactivation happens, then Q is freed upon the deactivation.
> 
> This commit addresses this issue by getting an extra reference before
> the possible deactivation, and releasing this extra reference after Q
> has been moved.
> 
> Tested-by: Chris Evich <cevich@redhat.com>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
>  block/bfq-cgroup.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index e1419edde2ec..8ab7f18ff8cb 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -651,6 +651,12 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct
> bfq_queue *bfqq,
>  		bfq_bfqq_expire(bfqd, bfqd->in_service_queue,
>  				false, BFQQE_PREEMPTED);
> 
> +	/*
> +	 * get extra reference to prevent bfqq from being freed in
> +	 * next possible deactivate
> +	 */
> +	bfqq->ref++;

Shouldn't this be hidden under some macro (bfq_get_queue_ref(), for 
instance) and also converted from int into refcount_t?

> +
>  	if (bfq_bfqq_busy(bfqq))
>  		bfq_deactivate_bfqq(bfqd, bfqq, false, false);
>  	else if (entity->on_st)
> @@ -670,6 +676,8 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct
> bfq_queue *bfqq,
> 
>  	if (!bfqd->in_service_queue && !bfqd->rq_in_driver)
>  		bfq_schedule_dispatch(bfqd);
> +	/* release extra ref taken above */
> +	bfq_put_queue(bfqq);
>  }
> 
>  /**

-- 
   Oleksandr Natalenko (post-factum)