| Message ID | 20190828064749.GA165571@LGEARND20B15 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Use WARN_ON rather than BUG() for bailout mount-operation | expand |
On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > 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(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 322da69..10fe000 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -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 <darrick.wong@oracle.com> --D > spin_unlock(&mp->m_perag_lock); > radix_tree_preload_end(); > error = -EEXIST; > -- > 2.6.2 >
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 <darrick.wong@oracle.com>님이 작성: > > On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > > 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(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index 322da69..10fe000 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -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 <darrick.wong@oracle.com> > > --D > > > spin_unlock(&mp->m_perag_lock); > > radix_tree_preload_end(); > > error = -EEXIST; > > -- > > 2.6.2 > >
On Thu, Aug 29, 2019 at 06:30:43AM +0900, Austin Kim wrote: > 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 > BR, > Guillermo Austin Kim > > 2019년 8월 29일 (목) 오전 12:14, Darrick J. Wong <darrick.wong@oracle.com>님이 작성: > > > > On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > > > 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(-) > > > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > > index 322da69..10fe000 100644 > > > --- a/fs/xfs/xfs_mount.c > > > +++ b/fs/xfs/xfs_mount.c > > > @@ -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 <darrick.wong@oracle.com> > > > > --D > > > > > spin_unlock(&mp->m_perag_lock); > > > radix_tree_preload_end(); > > > error = -EEXIST; > > > -- > > > 2.6.2 > > >
On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > 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(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 322da69..10fe000 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -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.
2019년 8월 29일 (목) 오후 4:56, Christoph Hellwig <hch@infradead.org>님이 작성: > > On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > > 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(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index 322da69..10fe000 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -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. > 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) 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? 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. diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 322da69..d831c13 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -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
On Thu, Aug 29, 2019 at 12:56:55AM -0700, Christoph Hellwig wrote: > On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > > 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(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index 322da69..10fe000 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -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. 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
On Thu, Aug 29, 2019 at 09:59:17PM +0900, Austin Kim wrote: > 2019년 8월 29일 (목) 오후 4:56, Christoph Hellwig <hch@infradead.org>님이 작성: > > > > On Wed, Aug 28, 2019 at 03:47:49PM +0900, Austin Kim wrote: > > > 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(-) > > > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > > index 322da69..10fe000 100644 > > > --- a/fs/xfs/xfs_mount.c > > > +++ b/fs/xfs/xfs_mount.c > > > @@ -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. > > > 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) > > 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 > 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. > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index 322da69..d831c13 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -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
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 322da69..10fe000 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -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;