[RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[Marcin Dziegielewski]

Currently if we issue reboot to the system pblk will close
ungracefully and in consequence it will need recovery on load.

This patch propose utilize of reboot notifier feature to trigger
gracefull pblk shutdown on reboot.

Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
---
 drivers/lightnvm/core.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 14 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 5f82036..4a421f5 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -27,6 +27,7 @@
 #include <linux/miscdevice.h>
 #include <linux/lightnvm.h>
 #include <linux/sched/sysctl.h>
+#include <linux/reboot.h>
 
 static LIST_HEAD(nvm_tgt_types);
 static DECLARE_RWSEM(nvm_tgtt_lock);
@@ -1138,9 +1139,52 @@ struct nvm_dev *nvm_alloc_dev(int node)
 }
 EXPORT_SYMBOL(nvm_alloc_dev);
 
+static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
+{
+	struct nvm_target *t, *tmp;
+
+	mutex_lock(&dev->mlock);
+	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
+		if (t->dev->parent != dev)
+			continue;
+		__nvm_remove_target(t, graceful);
+	}
+	mutex_unlock(&dev->mlock);
+
+	list_del(&dev->devices);
+
+	nvm_free(dev);
+}
+
+static int nvm_notify_reboot(struct notifier_block *this,
+			    unsigned long code, void *x)
+{
+	struct nvm_dev *dev, *t;
+
+	down_write(&nvm_lock);
+	if (list_empty(&nvm_devices)) {
+		up_write(&nvm_lock);
+		return NOTIFY_DONE;
+	}
+
+	list_for_each_entry_safe(dev, t, &nvm_devices, devices)
+		_nvm_unregister(dev, true);
+
+	up_write(&nvm_lock);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block nvm_notifier = {
+	.notifier_call	= nvm_notify_reboot,
+	.next		= NULL,
+	.priority	= INT_MAX, /* before any real devices */
+};
+
 int nvm_register(struct nvm_dev *dev)
 {
 	int ret, exp_pool_size;
+	bool is_first;
 
 	if (!dev->q || !dev->ops)
 		return -EINVAL;
@@ -1161,32 +1205,25 @@ int nvm_register(struct nvm_dev *dev)
 		return -ENOMEM;
 	}
 
-	/* register device with a supported media manager */
 	down_write(&nvm_lock);
+	is_first = list_empty(&nvm_devices);
+
+	/* register device with a supported media manager */
 	list_add(&dev->devices, &nvm_devices);
 	up_write(&nvm_lock);
 
+	if (is_first)
+		register_reboot_notifier(&nvm_notifier);
+
 	return 0;
 }
 EXPORT_SYMBOL(nvm_register);
 
 void nvm_unregister(struct nvm_dev *dev)
 {
-	struct nvm_target *t, *tmp;
-
-	mutex_lock(&dev->mlock);
-	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
-		if (t->dev->parent != dev)
-			continue;
-		__nvm_remove_target(t, false);
-	}
-	mutex_unlock(&dev->mlock);
-
 	down_write(&nvm_lock);
-	list_del(&dev->devices);
+	_nvm_unregister(dev, false);
 	up_write(&nvm_lock);
-
-	nvm_free(dev);
 }
 EXPORT_SYMBOL(nvm_unregister);
 
-- 
1.8.3.1

Re: [RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]

> On 23 Mar 2019, at 02.07, Marcin Dziegielewski <marcin.dziegielewski@intel.com> wrote:
> 
> Currently if we issue reboot to the system pblk will close
> ungracefully and in consequence it will need recovery on load.
> 
> This patch propose utilize of reboot notifier feature to trigger
> gracefull pblk shutdown on reboot.
> 
> Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
> —

Please, add changes since V1 next time.

