Skip to contentSkip to conversation

ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

PATCH New alex chen 3 participants 10 messages Oct 31, 2017 – Nov 2, 2017 OCFS2

lore
Share & tools
lore

alex chen

author
#0 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin
o2nm_depend_item

    configfs_depend_item
     inode_lock
      down_write
      -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
 fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +8 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
 		if (reg->hr_item_pinned || reg->hr_item_dropped)
 			goto skip_pin;
 
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+
 		/* Ignore ENOENT only for local hb (userdlm domain) */
 		ret = o2nm_depend_item(&reg->hr_item);
 		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
 			else {
 				mlog(ML_ERROR, "Pin region %s fails with %d\n",
 				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);
 				break;
 			}
 		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
 skip_pin:
 		if (found)
 			break;

-- 1.9.5.msysgit.1

#1 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Alex,

Quoted 26 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin
o2nm_depend_item

    configfs_depend_item
     inode_lock
      down_write
      -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
 fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 6 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
 		if (reg->hr_item_pinned || reg->hr_item_dropped)
 			goto skip_pin;
 
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

Quoted 21 lines
fs/ocfs2/cluster/heartbeat.c +5 -1
 		/* Ignore ENOENT only for local hb (userdlm domain) */
 		ret = o2nm_depend_item(&reg->hr_item);
 		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
 			else {
 				mlog(ML_ERROR, "Pin region %s fails with %d\n",
 				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);
 				break;
 			}
 		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
 skip_pin:
 		if (found)
 			break;
-- 1.9.5.msysgit.1
#2 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Quoted 16 lines · Joseph Qi

Hi Alex,

Quoted 13 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin

    o2nm_depend_item
     configfs_depend_item
      inode_lock
       down_write
       -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Hi Alex,
Actually, I don't figure why this code path will lead to a soft lockup yet. Can you elaborate further?

And I agree with Joseph that simply unlocking o2hb_live_lock will introduce a race here since we are waking through that global list.

Thanks,
Changwei

