Re: [PATCH v2] nvme: fix command effects log lifetime for multipath heads

[Christoph Hellwig <hch@lst.de>]

On Mon, Aug 31, 2026 at 03:57:16PM +0800, Yao Sang wrote:
> -static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
> -				struct nvme_effects_log **log)
> +static int nvme_read_effects_log(struct nvme_ctrl *ctrl, u8 csi,
> +				 struct nvme_effects_log **log)

What is the reason for splitting this function?  It might or
might now be useful, but it distracts a bit from the actual
bug fix.

> -static int nvme_init_effects_log(struct nvme_ctrl *ctrl,
> -		u8 csi, struct nvme_effects_log **log)
> +static void nvme_init_known_nvm_effects(struct nvme_effects_log *log)
>  {
> -	struct nvme_effects_log *effects, *old;
> -
> -	effects = kzalloc_obj(*effects);
> -	if (!effects)
> -		return -ENOMEM;
> -
> -	old = xa_store(&ctrl->cels, csi, effects, GFP_KERNEL);
> -	if (xa_is_err(old)) {
> -		kfree(effects);
> -		return xa_err(old);
> -	}
> -
> -	*log = effects;
> -	return 0;
> -}
> -
> -static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
> -{
> -	struct nvme_effects_log	*log = ctrl->effects;
> -
>  	log->acs[nvme_admin_format_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
>  						NVME_CMD_EFFECTS_NCC |
>  						NVME_CMD_EFFECTS_CSE_MASK);
> @@ -3550,25 +3553,41 @@ static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
>  
>  static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  {
> +	struct nvme_effects_log *cel;
>  	int ret = 0;
>  
> -	if (ctrl->effects)
> -		return 0;
> +	mutex_lock(&ctrl->subsys->lock);
> +	cel = xa_load(&ctrl->subsys->cels, NVME_CSI_NVM);
> +	if (cel)
> +		goto out_set_effects;
>  
>  	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
> -		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
> +		ret = nvme_read_effects_log(ctrl, NVME_CSI_NVM, &cel);
>  		if (ret < 0)
> -			return ret;
> +			goto out_unlock;
>  	}
>  
> -	if (!ctrl->effects) {
> -		ret = nvme_init_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
> -		if (ret < 0)
> -			return ret;
> +	if (!cel) {
> +		cel = kzalloc_obj(*cel);
> +		if (!cel) {
> +			ret = -ENOMEM;
> +			goto out_unlock;
> +		}
> +		ret = 0;

I also really do not understand the change here.

While the high-level idea looks good, this really needs to be
split into finer grade and well-documented patches that can be
easily understood.