make log force warnings less annoying

make log force warnings less annoying

[Christoph Hellwig]

I've recently had various issues root causing customer bug reports
due to xfs_log_force spamming the log with EIO warnings that are not
helpful at all.  This patch just removes them as they add no value.

I'd love to see this patch go into the next 4.10-rc and 4.9-stable
so that I can improve the quality of bug reports.

[PATCH] xfs: don't print warnings when xfs_log_force fails

[Christoph Hellwig]

There are only two reasons for xfs_log_force / xfs_log_force_lsn to fail:
one is an I/O error, for which xlog_bdstrat already logs a warning, and
the second is an already shutdown log due to a previous I/O errors.  In
the latter case we'll already have a previous indication for the actual
error, but the large stream of misleading warnings from xfs_log_force
will probably scroll it out of the message buffer.

Simply removing the warnings thus makes the XFS log reporting significantly
better.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index c39ac14..b1469f0 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3317,12 +3317,8 @@ xfs_log_force(
 	xfs_mount_t	*mp,
 	uint		flags)
 {
-	int	error;
-
 	trace_xfs_log_force(mp, 0, _RET_IP_);
-	error = _xfs_log_force(mp, flags, NULL);
-	if (error)
-		xfs_warn(mp, "%s: error %d returned.", __func__, error);
+	_xfs_log_force(mp, flags, NULL);
 }
 
 /*
@@ -3466,12 +3462,8 @@ xfs_log_force_lsn(
 	xfs_lsn_t	lsn,
 	uint		flags)
 {
-	int	error;
-
 	trace_xfs_log_force(mp, lsn, _RET_IP_);
-	error = _xfs_log_force_lsn(mp, lsn, flags, NULL);
-	if (error)
-		xfs_warn(mp, "%s: error %d returned.", __func__, error);
+	_xfs_log_force_lsn(mp, lsn, flags, NULL);
 }
 
 /*
-- 
2.1.4

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Alex Elder]

On 12/29/2016 02:23 AM, Christoph Hellwig wrote:
> There are only two reasons for xfs_log_force / xfs_log_force_lsn to fail:
> one is an I/O error, for which xlog_bdstrat already logs a warning, and
> the second is an already shutdown log due to a previous I/O errors.  In
> the latter case we'll already have a previous indication for the actual
> error, but the large stream of misleading warnings from xfs_log_force
> will probably scroll it out of the message buffer.
> 
> Simply removing the warnings thus makes the XFS log reporting significantly
> better.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This sounds like a good idea, though I haven't really reviewed
the code itself.  But I'd say a one line comment indicating why
errors from _xfs_log_force() are being ignored might be good.

					-Alex

> ---
>  fs/xfs/xfs_log.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c39ac14..b1469f0 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3317,12 +3317,8 @@ xfs_log_force(
>  	xfs_mount_t	*mp,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, 0, _RET_IP_);
> -	error = _xfs_log_force(mp, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force(mp, flags, NULL);
>  }
>  
>  /*
> @@ -3466,12 +3462,8 @@ xfs_log_force_lsn(
>  	xfs_lsn_t	lsn,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, lsn, _RET_IP_);
> -	error = _xfs_log_force_lsn(mp, lsn, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force_lsn(mp, lsn, flags, NULL);
>  }
>  
>  /*
> 

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Christoph Hellwig]

On Thu, Dec 29, 2016 at 08:42:08AM -0600, Alex Elder wrote:
> This sounds like a good idea, though I haven't really reviewed
> the code itself.  But I'd say a one line comment indicating why
> errors from _xfs_log_force() are being ignored might be good.

I don't really know the answer, but it probabls is: some callers
shouldn't ignore it and others can't handle it either.  Maybe I'll
need to some further work in this area, but it's not really
related to dropping the message.

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Alex Elder]

On 12/29/2016 12:09 PM, Christoph Hellwig wrote:
> On Thu, Dec 29, 2016 at 08:42:08AM -0600, Alex Elder wrote:
>> This sounds like a good idea, though I haven't really reviewed
>> the code itself.  But I'd say a one line comment indicating why
>> errors from _xfs_log_force() are being ignored might be good.
> 
> I don't really know the answer, but it probabls is: some callers
> shouldn't ignore it and others can't handle it either.  Maybe I'll
> need to some further work in this area, but it's not really
> related to dropping the message.
> 

I actually misspoke.

What I *meant* to say was that a comment about why we don't
want to log a message in these spots might be nice.  But
now I'm less interested in that and more interested in
why errors from _xfs_log_force*() can be ignored.

Anyway, I won't claim to have reviewed this, but your change
makes sense to me.

					-Alex

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Carlos Maiolino]

On Thu, Dec 29, 2016 at 09:23:19AM +0100, Christoph Hellwig wrote:
> There are only two reasons for xfs_log_force / xfs_log_force_lsn to fail:
> one is an I/O error, for which xlog_bdstrat already logs a warning, and
> the second is an already shutdown log due to a previous I/O errors.  In
> the latter case we'll already have a previous indication for the actual
> error, but the large stream of misleading warnings from xfs_log_force
> will probably scroll it out of the message buffer.
> 
> Simply removing the warnings thus makes the XFS log reporting significantly
> better.

Stop spamming the kernel log with the "error -5 returned." messages will be
great.

The code looks good too.

you can add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


Cheers

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c39ac14..b1469f0 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3317,12 +3317,8 @@ xfs_log_force(
>  	xfs_mount_t	*mp,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, 0, _RET_IP_);
> -	error = _xfs_log_force(mp, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force(mp, flags, NULL);
>  }
>  
>  /*
> @@ -3466,12 +3462,8 @@ xfs_log_force_lsn(
>  	xfs_lsn_t	lsn,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, lsn, _RET_IP_);
> -	error = _xfs_log_force_lsn(mp, lsn, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force_lsn(mp, lsn, flags, NULL);
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Christoph Hellwig]

On Thu, Dec 29, 2016 at 09:23:19AM +0100, Christoph Hellwig wrote:
> There are only two reasons for xfs_log_force / xfs_log_force_lsn to fail:
> one is an I/O error, for which xlog_bdstrat already logs a warning, and
> the second is an already shutdown log due to a previous I/O errors.  In
> the latter case we'll already have a previous indication for the actual
> error, but the large stream of misleading warnings from xfs_log_force
> will probably scroll it out of the message buffer.
> 
> Simply removing the warnings thus makes the XFS log reporting significantly
> better.

Darrick,

any opinion on this one?  I was hoping to sneak it into 4.10-rc still..

Re: [PATCH] xfs: don't print warnings when xfs_log_force fails

[Darrick J. Wong]

On Thu, Dec 29, 2016 at 09:23:19AM +0100, Christoph Hellwig wrote:
> There are only two reasons for xfs_log_force / xfs_log_force_lsn to fail:
> one is an I/O error, for which xlog_bdstrat already logs a warning, and
> the second is an already shutdown log due to a previous I/O errors.  In
> the latter case we'll already have a previous indication for the actual
> error, but the large stream of misleading warnings from xfs_log_force
> will probably scroll it out of the message buffer.
> 
> Simply removing the warnings thus makes the XFS log reporting significantly
> better.

Looks ok to me, so:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index c39ac14..b1469f0 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3317,12 +3317,8 @@ xfs_log_force(
>  	xfs_mount_t	*mp,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, 0, _RET_IP_);
> -	error = _xfs_log_force(mp, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force(mp, flags, NULL);
>  }
>  
>  /*
> @@ -3466,12 +3462,8 @@ xfs_log_force_lsn(
>  	xfs_lsn_t	lsn,
>  	uint		flags)
>  {
> -	int	error;
> -
>  	trace_xfs_log_force(mp, lsn, _RET_IP_);
> -	error = _xfs_log_force_lsn(mp, lsn, flags, NULL);
> -	if (error)
> -		xfs_warn(mp, "%s: error %d returned.", __func__, error);
> +	_xfs_log_force_lsn(mp, lsn, flags, NULL);
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html