> drivers/lightnvm/core.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5f82036..4a421f5 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -27,6 +27,7 @@
> #include <linux/miscdevice.h>
> #include <linux/lightnvm.h>
> #include <linux/sched/sysctl.h>
> +#include <linux/reboot.h>
> 
> static LIST_HEAD(nvm_tgt_types);
> static DECLARE_RWSEM(nvm_tgtt_lock);
> @@ -1138,9 +1139,52 @@ struct nvm_dev *nvm_alloc_dev(int node)
> }
> EXPORT_SYMBOL(nvm_alloc_dev);
> 
> +static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
> +{
> +	struct nvm_target *t, *tmp;
> +
> +	mutex_lock(&dev->mlock);
> +	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> +		if (t->dev->parent != dev)
> +			continue;
> +		__nvm_remove_target(t, graceful);
> +	}
> +	mutex_unlock(&dev->mlock);
> +
> +	list_del(&dev->devices);
> +
> +	nvm_free(dev);
> +}
> +
> +static int nvm_notify_reboot(struct notifier_block *this,
> +			    unsigned long code, void *x)
> +{
> +	struct nvm_dev *dev, *t;
> +
> +	down_write(&nvm_lock);
> +	if (list_empty(&nvm_devices)) {
> +		up_write(&nvm_lock);
> +		return NOTIFY_DONE;
> +	}
> +
> +	list_for_each_entry_safe(dev, t, &nvm_devices, devices)
> +		_nvm_unregister(dev, true);
> +
> +	up_write(&nvm_lock);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block nvm_notifier = {
> +	.notifier_call	= nvm_notify_reboot,
> +	.next		= NULL,
> +	.priority	= INT_MAX, /* before any real devices */

Nip: Can you align all values at ‘=‘?

> +};
> +
> int nvm_register(struct nvm_dev *dev)
> {
> 	int ret, exp_pool_size;
> +	bool is_first;
> 
> 	if (!dev->q || !dev->ops)
> 		return -EINVAL;
> @@ -1161,32 +1205,25 @@ int nvm_register(struct nvm_dev *dev)
> 		return -ENOMEM;
> 	}
> 
> -	/* register device with a supported media manager */
> 	down_write(&nvm_lock);
> +	is_first = list_empty(&nvm_devices);
> +
> +	/* register device with a supported media manager */
> 	list_add(&dev->devices, &nvm_devices);
> 	up_write(&nvm_lock);
> 
> +	if (is_first)
> +		register_reboot_notifier(&nvm_notifier);
> +
> 	return 0;
> }
> EXPORT_SYMBOL(nvm_register);
> 
> void nvm_unregister(struct nvm_dev *dev)
> {
> -	struct nvm_target *t, *tmp;
> -
> -	mutex_lock(&dev->mlock);
> -	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> -		if (t->dev->parent != dev)
> -			continue;
> -		__nvm_remove_target(t, false);
> -	}
> -	mutex_unlock(&dev->mlock);
> -
> 	down_write(&nvm_lock);
> -	list_del(&dev->devices);
> +	_nvm_unregister(dev, false);
> 	up_write(&nvm_lock);
> -
> -	nvm_free(dev);
> }
> EXPORT_SYMBOL(nvm_unregister);
> 
> --
> 1.8.3.1

Otherwise, it looks good to me.

Reviewed-by: Javier González <javier@javigon.com>



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[Marcin Dziegielewski]

