linux-kernel - Re: [PATCH v3] blk-iocost: Pass gendisk to ioc_refresh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y/z2B+153X0PYpjO@slm.duckdns.org>
Date:   Mon, 27 Feb 2023 08:27:19 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Breno Leitao <leitao@...ian.org>
Cc:     axboe@...nel.dk, cgroups@...r.kernel.org,
        linux-block@...r.kernel.org, hch@....de, josef@...icpanda.com,
        aherrmann@...e.de, mkoutny@...e.com, linux-kernel@...r.kernel.org,
        leit@...com
Subject: Re: [PATCH v3] blk-iocost: Pass gendisk to ioc_refresh_params

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@...nel.org>

Thanks.

-- 
tejun