Re: [PATCH 17/17] block, bfq: remove unnecessary local variable __bfqq in bfq_setup_merge

[Kemeng Shi]

on 2/23/2023 1:48 PM, Dan Carpenter wrote:
> Hi Kemeng,
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kemeng-Shi/block-bfq-properly-mark-bfqq-remained-idle/20230219-104312
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> patch link:    https://lore.kernel.org/r/20230219104309.1511562-18-shikemeng%40huaweicloud.com
> patch subject: [PATCH 17/17] block, bfq: remove unnecessary local variable __bfqq in bfq_setup_merge
> config: openrisc-randconfig-m041-20230219 (https://download.01.org/0day-ci/archive/20230220/202302200841.9zinyY7i-lkp@xxxxxxxxx/config)
> compiler: or1k-linux-gcc (GCC) 12.1.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> | Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> | Link: https://lore.kernel.org/r/202302200841.9zinyY7i-lkp@xxxxxxxxx/
> 
> New smatch warnings:
> block/bfq-iosched.c:2785 bfq_setup_merge() error: we previously assumed 'new_bfqq' could be null (see line 2766)
> 
> Old smatch warnings:
> block/bfq-iosched.c:6195 __bfq_insert_request() warn: variable dereferenced before check 'bfqq' (see line 6191)
> 
> vim +/new_bfqq +2785 block/bfq-iosched.c
> 
> 36eca894832351 Arianna Avanzini 2017-04-12  2751  static struct bfq_queue *
> 36eca894832351 Arianna Avanzini 2017-04-12  2752  bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
> 36eca894832351 Arianna Avanzini 2017-04-12  2753  {
> 36eca894832351 Arianna Avanzini 2017-04-12  2754  	int process_refs, new_process_refs;
> 36eca894832351 Arianna Avanzini 2017-04-12  2755  
> 36eca894832351 Arianna Avanzini 2017-04-12  2756  	/*
> 36eca894832351 Arianna Avanzini 2017-04-12  2757  	 * If there are no process references on the new_bfqq, then it is
> 36eca894832351 Arianna Avanzini 2017-04-12  2758  	 * unsafe to follow the ->new_bfqq chain as other bfqq's in the chain
> 36eca894832351 Arianna Avanzini 2017-04-12  2759  	 * may have dropped their last reference (not just their last process
> 36eca894832351 Arianna Avanzini 2017-04-12  2760  	 * reference).
> 36eca894832351 Arianna Avanzini 2017-04-12  2761  	 */
> 36eca894832351 Arianna Avanzini 2017-04-12  2762  	if (!bfqq_process_refs(new_bfqq))
> 36eca894832351 Arianna Avanzini 2017-04-12  2763  		return NULL;
> 36eca894832351 Arianna Avanzini 2017-04-12  2764  
> 36eca894832351 Arianna Avanzini 2017-04-12  2765  	/* Avoid a circular list and skip interim queue merges. */
> 114533e1e26a36 Kemeng Shi       2023-02-19 @2766  	while ((new_bfqq = new_bfqq->new_bfqq)) {
> 114533e1e26a36 Kemeng Shi       2023-02-19  2767  		if (new_bfqq == bfqq)
> 36eca894832351 Arianna Avanzini 2017-04-12  2768  			return NULL;
> 36eca894832351 Arianna Avanzini 2017-04-12  2769  	}
> 
> This now loops until new_bfqq is NULL.
> 
> 36eca894832351 Arianna Avanzini 2017-04-12  2770  
> 36eca894832351 Arianna Avanzini 2017-04-12  2771  	process_refs = bfqq_process_refs(bfqq);
> 36eca894832351 Arianna Avanzini 2017-04-12  2772  	new_process_refs = bfqq_process_refs(new_bfqq);
> 
> What?Sorry, I didn't expect that there is a behavior change. I will drop
this one in next version. Thanks.

-- 
Best wishes
Kemeng Shi