On 3/25/19 7:35 AM, Javier González wrote:
>> On 23 Mar 2019, at 02.07, Marcin Dziegielewski <marcin.dziegielewski@intel.com> wrote:
>>
>> Currently if we issue reboot to the system pblk will close
>> ungracefully and in consequence it will need recovery on load.
>>
>> This patch propose utilize of reboot notifier feature to trigger
>> gracefull pblk shutdown on reboot.
>>
>> Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
>> —
> 
> Please, add changes since V1 next time.
Yes, I forgot about it, my mistake.
> 
>> drivers/lightnvm/core.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 5f82036..4a421f5 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -27,6 +27,7 @@
>> #include <linux/miscdevice.h>
>> #include <linux/lightnvm.h>
>> #include <linux/sched/sysctl.h>
>> +#include <linux/reboot.h>
>>
>> static LIST_HEAD(nvm_tgt_types);
>> static DECLARE_RWSEM(nvm_tgtt_lock);
>> @@ -1138,9 +1139,52 @@ struct nvm_dev *nvm_alloc_dev(int node)
>> }
>> EXPORT_SYMBOL(nvm_alloc_dev);
>>
>> +static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
>> +{
>> +	struct nvm_target *t, *tmp;
>> +
>> +	mutex_lock(&dev->mlock);
>> +	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
>> +		if (t->dev->parent != dev)
>> +			continue;
>> +		__nvm_remove_target(t, graceful);
>> +	}
>> +	mutex_unlock(&dev->mlock);
>> +
>> +	list_del(&dev->devices);
>> +
>> +	nvm_free(dev);
>> +}
>> +
>> +static int nvm_notify_reboot(struct notifier_block *this,
>> +			    unsigned long code, void *x)
>> +{
>> +	struct nvm_dev *dev, *t;
>> +
>> +	down_write(&nvm_lock);
>> +	if (list_empty(&nvm_devices)) {
>> +		up_write(&nvm_lock);
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	list_for_each_entry_safe(dev, t, &nvm_devices, devices)
>> +		_nvm_unregister(dev, true);
>> +
>> +	up_write(&nvm_lock);
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block nvm_notifier = {
>> +	.notifier_call	= nvm_notify_reboot,
>> +	.next		= NULL,
>> +	.priority	= INT_MAX, /* before any real devices */
> 
> Nip: Can you align all values at ‘=‘?
Sure, I can change something, but I'm not sure what do you mean? Would 
you describe more explicitly or put example? Thanks in advance.
> 
>> +};
>> +
>> int nvm_register(struct nvm_dev *dev)
>> {
>> 	int ret, exp_pool_size;
>> +	bool is_first;
>>
>> 	if (!dev->q || !dev->ops)
>> 		return -EINVAL;
>> @@ -1161,32 +1205,25 @@ int nvm_register(struct nvm_dev *dev)
>> 		return -ENOMEM;
>> 	}
>>
>> -	/* register device with a supported media manager */
>> 	down_write(&nvm_lock);
>> +	is_first = list_empty(&nvm_devices);
>> +
>> +	/* register device with a supported media manager */
>> 	list_add(&dev->devices, &nvm_devices);
>> 	up_write(&nvm_lock);
>>
>> +	if (is_first)
>> +		register_reboot_notifier(&nvm_notifier);
>> +
>> 	return 0;
>> }
>> EXPORT_SYMBOL(nvm_register);
>>
>> void nvm_unregister(struct nvm_dev *dev)
>> {
>> -	struct nvm_target *t, *tmp;
>> -
>> -	mutex_lock(&dev->mlock);
>> -	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
>> -		if (t->dev->parent != dev)
>> -			continue;
>> -		__nvm_remove_target(t, false);
>> -	}
>> -	mutex_unlock(&dev->mlock);
>> -
>> 	down_write(&nvm_lock);
>> -	list_del(&dev->devices);
>> +	_nvm_unregister(dev, false);
>> 	up_write(&nvm_lock);
>> -
>> -	nvm_free(dev);
>> }
>> EXPORT_SYMBOL(nvm_unregister);
>>
>> --
>> 1.8.3.1
> 
> Otherwise, it looks good to me.
> 
> Reviewed-by: Javier González <javier@javigon.com>
> 
> 

