[PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.

[PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.

[raghu.prabhu13]

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

This is to prevent xfs_log_force from printing error message continuously (due
to xfs_sync and others) till umount if the disk has been forcefully unplugged.

This is to prevent messages like these from being displayed repeatedly.

[ 3873.009329] XFS (sdb3): xfs_log_force: error 5 returned.

Note, that even after xfs_do_force_shutdown has been called, xfs_log_force
doesn't stop till the filesystem has been unmounted (and it keeps printing
"error 5 returned" to kernel log). Also, xfs_do_force_shutdown is called
repeatedly on an already shutdown filesystem through xfs_fs_sync_fs.

To fix this, added condition over XFS_FORCED_SHUTDOWN to xfs_log_force and
xfs_fs_sync_fs.

To simulate it, mount an xfs filesystem located on external disk, and then pull
the power to the disk.

Now, the dmesg looks,

[  268.307303] XFS (sdb2): xfs_do_force_shutdown(0x1) called from line 1031 of file fs/xfs/xfs_buf.c.  Return address = 0xffffffff8127c13a
[  268.307318] XFS (sdb2): I/O Error Detected. Shutting down filesystem
[  268.307323] XFS (sdb2): Please umount the filesystem and rectify the problem(s)

---

Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and xfs_flush_worker.
Version 3: Removed calling xfs_syncd_stop in xfs_bwrite.
Version 4: Added return statements to xfs_log_force and xfs_fs_sync_fs.
Version 5: As per suggestion, added xfs_warn to xfs_fs_sync_fs and removed EIO
return in xfs_log_force

 Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
 Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 fs/xfs/xfs_log.c   | 5 ++++-
 fs/xfs/xfs_super.c | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 7f4f937..161c925 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3002,7 +3002,10 @@ xfs_log_force(
 
 	trace_xfs_log_force(mp, 0);
 	error = _xfs_log_force(mp, flags, NULL);
-	if (error)
+	/*
+	 * Avoid warning when the filesystem has already shutdown.
+	 */
+	if (error && !XFS_FORCED_SHUTDOWN(mp))
 		xfs_warn(mp, "%s: error %d returned.", __func__, error);
 }
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bdaf4cb..ebafc99 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -943,6 +943,11 @@ xfs_fs_sync_fs(
 	if (!wait)
 		return 0;
 
+	if (XFS_FORCED_SHUTDOWN(mp)) {
+		xfs_warn(mp, "Sync called: filesystem already shutdown");
+		return XFS_ERROR(EIO);
+	}
+
 	error = xfs_quiesce_data(mp);
 	if (error)
 		return -error;
-- 
1.7.12

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.

[Carlos Maiolino]

Hi, particularly, these messages are annoying to me too, and it's great to see
this patch, have some comments though.

> This is to prevent xfs_log_force from printing error message continuously (due
> to xfs_sync and others) till umount if the disk has been forcefully unplugged.
> 
> This is to prevent messages like these from being displayed repeatedly.
> 
> [ 3873.009329] XFS (sdb3): xfs_log_force: error 5 returned.
> 
> Note, that even after xfs_do_force_shutdown has been called, xfs_log_force
> doesn't stop till the filesystem has been unmounted (and it keeps printing
> "error 5 returned" to kernel log). Also, xfs_do_force_shutdown is called
> repeatedly on an already shutdown filesystem through xfs_fs_sync_fs.
> 
> To fix this, added condition over XFS_FORCED_SHUTDOWN to xfs_log_force and
> xfs_fs_sync_fs.
> 
> To simulate it, mount an xfs filesystem located on external disk, and then pull
> the power to the disk.
> 
> Now, the dmesg looks,
> 
> [  268.307303] XFS (sdb2): xfs_do_force_shutdown(0x1) called from line 1031 of file fs/xfs/xfs_buf.c.  Return address = 0xffffffff8127c13a
> [  268.307318] XFS (sdb2): I/O Error Detected. Shutting down filesystem
> [  268.307323] XFS (sdb2): Please umount the filesystem and rectify the problem(s)
> 

To be honest this commit message looks a little confusing to me, I'd prefer
something a little more direct and clear, but this is just my humble opinion.

I'd say something like:

Prevents xfs_log_force from print continuously error messages after filesystem
has been shutdown:

[ 3873.009329] XFS (sdb3): xfs_log_force: error 5 returned.

> ---
> 
> Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
> Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and xfs_flush_worker.
> Version 3: Removed calling xfs_syncd_stop in xfs_bwrite.
> Version 4: Added return statements to xfs_log_force and xfs_fs_sync_fs.
> Version 5: As per suggestion, added xfs_warn to xfs_fs_sync_fs and removed EIO
> return in xfs_log_force
> 
>  Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>  Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  fs/xfs/xfs_log.c   | 5 ++++-
>  fs/xfs/xfs_super.c | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7f4f937..161c925 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3002,7 +3002,10 @@ xfs_log_force(
>  
>  	trace_xfs_log_force(mp, 0);
>  	error = _xfs_log_force(mp, flags, NULL);
> -	if (error)
> +	/*
> +	 * Avoid warning when the filesystem has already shutdown.
> +	 */
> +	if (error && !XFS_FORCED_SHUTDOWN(mp))
>  		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index bdaf4cb..ebafc99 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -943,6 +943,11 @@ xfs_fs_sync_fs(
>  	if (!wait)
>  		return 0;
>  
> +	if (XFS_FORCED_SHUTDOWN(mp)) {
> +		xfs_warn(mp, "Sync called: filesystem already shutdown");
> +		return XFS_ERROR(EIO);
> +	}
> +

Looks like this will keep printing error messages every time a sync() happens,
either via flusher threads or manually by user once xfs_fs_sync_fs() will
still be called for it.

I would suggest to add some static variable here to check if the filesystem has
been already shutted down before print the message again, to also avoid message
flooding due xfs_fs_sync_fs() calls although I'm not sure another people will
agree with it.

Despit my comments, this looks good for me

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

-- 
--Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.

[Dave Chinner]

On Sat, Sep 08, 2012 at 03:28:11AM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> This is to prevent xfs_log_force from printing error message continuously (due
> to xfs_sync and others) till umount if the disk has been forcefully unplugged.
> 
> This is to prevent messages like these from being displayed repeatedly.
> 
> [ 3873.009329] XFS (sdb3): xfs_log_force: error 5 returned.
> 
> Note, that even after xfs_do_force_shutdown has been called, xfs_log_force
> doesn't stop till the filesystem has been unmounted (and it keeps printing
> "error 5 returned" to kernel log). Also, xfs_do_force_shutdown is called
> repeatedly on an already shutdown filesystem through xfs_fs_sync_fs.
> 
> To fix this, added condition over XFS_FORCED_SHUTDOWN to xfs_log_force and
> xfs_fs_sync_fs.
> 
> To simulate it, mount an xfs filesystem located on external disk, and then pull
> the power to the disk.
> 
> Now, the dmesg looks,
> 
> [  268.307303] XFS (sdb2): xfs_do_force_shutdown(0x1) called from line 1031 of file fs/xfs/xfs_buf.c.  Return address = 0xffffffff8127c13a
> [  268.307318] XFS (sdb2): I/O Error Detected. Shutting down filesystem
> [  268.307323] XFS (sdb2): Please umount the filesystem and rectify the problem(s)
> 
> ---
> 
> Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
> Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and xfs_flush_worker.
> Version 3: Removed calling xfs_syncd_stop in xfs_bwrite.
> Version 4: Added return statements to xfs_log_force and xfs_fs_sync_fs.
> Version 5: As per suggestion, added xfs_warn to xfs_fs_sync_fs and removed EIO
> return in xfs_log_force
> 
>  Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>  Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>

Signed-off-by implies you tested it. Tested-by is used to
acknowledge third party testers, usually in the case to acknowledge
the fix was Tested-by the Reported-by...

And the signed-off-by belongs in the commit message (i.e. about the
first ---) not in the section after the commit message that is
stripped by the git tools.

> ---
>  fs/xfs/xfs_log.c   | 5 ++++-
>  fs/xfs/xfs_super.c | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7f4f937..161c925 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3002,7 +3002,10 @@ xfs_log_force(
>  
>  	trace_xfs_log_force(mp, 0);
>  	error = _xfs_log_force(mp, flags, NULL);
> -	if (error)
> +	/*
> +	 * Avoid warning when the filesystem has already shutdown.
> +	 */
> +	if (error && !XFS_FORCED_SHUTDOWN(mp))
>  		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index bdaf4cb..ebafc99 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -943,6 +943,11 @@ xfs_fs_sync_fs(
>  	if (!wait)
>  		return 0;
>  
> +	if (XFS_FORCED_SHUTDOWN(mp)) {
> +		xfs_warn(mp, "Sync called: filesystem already shutdown");
> +		return XFS_ERROR(EIO);
> +	}

Why should we emit a warning here? There's already information in the
log to indicate the filesystem is shut down...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: Re: [PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.

[Raghavendra D Prabhu]

[-- Attachment #1.1: Type: text/plain, Size: 2761 bytes --]

Hi,


* On Wed, Sep 12, 2012 at 07:05:12AM +1000, Dave Chinner <david@fromorbit.com> wrote:
>On Sat, Sep 08, 2012 at 03:28:11AM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
.....
....
...

>>
>> Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
>> Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and xfs_flush_worker.
>> Version 3: Removed calling xfs_syncd_stop in xfs_bwrite.
>> Version 4: Added return statements to xfs_log_force and xfs_fs_sync_fs.
>> Version 5: As per suggestion, added xfs_warn to xfs_fs_sync_fs and removed EIO
>> return in xfs_log_force
>>
>>  Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>  Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>
>Signed-off-by implies you tested it. Tested-by is used to
>acknowledge third party testers, usually in the case to acknowledge
>the fix was Tested-by the Reported-by...
Sorry about that, I meant to add only the Signed-off part.

>
>And the signed-off-by belongs in the commit message (i.e. about the
>first ---) not in the section after the commit message that is
>stripped by the git tools.

I will fix that.

>
>> ---
>>  fs/xfs/xfs_log.c   | 5 ++++-
>>  fs/xfs/xfs_super.c | 5 +++++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
>> index 7f4f937..161c925 100644
>> --- a/fs/xfs/xfs_log.c
>> +++ b/fs/xfs/xfs_log.c
>> @@ -3002,7 +3002,10 @@ xfs_log_force(
>>
>>  	trace_xfs_log_force(mp, 0);
>>  	error = _xfs_log_force(mp, flags, NULL);
>> -	if (error)
>> +	/*
>> +	 * Avoid warning when the filesystem has already shutdown.
>> +	 */
>> +	if (error && !XFS_FORCED_SHUTDOWN(mp))
>>  		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>>  }
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index bdaf4cb..ebafc99 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -943,6 +943,11 @@ xfs_fs_sync_fs(
>>  	if (!wait)
>>  		return 0;
>>
>> +	if (XFS_FORCED_SHUTDOWN(mp)) {
>> +		xfs_warn(mp, "Sync called: filesystem already shutdown");
>> +		return XFS_ERROR(EIO);
>> +	}
>
>Why should we emit a warning here? There's already information in the
>log to indicate the filesystem is shut down...

Actually in a previous review, you had suggested to add 
information about it since 'sync' is called manually/less 
often, so better to have more verbosity about it and user gets 
feedback on it. So, is this fine?

>
>Cheers,
>
>Dave.
>-- 
>Dave Chinner
>david@fromorbit.com
>




Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs