[Devel] [PATCH vz7] ploop: increase logging on errors when opening new deltas

[Pavel Tikhomirov]

Looks good, see minor nits in inline comments.

On 22.05.2023 23:39, Alexander Atanasov wrote:
> Ocassionally we got EBUSY but it is a bit over used,
> so it is not clear what it means.
> 
> Add more logging to catch the source of the error.
> 
> https://jira.vzint.dev/browse/PSBM-146836
> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
>   drivers/block/ploop/dev.c         |  9 ++++++++-
>   drivers/block/ploop/fmt_ploop1.c  |  4 +++-
>   drivers/block/ploop/io_kaio.c     | 13 ++++++++++---
>   drivers/block/ploop/io_kaio_map.c |  6 ++++--
>   4 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 6eb22168b5fe..75e427927713 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -3572,8 +3572,11 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   			   sizeof(struct ploop_ctl_chunk)))
>   		return -EFAULT;
>   
> -	if (plo->maintenance_type != PLOOP_MNTN_OFF)
> +	if (plo->maintenance_type != PLOOP_MNTN_OFF) {
> +		if (printk_ratelimit())
> +			PL_WARN(plo, "Attempt to replace while in maintenance mode\n");
>   		return -EBUSY;
> +	}
>   
>   	old_delta = find_delta(plo, ctl.pctl_level);
>   	if (old_delta == NULL)
> @@ -3586,6 +3589,10 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   	if (IS_ERR(delta))
>   		return PTR_ERR(delta);
>   
> +	WARN_ONCE(delta->ops != old_delta->ops,
> +		  "New delta uses different io %p vs %p\n",
> +		  delta->ops, old_delta->ops);
> +
>   	err = delta->ops->compose(delta, 1, &chunk);
>   	if (err)
>   		goto out_destroy;
> diff --git a/drivers/block/ploop/fmt_ploop1.c b/drivers/block/ploop/fmt_ploop1.c
> index e59a9eb50ac2..a89804561e57 100644
> --- a/drivers/block/ploop/fmt_ploop1.c
> +++ b/drivers/block/ploop/fmt_ploop1.c
> @@ -314,8 +314,10 @@ ploop1_open(struct ploop_delta * delta)
>   	if (!(delta->flags & PLOOP_FMT_RDONLY)) {
>   		pvd_header_set_disk_in_use(vh);
>   		err = delta->io.ops->sync_write(&delta->io, ph->dyn_page, 4096, 0, 0);
> -		if (err)
> +		if (err) {
> +			PL_ERR(delta->plo, "write failed updating in use\n");

Don't we need err printed here? (like in other places below where we 
have err printed)

>   			goto out_err;
> +		}
>   	}
>   
>   	delta->io.alloc_head = ph->alloc_head;
> diff --git a/drivers/block/ploop/io_kaio.c b/drivers/block/ploop/io_kaio.c
> index ab93d2c70bc5..b7258252deb2 100644
> --- a/drivers/block/ploop/io_kaio.c
> +++ b/drivers/block/ploop/io_kaio.c
> @@ -997,17 +997,23 @@ static int kaio_open(struct ploop_io * io)
>   	io->files.bdev = io->files.inode->i_sb->s_bdev;
>   
>   	err = io->ops->sync(io);
> -	if (err)
> +	if (err) {
> +		PL_WARN(delta->plo, "open failed to sync err=%d\n", err);
>   		return err;
> +	}
>   
>   	mutex_lock(&io->files.inode->i_mutex);
>   	err = kaio_invalidate_cache(io);
> -	if (!err)
> +	if (err)
> +		PL_WARN(delta->plo, "invaldiate_cache failed err=%d\n", err);
> +	else
>   		err = ploop_kaio_open(file, delta->flags & PLOOP_FMT_RDONLY);
>   	mutex_unlock(&io->files.inode->i_mutex);
>   
> -	if (err)
> +	if (err) {
> +		PL_WARN(delta->plo, "open failed err=%d\n", err);
>   		return err;
> +	}
>   
>   	io->files.em_tree = &dummy_em_tree;
>   
> @@ -1019,6 +1025,7 @@ static int kaio_open(struct ploop_io * io)
>   			err = PTR_ERR(io->fsync_thread);
>   			io->fsync_thread = NULL;
>   			ploop_kaio_close(io->files.mapping, 0);
> +			PL_WARN(delta->plo, "fsync thread start failed=%d\n", err);
>   			return err;
>   		}
>   
> diff --git a/drivers/block/ploop/io_kaio_map.c b/drivers/block/ploop/io_kaio_map.c
> index d4ff39d95e74..5a3d11e0f4a9 100644
> --- a/drivers/block/ploop/io_kaio_map.c
> +++ b/drivers/block/ploop/io_kaio_map.c
> @@ -35,9 +35,11 @@ int ploop_kaio_open(struct file * file, int rdonly)
>   				else
>   					m->readers++;
>   			} else {
> -				if (m->readers)
> +				if (m->readers) {
> +					pr_warn("File is already active:%d\n",
> +						m->readers);
>   					err = -EBUSY;
> -				else
> +				} else
>   					m->readers = -1;

I'd rather you follow the codding style here and add {} to both branches.

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces

>   			}
>   			goto kaio_open_done;

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