[Devel] [PATCH vz7 v3] ploop: properly restore old delta kobject on replace error

[Pavel Tikhomirov]

On 15.06.2023 04:23, Alexander Atanasov wrote:
> Current code removes the old_delta kobject before the new delta
> is completely ready to be used, in case there is an error while
> reading BAT, the "too short BAT" error we have seen, the new delta
> and its objects are destroyed properly but the original delta kobject
> is not restored, so pdelta sysfs dir stay empty and userspace tools
> get confused since they consult this files before performing operations
> on the ploop device.
> 
> To fix this instead of deleting the delta's kobject early move it at
> the end and on error restore back original kobject.
> 
> Extract code to rename kobject into a function and cleanup its users.
> Since kobject_add can fail, get an extra reference on error so later
> kobject_del/kobject_put would not destroy it unexpectedly. Object
> should be intact except that it would not be visible in sysfs.
> 
> Fixes:
> 5f3ee110e6f4 (ploop: Repopulate holes_bitmap on changing delta, 2019-04-17)
> 
> https://jira.vzint.dev/browse/PSBM-146797

Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>

> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
>   drivers/block/ploop/dev.c | 54 ++++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 26 deletions(-)
> 
> v1->v2: Addressing review comments. Removed the rename to minimize
> possibility of errors. added getting extra refs after failed add.
> 
> v2->v3: dropped the idea of extra refs since they are not necessary.
> double kobject_put is not an issue  since parent is cleared and
> kset objects are not used.
> 
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 6eb22168b5fe..0d6272b39863 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -3594,27 +3594,24 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   	if (err)
>   		goto out_destroy;
>   
> -	kobject_del(&old_delta->kobj);
> -
> -	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
> -			  "%d", delta->level);
> -	/* _put below is a pair for _get for OLD delta */
> -	kobject_put(&plo->kobj);
> -
> -	if (err < 0) {
> -		kobject_put(&plo->kobj);
> -		goto out_close;
> -	}
> -
>   	ploop_quiesce(plo);
>   	if (delta->ops->replace_delta) {
>   		err = delta->ops->replace_delta(delta);
>   		if (err) {
>   			ploop_relax(plo);
> -			goto out_kobj_del;
> +			goto out_close;
>   		}
>   	}
>   
> +	/* Remove old delta kobj to avoid name collision with the new one */
> +	kobject_del(&old_delta->kobj);
> +	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
> +			  "%d", delta->level);
> +	if (err < 0) {
> +		/* _put for failed _ADD */
> +		kobject_put(&plo->kobj);
> +		goto out_kobj_restore;
> +	}
>   	ploop_map_destroy(&plo->map);
>   	list_replace_init(&old_delta->list, &delta->list);
>   	ploop_delta_list_changed(plo);
> @@ -3632,11 +3629,14 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   	old_delta->ops->stop(old_delta);
>   	old_delta->ops->destroy(old_delta);
>   	kobject_put(&old_delta->kobj);
> -	return 0;
> -
> -out_kobj_del:
> -	kobject_del(&delta->kobj);
>   	kobject_put(&plo->kobj);
> +	return 0;
> +out_kobj_restore:
> +	/* we haven't dropped our plo->kobj ref just add back */
> +	err = KOBJECT_ADD(&old_delta->kobj, &plo->kobj, "%d", old_delta->level);
> +	if (err < 0)
> +		/* Nothing we can do unfortunately */
> +		PL_ERR(plo, "Failed to restore old delta kobject\n");
>   out_close:
>   	delta->ops->stop(delta);
>   out_destroy:
> @@ -3965,6 +3965,16 @@ static void renumber_deltas(struct ploop_device * plo)
>   	}
>   }
>   
> +/*
> + * Have to implement own version of kobject_rename since it is GPL only symbol
> + */
> +static int ploop_rename_delta(struct ploop_delta *delta, int level, char *pref)
> +{
> +	kobject_del(&delta->kobj);
> +	return KOBJECT_ADD(&delta->kobj, &delta->plo->kobj,
> +			  "%s%d", pref ? : "", level);
> +}
> +
>   static void rename_deltas(struct ploop_device * plo, int level)
>   {
>   	struct ploop_delta * delta;
> @@ -3974,15 +3984,7 @@ static void rename_deltas(struct ploop_device * plo, int level)
>   
>   		if (delta->level < level)
>   			continue;
> -#if 0
> -		/* Oops, kobject_rename() is not exported! */
> -		sprintf(nname, "%d", delta->level);
> -		err = kobject_rename(&delta->kobj, nname);
> -#else
> -		kobject_del(&delta->kobj);
> -		err = KOBJECT_ADD(&delta->kobj, &plo->kobj,
> -				  "%d", delta->level);
> -#endif
> +		err = ploop_rename_delta(delta, delta->level, NULL);
>   		if (err)
>   			PL_WARN(plo, "rename_deltas: %d %d %d\n", err, level, delta->level);
>   	}

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.