Re: [RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[Hans Holmberg]

On Fri, Mar 22, 2019 at 7:07 PM Marcin Dziegielewski
<marcin.dziegielewski@intel.com> wrote:
>
> Currently if we issue reboot to the system pblk will close
> ungracefully and in consequence it will need recovery on load.
>
> This patch propose utilize of reboot notifier feature to trigger
> gracefull pblk shutdown on reboot.


To put in my two-penny worth, this seems unmotivated.

If user-land chooses to reboot, why should we delay the reboot in the
order of second(s)?
Recovering should be faster than padding anyway, so why not take the hit there?

Also, It's nice to be able to test OOB recovery by just rebooting the
system, and I have not found this sort of thing being done anywhere
else in the block layer.

/ Hans
>
> Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
> ---
>  drivers/lightnvm/core.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5f82036..4a421f5 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -27,6 +27,7 @@
>  #include <linux/miscdevice.h>
>  #include <linux/lightnvm.h>
>  #include <linux/sched/sysctl.h>
> +#include <linux/reboot.h>
>
>  static LIST_HEAD(nvm_tgt_types);
>  static DECLARE_RWSEM(nvm_tgtt_lock);
> @@ -1138,9 +1139,52 @@ struct nvm_dev *nvm_alloc_dev(int node)
>  }
>  EXPORT_SYMBOL(nvm_alloc_dev);
>
> +static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
> +{
> +       struct nvm_target *t, *tmp;
> +
> +       mutex_lock(&dev->mlock);
> +       list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> +               if (t->dev->parent != dev)
> +                       continue;
> +               __nvm_remove_target(t, graceful);
> +       }
> +       mutex_unlock(&dev->mlock);
> +
> +       list_del(&dev->devices);
> +
> +       nvm_free(dev);
> +}
> +
> +static int nvm_notify_reboot(struct notifier_block *this,
> +                           unsigned long code, void *x)
> +{
> +       struct nvm_dev *dev, *t;
> +
> +       down_write(&nvm_lock);
> +       if (list_empty(&nvm_devices)) {
> +               up_write(&nvm_lock);
> +               return NOTIFY_DONE;
> +       }
> +
> +       list_for_each_entry_safe(dev, t, &nvm_devices, devices)
> +               _nvm_unregister(dev, true);
> +
> +       up_write(&nvm_lock);
> +
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block nvm_notifier = {
> +       .notifier_call  = nvm_notify_reboot,
> +       .next           = NULL,
> +       .priority       = INT_MAX, /* before any real devices */
> +};
> +
>  int nvm_register(struct nvm_dev *dev)
>  {
>         int ret, exp_pool_size;
> +       bool is_first;
>
>         if (!dev->q || !dev->ops)
>                 return -EINVAL;
> @@ -1161,32 +1205,25 @@ int nvm_register(struct nvm_dev *dev)
>                 return -ENOMEM;
>         }
>
> -       /* register device with a supported media manager */
>         down_write(&nvm_lock);
> +       is_first = list_empty(&nvm_devices);
> +
> +       /* register device with a supported media manager */
>         list_add(&dev->devices, &nvm_devices);
>         up_write(&nvm_lock);
>
> +       if (is_first)
> +               register_reboot_notifier(&nvm_notifier);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL(nvm_register);
>
>  void nvm_unregister(struct nvm_dev *dev)
>  {
> -       struct nvm_target *t, *tmp;
> -
> -       mutex_lock(&dev->mlock);
> -       list_for_each_entry_safe(t, tmp, &dev->targets, list) {
> -               if (t->dev->parent != dev)
> -                       continue;
> -               __nvm_remove_target(t, false);
> -       }
> -       mutex_unlock(&dev->mlock);
> -
>         down_write(&nvm_lock);
> -       list_del(&dev->devices);
> +       _nvm_unregister(dev, false);
>         up_write(&nvm_lock);
> -
> -       nvm_free(dev);
>  }
>  EXPORT_SYMBOL(nvm_unregister);
>
> --
> 1.8.3.1
>

Re: [RFC PATCH v2] lightnvm: add mechanism to trigger pblk close on reboot

[Matias Bjørling]

On 3/25/19 11:11 AM, Hans Holmberg wrote:
> On Fri, Mar 22, 2019 at 7:07 PM Marcin Dziegielewski
> <marcin.dziegielewski@intel.com> wrote:
>>
>> Currently if we issue reboot to the system pblk will close
>> ungracefully and in consequence it will need recovery on load.
>>
>> This patch propose utilize of reboot notifier feature to trigger
>> gracefull pblk shutdown on reboot.
> 
> 
> To put in my two-penny worth, this seems unmotivated.
> 
> If user-land chooses to reboot, why should we delay the reboot in the
> order of second(s)?
> Recovering should be faster than padding anyway, so why not take the hit there?
> 
> Also, It's nice to be able to test OOB recovery by just rebooting the
> system, and I have not found this sort of thing being done anywhere
> else in the block layer.
> 
> / Hans

I believe this is taken care of in user-space. E.g., /etc/rc0.d/ has all 
the scripts to properly close down file-systems, device mappers, etc. 
This should be handled at the distribution level.

>>
>> Signed-off-by: Marcin Dziegielewski <marcin.dziegielewski@intel.com>
>> ---
>>   drivers/lightnvm/core.c | 65 ++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 51 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 5f82036..4a421f5 100644
>> --- a/drivers/lightnvm/core.c
>> +++ b/drivers/lightnvm/core.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/miscdevice.h>
>>   #include <linux/lightnvm.h>
>>   #include <linux/sched/sysctl.h>
>> +#include <linux/reboot.h>
>>
>>   static LIST_HEAD(nvm_tgt_types);
>>   static DECLARE_RWSEM(nvm_tgtt_lock);
>> @@ -1138,9 +1139,52 @@ struct nvm_dev *nvm_alloc_dev(int node)
>>   }
>>   EXPORT_SYMBOL(nvm_alloc_dev);
>>
>> +static void _nvm_unregister(struct nvm_dev *dev, bool graceful)
>> +{
>> +       struct nvm_target *t, *tmp;
>> +
>> +       mutex_lock(&dev->mlock);
>> +       list_for_each_entry_safe(t, tmp, &dev->targets, list) {
>> +               if (t->dev->parent != dev)
>> +                       continue;
>> +               __nvm_remove_target(t, graceful);
>> +       }
>> +       mutex_unlock(&dev->mlock);
>> +
>> +       list_del(&dev->devices);
>> +
>> +       nvm_free(dev);
>> +}
>> +
>> +static int nvm_notify_reboot(struct notifier_block *this,
>> +                           unsigned long code, void *x)
>> +{
>> +       struct nvm_dev *dev, *t;
>> +
>> +       down_write(&nvm_lock);
>> +       if (list_empty(&nvm_devices)) {
>> +               up_write(&nvm_lock);
>> +               return NOTIFY_DONE;
>> +       }
>> +
>> +       list_for_each_entry_safe(dev, t, &nvm_devices, devices)
>> +               _nvm_unregister(dev, true);
>> +
>> +       up_write(&nvm_lock);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block nvm_notifier = {
>> +       .notifier_call  = nvm_notify_reboot,
>> +       .next           = NULL,
>> +       .priority       = INT_MAX, /* before any real devices */
>> +};
>> +
>>   int nvm_register(struct nvm_dev *dev)
>>   {
>>          int ret, exp_pool_size;
>> +       bool is_first;
>>
>>          if (!dev->q || !dev->ops)
>>                  return -EINVAL;
>> @@ -1161,32 +1205,25 @@ int nvm_register(struct nvm_dev *dev)
>>                  return -ENOMEM;
>>          }
>>
>> -       /* register device with a supported media manager */
>>          down_write(&nvm_lock);
>> +       is_first = list_empty(&nvm_devices);
>> +
>> +       /* register device with a supported media manager */
>>          list_add(&dev->devices, &nvm_devices);
>>          up_write(&nvm_lock);
>>
>> +       if (is_first)
>> +               register_reboot_notifier(&nvm_notifier);
>> +
>>          return 0;
>>   }
>>   EXPORT_SYMBOL(nvm_register);
>>
>>   void nvm_unregister(struct nvm_dev *dev)
>>   {
>> -       struct nvm_target *t, *tmp;
>> -
>> -       mutex_lock(&dev->mlock);
>> -       list_for_each_entry_safe(t, tmp, &dev->targets, list) {
>> -               if (t->dev->parent != dev)
>> -                       continue;
>> -               __nvm_remove_target(t, false);
>> -       }
>> -       mutex_unlock(&dev->mlock);
>> -
>>          down_write(&nvm_lock);
>> -       list_del(&dev->devices);
>> +       _nvm_unregister(dev, false);
>>          up_write(&nvm_lock);
>> -
>> -       nvm_free(dev);
>>   }
>>   EXPORT_SYMBOL(nvm_unregister);
>>
>> --
>> 1.8.3.1
>>