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

[RFC PATCH] 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 | 64 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index 5f82036..5488051 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,6 +1139,48 @@ 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;
@@ -1161,8 +1204,11 @@ int nvm_register(struct nvm_dev *dev)
 		return -ENOMEM;
 	}
 
-	/* register device with a supported media manager */
 	down_write(&nvm_lock);
+	if (list_empty(&nvm_devices))
+		register_reboot_notifier(&nvm_notifier);
+
+	/* register device with a supported media manager */
 	list_add(&dev->devices, &nvm_devices);
 	up_write(&nvm_lock);
 
@@ -1172,21 +1218,13 @@ int nvm_register(struct nvm_dev *dev)
 
 void nvm_unregister(struct nvm_dev *dev)
 {
-	struct nvm_target *t, *tmp;
+	down_write(&nvm_lock);
 
-	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);
+	_nvm_unregister(dev, false);
 
-	down_write(&nvm_lock);
-	list_del(&dev->devices);
+	if (list_empty(&nvm_devices))
+		unregister_reboot_notifier(&nvm_notifier);
 	up_write(&nvm_lock);
-
-	nvm_free(dev);
 }
 EXPORT_SYMBOL(nvm_unregister);
 
-- 
1.8.3.1

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

[Javier González]

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

> On 20 Mar 2019, at 16.38, 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>
> ---
> drivers/lightnvm/core.c | 64 +++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index 5f82036..5488051 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,6 +1139,48 @@ 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 */

Why this priority?

> +};
> +
> int nvm_register(struct nvm_dev *dev)
> {
> 	int ret, exp_pool_size;
> @@ -1161,8 +1204,11 @@ int nvm_register(struct nvm_dev *dev)
> 		return -ENOMEM;
> 	}
> 
> -	/* register device with a supported media manager */
> 	down_write(&nvm_lock);
> +	if (list_empty(&nvm_devices))
> +		register_reboot_notifier(&nvm_notifier);
> +
> +	/* register device with a supported media manager */
> 	list_add(&dev->devices, &nvm_devices);
> 	up_write(&nvm_lock);
> 
> @@ -1172,21 +1218,13 @@ int nvm_register(struct nvm_dev *dev)
> 
> void nvm_unregister(struct nvm_dev *dev)
> {
> -	struct nvm_target *t, *tmp;
> +	down_write(&nvm_lock);
> 
> -	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);
> +	_nvm_unregister(dev, false);

You are adding an extra lock dependency here. I cannot see any obvious
problem with it, but you probably want to test this with lock debugging
enabled.

> 
> -	down_write(&nvm_lock);
> -	list_del(&dev->devices);
> +	if (list_empty(&nvm_devices))
> +		unregister_reboot_notifier(&nvm_notifier);
> 	up_write(&nvm_lock);
> -
> -	nvm_free(dev);
> }
> EXPORT_SYMBOL(nvm_unregister);
> 
> --
> 1.8.3.1

Otherwise, I think adding this functionality is beneficial.



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

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

[Marcin Dziegielewski]

On 3/21/19 10:32 AM, Javier González wrote:
>> On 20 Mar 2019, at 16.38, 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>
>> ---
>> drivers/lightnvm/core.c | 64 +++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 51 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>> index 5f82036..5488051 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,6 +1139,48 @@ 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 */
> 
> Why this priority?

I believe that is the safest priority for our case, I based on bcache 
approach. Should I use other priority?

