Sender: Tejun Heo <htejun@gmail.com>
Date:   Mon, 27 Feb 2023 08:27:19 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Breno Leitao <leitao@debian.org>
Cc:     axboe@kernel.dk, cgroups@vger.kernel.org,
        linux-block@vger.kernel.org, hch@lst.de, josef@toxicpanda.com,
        aherrmann@suse.de, mkoutny@suse.com, linux-kernel@vger.kernel.org,
        leit@fb.com
Subject: Re: [PATCH v3] blk-iocost: Pass gendisk to ioc_refresh_params
Message-ID: <Y/z2B+153X0PYpjO@slm.duckdns.org>
References: <20230227151252.1411499-1-leitao@debian.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230227151252.1411499-1-leitao@debian.org>
Precedence: bulk

Hello,

A couple minor nitpicks:

Can you please add a short comment here too saying that @ioc->rqos.disk
isn't initialized yet when this function is called from the init path?

> +static int ioc_autop_idx(struct ioc *ioc, struct gendisk *disk)
>  {
>  	int idx = ioc->autop_idx;
>  	const struct ioc_params *p = &autop[idx];
> @@ -808,11 +808,11 @@ static int ioc_autop_idx(struct ioc *ioc)
>  	u64 now_ns;
>  
>  	/* rotational? */
> -	if (!blk_queue_nonrot(ioc->rqos.disk->queue))
> +	if (!blk_queue_nonrot(disk->queue))
>  		return AUTOP_HDD;
>  
>  	/* handle SATA SSDs w/ broken NCQ */
> -	if (blk_queue_depth(ioc->rqos.disk->queue) == 1)
> +	if (blk_queue_depth(disk->queue) == 1)
>  		return AUTOP_SSD_QD1;
>  
>  	/* use one of the normal ssd sets */
> @@ -901,14 +901,19 @@ static void ioc_refresh_lcoefs(struct ioc *ioc)
>  		    &c[LCOEF_WPAGE], &c[LCOEF_WSEQIO], &c[LCOEF_WRANDIO]);
>  }
>  
> -static bool ioc_refresh_params(struct ioc *ioc, bool force)
> +/*
> + * struct gendisk is required as an argument because ioc->rqos.disk
> + * might not be properly initialized
> + */

Here too, let's explicitly say when it's not initialized.

> +static bool _ioc_refresh_params(struct ioc *ioc, bool force,
> +				struct gendisk *disk)

Given that __ are about an order of magnitude more common in the kernel,
would you mind renaming it to __ioc_refresh_params() or e.g.
ioc_refresh_params_disk()?

Please feel free to add

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun