Re: [PATCH v3] blk-iocost: Pass gendisk to ioc_refresh_params

[Tejun Heo <tj@kernel.org>]

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