>> +};
>> +
>> int nvm_register(struct nvm_dev *dev)
>> {
>> 	int ret, exp_pool_size;
>> @@ -1161,8 +1204,11 @@ int nvm_register(struct nvm_dev *dev)
>> 		return -ENOMEM;
>> 	}
>>
>> -	/* register device with a supported media manager */
>> 	down_write(&nvm_lock);
>> +	if (list_empty(&nvm_devices))
>> +		register_reboot_notifier(&nvm_notifier);
>> +
>> +	/* register device with a supported media manager */
>> 	list_add(&dev->devices, &nvm_devices);
>> 	up_write(&nvm_lock);
>>
>> @@ -1172,21 +1218,13 @@ int nvm_register(struct nvm_dev *dev)
>>
>> void nvm_unregister(struct nvm_dev *dev)
>> {
>> -	struct nvm_target *t, *tmp;
>> +	down_write(&nvm_lock);
>>
>> -	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);
>> +	_nvm_unregister(dev, false);
> 
> You are adding an extra lock dependency here. I cannot see any obvious
> problem with it, but you probably want to test this with lock debugging
> enabled.

It was good suggestion, thanks. With enabled lock debugging I have found
one potential deadlock, I will send second version of this patch.

> 
>>
>> -	down_write(&nvm_lock);
>> -	list_del(&dev->devices);
>> +	if (list_empty(&nvm_devices))
>> +		unregister_reboot_notifier(&nvm_notifier);
>> 	up_write(&nvm_lock);
>> -
>> -	nvm_free(dev);
>> }
>> EXPORT_SYMBOL(nvm_unregister);
>>
>> --
>> 1.8.3.1
> 
> Otherwise, I think adding this functionality is beneficial.
> 
> 

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

[Javier González]

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


> On 22 Mar 2019, at 13.01, Marcin Dziegielewski <marcin.dziegielewski@intel.com> wrote:
> 
> 
> 
> On 3/21/19 10:32 AM, Javier González wrote:
>>> On 20 Mar 2019, at 16.38, 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>
>>> ---
>>> drivers/lightnvm/core.c | 64 +++++++++++++++++++++++++++++++++++++++----------
>>> 1 file changed, 51 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
>>> index 5f82036..5488051 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,6 +1139,48 @@ 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 */
>> Why this priority?
> 
> I believe that is the safest priority for our case, I based on bcache
> approach. Should I use other priority?
> 

I’m not very familiar with the notifier_block. I quick look showed that
most modules do not use it, that’s what I asked. Not real feedback here.

>>> +};
>>> +
>>> int nvm_register(struct nvm_dev *dev)
>>> {
>>> 	int ret, exp_pool_size;
>>> @@ -1161,8 +1204,11 @@ int nvm_register(struct nvm_dev *dev)
>>> 		return -ENOMEM;
>>> 	}
>>> 
>>> -	/* register device with a supported media manager */
>>> 	down_write(&nvm_lock);
>>> +	if (list_empty(&nvm_devices))
>>> +		register_reboot_notifier(&nvm_notifier);
>>> +
>>> +	/* register device with a supported media manager */
>>> 	list_add(&dev->devices, &nvm_devices);
>>> 	up_write(&nvm_lock);
>>> 
>>> @@ -1172,21 +1218,13 @@ int nvm_register(struct nvm_dev *dev)
>>> 
>>> void nvm_unregister(struct nvm_dev *dev)
>>> {
>>> -	struct nvm_target *t, *tmp;
>>> +	down_write(&nvm_lock);
>>> 
>>> -	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);
>>> +	_nvm_unregister(dev, false);
>> You are adding an extra lock dependency here. I cannot see any obvious
>> problem with it, but you probably want to test this with lock debugging
>> enabled.
> 
> It was good suggestion, thanks. With enabled lock debugging I have found
> one potential deadlock, I will send second version of this patch.
> 

Cool.


>>> -	down_write(&nvm_lock);
>>> -	list_del(&dev->devices);
>>> +	if (list_empty(&nvm_devices))
>>> +		unregister_reboot_notifier(&nvm_notifier);
>>> 	up_write(&nvm_lock);
>>> -
>>> -	nvm_free(dev);
>>> }
>>> EXPORT_SYMBOL(nvm_unregister);
>>> 
>>> --
>>> 1.8.3.1
>> Otherwise, I think adding this functionality is beneficial.

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