xfs: Use WARN_ON rather than BUG() for bailout mount-operation

PATCH In review Austin Kim 3 participants 8 messages Aug 28, 2019 – Aug 29, 2019 XFS filesystem

Share & tools

Austin Kim

author

2019-08-28 06:47 UTC #0 lore raw

PATCH 1/1 xfs: Use WARN_ON rather than BUG() for bailout mount-operation

If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.

For this reason, using WARN_ON rather than BUG() could prevent this situation.

---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/xfs/xfs_mount.c +1 -2

index 322da69..10fe000 100644
@@ -213,8 +213,7 @@ xfs_initialize_perag(
 			goto out_hash_destroy;
 
 		spin_lock(&mp->m_perag_lock);
-		if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-			BUG();
+		if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){
 			spin_unlock(&mp->m_perag_lock);
 			radix_tree_preload_end();
 			error = -EEXIST;

Signature · 1 line

2.6.2

Darrick J. Wong

Reviewed-by on v1 1/1

2019-08-28 15:14 UTC #1 lore raw

Quoted 13 lines · Austin Kim

If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644

fs/xfs/xfs_mount.c xfs_initialize_perag( · line 216

Quoted 6 lines

fs/xfs/xfs_mount.c +1 -2

@@ -213,8 +213,7 @@ xfs_initialize_perag(
 			goto out_hash_destroy;
 
 		spin_lock(&mp->m_perag_lock);
-		if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-			BUG();
+		if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){

Need a space before the brace.

Will fix on import,

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 6 lines

fs/xfs/xfs_mount.c +0 -0

 			spin_unlock(&mp->m_perag_lock);
 			radix_tree_preload_end();
 			error = -EEXIST;

Signature · 1 line

2.6.2

Austin Kim

author

2019-08-28 21:30 UTC #2 lore raw

Dear Mr. Darrick J. Wong

Thanks for reviewing patch. BTW, I have a question for you.

Do I have to update the patch again with 'a space before the brace'? Or could I just wait for the patch to be imported?

It would be thankful if you give me a feedback.

BR,
Guillermo Austin Kim

2019년 8월 29일 (목) 오전 12:14, Darrick J. Wong <[email protected]>님이 작성:

Quoted 35 lines · Austin Kim

Quoted 13 lines
If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644
fs/xfs/xfs_mount.c xfs_initialize_perag( · line 216
Quoted 6 lines
fs/xfs/xfs_mount.c +1 -2
@@ -213,8 +213,7 @@ xfs_initialize_perag(
                      goto out_hash_destroy;
 
              spin_lock(&mp->m_perag_lock);
-             if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-                     BUG();
+             if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){
Need a space before the brace.
Will fix on import,
Reviewed-by Darrick J. Wong <[email protected]>
--D
Quoted 6 lines
fs/xfs/xfs_mount.c +0 -1
                      spin_unlock(&mp->m_perag_lock);
                      radix_tree_preload_end();
                      error = -EEXIST;
--
2.6.2

Darrick J. Wong

2019-08-28 21:51 UTC #3 lore raw

Quoted 8 lines · Austin Kim

Dear Mr. Darrick J. Wong
Thanks for reviewing patch. BTW, I have a question for you.
Do I have to update the patch again with 'a space before the brace'? Or could I just wait for the patch to be imported?
It would be thankful if you give me a feedback.

I made the correction when I imported your patch into my testing tree, so you don't need to resubmit the patch just to fix that one thing.

However, if you decide to send a new revision *for some other reason*, then please make the correction in your own tree before resubmitting.

--D

Quoted 39 lines

BR,
Guillermo Austin Kim
2019년 8월 29일 (목) 오전 12:14, Darrick J. Wong <[email protected]>님이 작성:
Quoted 35 lines · Austin Kim
Quoted 13 lines
If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644
fs/xfs/xfs_mount.c xfs_initialize_perag( · line 216
Quoted 6 lines
fs/xfs/xfs_mount.c +1 -2
@@ -213,8 +213,7 @@ xfs_initialize_perag(
                      goto out_hash_destroy;
 
              spin_lock(&mp->m_perag_lock);
-             if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-                     BUG();
+             if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){
Need a space before the brace.
Will fix on import,
Reviewed-by Darrick J. Wong <[email protected]>
--D
Quoted 6 lines
fs/xfs/xfs_mount.c +0 -1
                      spin_unlock(&mp->m_perag_lock);
                      radix_tree_preload_end();
                      error = -EEXIST;
--
2.6.2

Christoph Hellwig

2019-08-29 08:18 UTC #4 lore raw

In reply to Austin Kim

Quoted 13 lines · Austin Kim

If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644

fs/xfs/xfs_mount.c xfs_initialize_perag( · line 216

Quoted 6 lines

fs/xfs/xfs_mount.c +1 -2

@@ -213,8 +213,7 @@ xfs_initialize_perag(
 			goto out_hash_destroy;
 
 		spin_lock(&mp->m_perag_lock);
-		if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-			BUG();
+		if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){

Please make this a WARN_ON_ONCE so that we don't see a flodding of messages in case of this error.

Darrick J. Wong

2019-08-29 15:58 UTC #6 lore raw

Quoted 31 lines · Austin Kim

2019년 8월 29일 (목) 오후 4:56, Christoph Hellwig <[email protected]>님이 작성:
Quoted 17 lines · Austin Kim
Quoted 15 lines
If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644
@@ -213,8 +213,7 @@ xfs_initialize_perag(
                      goto out_hash_destroy;
 
Quoted 8 lines
fs/xfs/xfs_mount.c xfs_initialize_perag(
Quoted 4 lines
fs/xfs/xfs_mount.c +1 -2
              spin_lock(&mp->m_perag_lock);
-             if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-                     BUG();
+             if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){
Please make this a WARN_ON_ONCE so that we don't see a flodding of messages in case of this error.
Hello, Mr. Christoph
Thanks for good feedback.
If the kernel log is flooded with error message, as you pointed out, it may cause other side-effect.(e.g: system non-responsive or lockup)

Quoted 4 lines

To. Mr. Darrick J. Wong
If you or other kernel developers do not disagree with the idea(WARN_ON_ONCE instead of WARN_ON),
do I have to resend the patch with new revision?

Yes, and please add your Signed-off-by in the new revision.

--D

Quoted 29 lines

The title, the commit message and patch might be changed as followings;
======
xfs: Use WARN_ON_ONCE rather than BUG() for bailout mount-operation
If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON_ONCE rather than BUG() could prevent this situation.
fs/xfs/xfs_mount.c +1 -2
index 322da69..d831c13 100644
@@ -213,8 +213,7 @@ xfs_initialize_perag(
                        goto out_hash_destroy;
 
                spin_lock(&mp->m_perag_lock);
-               if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-                       BUG();
+               if (WARN_ON_ONCE(radix_tree_insert(&mp->m_perag_tree,
 index, pag))) {
                        spin_unlock(&mp->m_perag_lock);
                        radix_tree_preload_end();
                        error = -EEXIST;
======
BR,
Guillermo Austin Kim

Darrick J. Wong

2019-08-29 15:01 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 16 lines · Christoph Hellwig

Quoted 15 lines · Austin Kim
If the CONFIG_BUG is enabled, BUG() is executed and then system is crashed. However, the bailout for mount is no longer proceeding.
For this reason, using WARN_ON rather than BUG() could prevent this situation.
---
 fs/xfs/xfs_mount.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_mount.c +0 -0
index 322da69..10fe000 100644
@@ -213,8 +213,7 @@ xfs_initialize_perag(
 			goto out_hash_destroy;
 

Quoted 7 lines

fs/xfs/xfs_mount.c xfs_initialize_perag( · line 216
Quoted 4 lines
fs/xfs/xfs_mount.c +1 -2
 		spin_lock(&mp->m_perag_lock);
-		if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
-			BUG();
+		if (WARN_ON(radix_tree_insert(&mp->m_perag_tree, index, pag))){
Please make this a WARN_ON_ONCE so that we don't see a flodding of messages in case of this error.

How would it flood? If the radix tree insertion fails we dump all the pag structures and fail log recovery / mount / growfs. I suppose if one were out of memory and hammering the system hard with repeated mount calls...

--D