Quoted 51 lines
Quoted 13 lines
Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
  1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 6 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
  		if (reg->hr_item_pinned || reg->hr_item_dropped)
  			goto skip_pin;
 
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
  		/* Ignore ENOENT only for local hb (userdlm domain) */
  		ret = o2nm_depend_item(&reg->hr_item);
  		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
  			else {
  				mlog(ML_ERROR, "Pin region %s fails with %d\n",
  				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
  				break;
  			}
  		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
  skip_pin:
  		if (found)
  			break;
-- 1.9.5.msysgit.1

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#3 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

In reply to Joseph Qi

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

Quoted 32 lines · Joseph Qi

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin
o2nm_depend_item

    configfs_depend_item
     inode_lock
      down_write
      -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
 fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
 		if (reg->hr_item_pinned || reg->hr_item_dropped)
 			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
 		/* Ignore ENOENT only for local hb (userdlm domain) */
 		ret = o2nm_depend_item(&reg->hr_item);
 		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
 			else {
 				mlog(ML_ERROR, "Pin region %s fails with %d\n",
 				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
 				break;
 			}
 		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
 skip_pin:
 		if (found)
 			break;
-- 1.9.5.msysgit.1

.

#4 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

Quoted 43 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin

    o2nm_depend_item
     configfs_depend_item
      inode_lock
       down_write
       -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
  1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
  		if (reg->hr_item_pinned || reg->hr_item_dropped)
  			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

Quoted 6 lines

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
  		/* Ignore ENOENT only for local hb (userdlm domain) */
  		ret = o2nm_depend_item(&reg->hr_item);
  		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
  			else {
  				mlog(ML_ERROR, "Pin region %s fails with %d\n",
  				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
  				break;
  			}
  		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
  skip_pin:
  		if (found)
  			break;
-- 1.9.5.msysgit.1

.

author
#5 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Changwei,

Thanks for you reply.

Quoted 10 lines · Changwei Ge

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

It is a very very bad idea to sleep while holding a spin lock.
If a process grabs a spinlock and goes to sleep before releasing it. Then this process yields the control of the CPU to a second process. Unfortunately the second process also need to grab the spinlock and it will busy wait. On an uniprocessor machine the second process will lock the CPU not allowing the first process to wake up and release the spinlock so the second process can't continue, it is basically a deadlock.

Quoted 50 lines
Quoted 43 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)
o2hb_region_pin

    o2nm_depend_item
     configfs_depend_item
      inode_lock
       down_write
       -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
  1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
  		if (reg->hr_item_pinned || reg->hr_item_dropped)
  			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

Quoted 6 lines

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Quoted 4 lines

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

In global heartbeat mode, we pin all regions if there is the first dependent user and unpin all regions if the number of dependent users falls to zero.
So there is not exist another region will be attached or detached after spin_unlock().

Thank,
Alex

Quoted 34 lines
Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
  		/* Ignore ENOENT only for local hb (userdlm domain) */
  		ret = o2nm_depend_item(&reg->hr_item);
  		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
  			else {
  				mlog(ML_ERROR, "Pin region %s fails with %d\n",
  				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
  				break;
  			}
  		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
  skip_pin:
  		if (found)
  			break;
-- 1.9.5.msysgit.1

.

.

#6 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Alex,

Quoted 16 lines · alex chen

Hi Changwei,

Thanks for you reply.

Quoted 10 lines · Changwei Ge

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

Quoted 7 lines

It is a very very bad idea to sleep while holding a spin lock.
If a process grabs a spinlock and goes to sleep before releasing it. Then this process yields the control of the CPU to a second process. Unfortunately the second process also need to grab the spinlock and it will busy wait. On an uniprocessor machine the second process will lock the CPU not allowing the first process to wake up and release the spinlock so the second process can't continue, it is basically a deadlock.

I agree. This soft lockup truly exists. This point is OK for me.

Quoted 51 lines
Quoted 51 lines
Quoted 50 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)

    o2hb_region_pin
     o2nm_depend_item
      configfs_depend_item
       inode_lock
        down_write
        -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
   fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
   1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
   		if (reg->hr_item_pinned || reg->hr_item_dropped)
   			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Quoted 8 lines
Quoted 4 lines

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

In global heartbeat mode, we pin all regions if there is the first dependent user and unpin all regions if the number of dependent users falls to zero.
So there is not exist another region will be attached or detached after spin_unlock().

Well, as for local heartbeat mode, I think, we still need to protect that global list.

Thanks,
Changwei

Quoted 39 lines

Thank,
Alex

Quoted 34 lines
Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
   		/* Ignore ENOENT only for local hb (userdlm domain) */
   		ret = o2nm_depend_item(&reg->hr_item);
   		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
   			else {
   				mlog(ML_ERROR, "Pin region %s fails with %d\n",
   				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
   				break;
   			}
   		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
   skip_pin:
   		if (found)
   			break;
-- 1.9.5.msysgit.1

.

.

author
#7 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Changwei,

Quoted 89 lines · Changwei Ge

Hi Alex,

Quoted 16 lines · alex chen

Hi Changwei,

Thanks for you reply.

Quoted 10 lines · Changwei Ge

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

Quoted 7 lines

It is a very very bad idea to sleep while holding a spin lock.
If a process grabs a spinlock and goes to sleep before releasing it. Then this process yields the control of the CPU to a second process. Unfortunately the second process also need to grab the spinlock and it will busy wait. On an uniprocessor machine the second process will lock the CPU not allowing the first process to wake up and release the spinlock so the second process can't continue, it is basically a deadlock.

I agree. This soft lockup truly exists. This point is OK for me.

Quoted 51 lines
Quoted 51 lines
Quoted 50 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user
spin_lock(&o2hb_live_lock)

    o2hb_region_pin
     o2nm_depend_item
      configfs_depend_item
       inode_lock
        down_write
        -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
   fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
   1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
   		if (reg->hr_item_pinned || reg->hr_item_dropped)
   			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Quoted 8 lines
Quoted 4 lines

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

In global heartbeat mode, we pin all regions if there is the first dependent user and unpin all regions if the number of dependent users falls to zero.
So there is not exist another region will be attached or detached after spin_unlock().

Quoted 6 lines

Well, as for local heartbeat mode, I think, we still need to protect that global list.

Thanks,
Changwei

For the local heartbeat mode, as I said before, here we already found the region and will break the loop after depending item, so we just need to protect the region during depending item. The global list doesn't need to protect anymore.

I find a problem in my patch. The reg->hr_item_pinned should be protect by the o2hb_live_lock, so we should call spin_lock(&o2hb_live_lock) immediately after o2nm_depend_item(); I will modified the patch and resend the patch.

Thank,
Alex

Quoted 43 lines
Quoted 39 lines

Thank,
Alex

Quoted 34 lines
Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
   		/* Ignore ENOENT only for local hb (userdlm domain) */
   		ret = o2nm_depend_item(&reg->hr_item);
   		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
   			else {
   				mlog(ML_ERROR, "Pin region %s fails with %d\n",
   				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
   				break;
   			}
   		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
   skip_pin:
   		if (found)
   			break;
-- 1.9.5.msysgit.1

.

.

.

#8 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Quoted 95 lines · alex chen

Hi Changwei,

Quoted 92 lines · Changwei Ge

Hi Alex,

Quoted 16 lines · alex chen

Hi Changwei,

Thanks for you reply.

Quoted 10 lines · Changwei Ge

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

Quoted 7 lines

It is a very very bad idea to sleep while holding a spin lock.
If a process grabs a spinlock and goes to sleep before releasing it. Then this process yields the control of the CPU to a second process. Unfortunately the second process also need to grab the spinlock and it will busy wait. On an uniprocessor machine the second process will lock the CPU not allowing the first process to wake up and release the spinlock so the second process can't continue, it is basically a deadlock.

I agree. This soft lockup truly exists. This point is OK for me.

Quoted 51 lines
Quoted 51 lines
Quoted 50 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user

    spin_lock(&o2hb_live_lock)
     o2hb_region_pin
      o2nm_depend_item
       configfs_depend_item
        inode_lock
         down_write
         -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
    fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
    1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
    		if (reg->hr_item_pinned || reg->hr_item_dropped)
    			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Quoted 8 lines
Quoted 4 lines

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

In global heartbeat mode, we pin all regions if there is the first dependent user and unpin all regions if the number of dependent users falls to zero.
So there is not exist another region will be attached or detached after spin_unlock().

Well, as for local heartbeat mode, I think, we still need to protect that global list.

Quoted 6 lines
Quoted 3 lines

Thanks,
Changwei

For the local heartbeat mode, as I said before, here we already found the region and will break the loop after depending item, so we just need to protect the region during depending item. The global list doesn't need to protect anymore.

Hi Alex,
Um, I think this is too tricky.

Quoted 4 lines

I find a problem in my patch. The reg->hr_item_pinned should be protect by the o2hb_live_lock, so we should call spin_lock(&o2hb_live_lock) immediately after o2nm_depend_item(); I will modified the patch and resend the patch

Well, if you do so to revise your patch, how can you keep the consistency between setting ::hr_item_pinned and pining ::hr_item.

What will ::hr_item_pinned stand for against your revision? I think this fix is fragile.

Besides that the issue reported by Fungguang seems to be a crash issue rather than a lockup one.

Thanks,
Changwei

Quoted 48 lines

Thank,
Alex

Quoted 43 lines
Quoted 39 lines

Thank,
Alex

Quoted 34 lines
Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
    		/* Ignore ENOENT only for local hb (userdlm domain) */
    		ret = o2nm_depend_item(&reg->hr_item);
    		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
    			else {
    				mlog(ML_ERROR, "Pin region %s fails with %d\n",
    				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
    				break;
    			}
    		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
    skip_pin:
    		if (found)
    			break;
-- 1.9.5.msysgit.1

.

.

.

author
#9 lore raw

PATCH 1/1 ocfs2/cluster: unlock the o2hb_live_lock before the o2nm_depend_item()

Hi Changwei,

Thanks for you reply.

Quoted 99 lines · Changwei Ge
Quoted 98 lines · alex chen

Hi Changwei,

Quoted 95 lines · Changwei Ge

Hi Alex,

Quoted 16 lines · alex chen

Hi Changwei,

Thanks for you reply.

Quoted 10 lines · Changwei Ge

Hi Alex,

Quoted 4 lines · alex chen

Hi Joseph and Changwei,

It's our basic principle that the function in which may sleep can't be called within spinlock hold.

I suppose this principle is a suggestion not a restriction.

Quoted 7 lines

It is a very very bad idea to sleep while holding a spin lock.
If a process grabs a spinlock and goes to sleep before releasing it. Then this process yields the control of the CPU to a second process. Unfortunately the second process also need to grab the spinlock and it will busy wait. On an uniprocessor machine the second process will lock the CPU not allowing the first process to wake up and release the spinlock so the second process can't continue, it is basically a deadlock.

I agree. This soft lockup truly exists. This point is OK for me.

Quoted 51 lines
Quoted 51 lines
Quoted 50 lines · Joseph Qi
Quoted 32 lines

Hi Alex,

Quoted 29 lines · alex chen

In the following situation, the down_write() will be called under the spin_lock(), which may lead a soft lockup:
o2hb_region_inc_user

    spin_lock(&o2hb_live_lock)
     o2hb_region_pin
      o2nm_depend_item
       configfs_depend_item
        inode_lock
         down_write
         -->here may sleep and reschedule

So we should unlock the o2hb_live_lock before the o2nm_depend_item(), and get item reference in advance to prevent the region to be released.

Signed-off-by Alex Chen <[email protected]>
Reviewed-by Yiwen Jiang <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
    fs/ocfs2/cluster/heartbeat.c | 8 ++++++++
    1 file changed, 8 insertions(+)
fs/ocfs2/cluster/heartbeat.c +0 -0
index d020604..f1142a9 100644
@@ -2399,6 +2399,9 @@ static int o2hb_region_pin(const char *region_uuid)
    		if (reg->hr_item_pinned || reg->hr_item_dropped)
    			goto skip_pin;
Quoted 8 lines

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid) · line 2404

Quoted 3 lines
fs/ocfs2/cluster/heartbeat.c +3 -0
+		config_item_get(&reg->hr_item);
+		spin_unlock(&o2hb_live_lock);
+

If unlock here, the iteration of o2hb_all_regions is no longer safe.

Thanks,
Joseph

In local heartbeat mode, here we already found the region and will break the loop after depending item, we get the item reference before spin_unlock(), that means the region will never be released by the o2hb_region_release() until we put the item reference after spin_lock(&o2hb_live_lock), so we can safely iterate over the list.
In global heartbeat mode, it doesn't matter that some regions may be deleted after spin_unlock(), because we just pin all the active regions.

Quoted 8 lines
Quoted 4 lines

But we are still iterating elements on global list *o2hb_all_regions*, right? How can we guarantee that no more elements will be attached or detached while the lock is unlocked.

In global heartbeat mode, we pin all regions if there is the first dependent user and unpin all regions if the number of dependent users falls to zero.
So there is not exist another region will be attached or detached after spin_unlock().

Well, as for local heartbeat mode, I think, we still need to protect that global list.

Thanks,
Changwei

Quoted 7 lines
Quoted 3 lines

For the local heartbeat mode, as I said before, here we already found the region and will break the loop after depending item, so we just need to protect the region during depending item. The global list doesn't need to protect anymore.

Hi Alex,
Um, I think this is too tricky.

The o2hb_live_lock mainly protect the "o2hb_all_regions" not the region and we will not iterate the list o2hb_all_regions any more after we find the region. So we can unlock here. If you have any other opinions, please let me know and I really appreciate you if you can express your opinions as detailed as possible.

Quoted 10 lines
Quoted 4 lines

I find a problem in my patch. The reg->hr_item_pinned should be protect by the o2hb_live_lock, so we should call spin_lock(&o2hb_live_lock) immediately after o2nm_depend_item(); I will modified the patch and resend the patch

Well, if you do so to revise your patch, how can you keep the consistency between setting ::hr_item_pinned and pining ::hr_item.

What will ::hr_item_pinned stand for against your revision? I think this fix is fragile.

Firstly, the ::hr_item_pinned is set in o2hb_region_pin() when mounting filesystem and cleared in o2hb_region_unpin() when umounting filesystem. mount and umount the filesystem can only be called one by one. So the ::hr_item_pinned will not be set and cleared concurrently. Secondly, Although the o2hb_region_pin() is multiple calls, but are called serial in the function dlm_register_domain_handlers(). So the ::hr_item_pinned will not be set concurrently. So we don't need to worry about the consistency of the ::hr_item_pinned.

Quoted 6 lines

Besides that the issue reported by Fungguang seems to be a crash issue rather than a lockup one.

Thanks,
Changwei

Um, I think the CONFIG_DEBUG_ATOMIC_SLEEP had been defined in the kernel tested by Fengguang, so the warning "BUG: sleeping function..." could be printed. But normally, the CONFIG_DEBUG_ATOMIC_SLEEP would not be defined in the released kernel. so it is a soft lockup. You can read the function ___might_sleep() for more details.

Thanks,
Alex

Quoted 52 lines
Quoted 48 lines

Thank,
Alex

Quoted 43 lines
Quoted 39 lines

Thank,
Alex

Quoted 34 lines
Quoted 30 lines

Thanks,
Alex

Quoted 24 lines
Quoted 11 lines
fs/ocfs2/cluster/heartbeat.c +2 -0
    		/* Ignore ENOENT only for local hb (userdlm domain) */
    		ret = o2nm_depend_item(&reg->hr_item);
    		if (!ret) {

@@ -2410,9 +2413,14 @@ static int o2hb_region_pin(const char *region_uuid)
    			else {
    				mlog(ML_ERROR, "Pin region %s fails with %d\n",
    				     uuid, ret);
+				config_item_put(&reg->hr_item);
+				spin_lock(&o2hb_live_lock);

fs/ocfs2/cluster/heartbeat.c static int o2hb_region_pin(const char *region_uuid)

Quoted 10 lines
fs/ocfs2/cluster/heartbeat.c +3 -1
    				break;
    			}
    		}
+
+		config_item_put(&reg->hr_item);
+		spin_lock(&o2hb_live_lock);
    skip_pin:
    		if (found)
    			break;
-- 1.9.5.msysgit.1

.

.

.

.