ploop: Fix maintaince_type obtaining design problem

Submitted by Kirill Tkhai on May 23, 2019, 11:13 a.m.

Details

Message ID 155860999678.29410.2882446078935815842.stgit@localhost.localdomain
State New
Series "ploop: Fix maintaince_type obtaining design problem"
Headers show

Commit Message

Kirill Tkhai May 23, 2019, 11:13 a.m.
"ploop-balloon status <dev>" made in parallel with "pcompact"
may break its logic and hang in unpredictable state.

Read comment in the function.

https://jira.sw.ru/browse/PSBM-94727

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 drivers/block/ploop/dev.c      |   31 +++++++++++++++++++++++++++++++
 include/linux/ploop/ploop_if.h |    3 +++
 2 files changed, 34 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
index d896ddec766a..d07928552291 100644
--- a/drivers/block/ploop/dev.c
+++ b/drivers/block/ploop/dev.c
@@ -4481,6 +4481,35 @@  static int ploop_balloon_ioc(struct ploop_device *plo, unsigned long arg)
 	if (ctl.inflate && ctl.keep_intact)
 		return -EINVAL;
 
+	/*
+	 * Userspace may use this IOC to get current maintaince mode
+	 * without changing it, or to change it. The ctl.keep_intact
+	 * modifier is used to differ, what the userspace wants.
+	 *
+	 * The problem was that:
+	 * 1)userspace sometimes passes the modifier set to 1, when
+	 *   the state *must* be changed;
+	 * 2)kernel completely ignores the modifier in case of device
+	 *   is in PLOOP_MNTN_DISCARD, PLOOP_MNTN_FBLOADED, PLOOP_MNTN_RELOC
+	 *   or PLOOP_MNTN_OFF mode.
+	 *
+	 * So, the harmless R/O command "ploop-balloon state <dev>"
+	 * made in parallel with "pcompact" switches the device in
+	 * unexpected maintaince mode, and "pcompact" lose its logic
+	 * and hangs.
+	 *
+	 * It's not possible to fix the problem in one of userspace
+	 * or kernel, since both of them suck. So, we introduce new
+	 * PLOOP_KEEP_INTACK_ALWAYS for newer pcompact to never change
+	 * current state and to fix the problem. Userspace will use
+	 * it. Old kernel and new userspace will work OK together,
+	 * since there is boolean !0 comparison in the kernel.
+	 *
+	 * Old code may be seen at least in ploop 7.0.140.2.
+	 */
+	if (ctl.keep_intact == PLOOP_KEEP_INTACK_ALWAYS)
+		goto mntn_type;
+
 	switch (plo->maintenance_type) {
 	case PLOOP_MNTN_DISCARD:
 		if (!test_bit(PLOOP_S_DISCARD_LOADED, &plo->state))
@@ -4515,6 +4544,8 @@  static int ploop_balloon_ioc(struct ploop_device *plo, unsigned long arg)
 			ploop_relax(plo);
 		}
 	}
+
+mntn_type:
 	ctl.mntn_type = plo->maintenance_type;
 
 	return copy_to_user((void*)arg, &ctl, sizeof(ctl));
diff --git a/include/linux/ploop/ploop_if.h b/include/linux/ploop/ploop_if.h
index 3b5928cfb69e..9c5e541f5f02 100644
--- a/include/linux/ploop/ploop_if.h
+++ b/include/linux/ploop/ploop_if.h
@@ -245,6 +245,9 @@  enum {
 	PLOOP_MNTN_PUSH_BACKUP, /* push backup is in progress */
 };
 
+#define PLOOP_KEEP_INTACK_OLD		1
+#define PLOOP_KEEP_INTACK_ALWAYS	2
+
 /*
  * This define should be in sync with enum above.
  * NB: PLOOP_MNTN_TRACK is handled separately because

Comments

Kirill Tkhai May 23, 2019, 12:32 p.m.
Please, consider RK for this.

On 23.05.2019 14:13, Kirill Tkhai wrote:
> "ploop-balloon status <dev>" made in parallel with "pcompact"
> may break its logic and hang in unpredictable state.
> 
> Read comment in the function.
> 
> https://jira.sw.ru/browse/PSBM-94727
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  drivers/block/ploop/dev.c      |   31 +++++++++++++++++++++++++++++++
>  include/linux/ploop/ploop_if.h |    3 +++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index d896ddec766a..d07928552291 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -4481,6 +4481,35 @@ static int ploop_balloon_ioc(struct ploop_device *plo, unsigned long arg)
>  	if (ctl.inflate && ctl.keep_intact)
>  		return -EINVAL;
>  
> +	/*
> +	 * Userspace may use this IOC to get current maintaince mode
> +	 * without changing it, or to change it. The ctl.keep_intact
> +	 * modifier is used to differ, what the userspace wants.
> +	 *
> +	 * The problem was that:
> +	 * 1)userspace sometimes passes the modifier set to 1, when
> +	 *   the state *must* be changed;
> +	 * 2)kernel completely ignores the modifier in case of device
> +	 *   is in PLOOP_MNTN_DISCARD, PLOOP_MNTN_FBLOADED, PLOOP_MNTN_RELOC
> +	 *   or PLOOP_MNTN_OFF mode.
> +	 *
> +	 * So, the harmless R/O command "ploop-balloon state <dev>"
> +	 * made in parallel with "pcompact" switches the device in
> +	 * unexpected maintaince mode, and "pcompact" lose its logic
> +	 * and hangs.
> +	 *
> +	 * It's not possible to fix the problem in one of userspace
> +	 * or kernel, since both of them suck. So, we introduce new
> +	 * PLOOP_KEEP_INTACK_ALWAYS for newer pcompact to never change
> +	 * current state and to fix the problem. Userspace will use
> +	 * it. Old kernel and new userspace will work OK together,
> +	 * since there is boolean !0 comparison in the kernel.
> +	 *
> +	 * Old code may be seen at least in ploop 7.0.140.2.
> +	 */
> +	if (ctl.keep_intact == PLOOP_KEEP_INTACK_ALWAYS)
> +		goto mntn_type;
> +
>  	switch (plo->maintenance_type) {
>  	case PLOOP_MNTN_DISCARD:
>  		if (!test_bit(PLOOP_S_DISCARD_LOADED, &plo->state))
> @@ -4515,6 +4544,8 @@ static int ploop_balloon_ioc(struct ploop_device *plo, unsigned long arg)
>  			ploop_relax(plo);
>  		}
>  	}
> +
> +mntn_type:
>  	ctl.mntn_type = plo->maintenance_type;
>  
>  	return copy_to_user((void*)arg, &ctl, sizeof(ctl));
> diff --git a/include/linux/ploop/ploop_if.h b/include/linux/ploop/ploop_if.h
> index 3b5928cfb69e..9c5e541f5f02 100644
> --- a/include/linux/ploop/ploop_if.h
> +++ b/include/linux/ploop/ploop_if.h
> @@ -245,6 +245,9 @@ enum {
>  	PLOOP_MNTN_PUSH_BACKUP, /* push backup is in progress */
>  };
>  
> +#define PLOOP_KEEP_INTACK_OLD		1
> +#define PLOOP_KEEP_INTACK_ALWAYS	2
> +
>  /*
>   * This define should be in sync with enum above.
>   * NB: PLOOP_MNTN_TRACK is handled separately because
>