[PATCH v4] Stop periodic syncing if filesystem is already shutdown.

[PATCH v4] Stop periodic syncing if filesystem is already shutdown.

[raghu.prabhu13]

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

This is to prevent xfs_log_force from running ad-infinitum (due to xfs_sync)
till umount if the disk has been forcefully unplugged.

This is also 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).

To fix it, added return statements to xfs_log_force and xfs_fs_sync_fs if the
filesystem is already shutdown -- based on XFS_FORCED_SHUTDOWN.

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

Tested it on latest linus tree.

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.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 6db1fef..e4192b2 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2932,6 +2932,13 @@ xfs_log_force(
 {
 	int	error;
 
+	/*
+	 * No need to printk here since xfs_bwrite already printks about xfs
+	 * shutdown if it has shutdown already.
+	 */
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return;
+
 	error = _xfs_log_force(mp, flags, NULL);
 	if (error)
 		xfs_warn(mp, "%s: error %d returned.", __func__, error);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index dab9a5f..b0f6041 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1010,6 +1010,13 @@ xfs_fs_sync_fs(
 	int			error;
 
 	/*
+	 * No need to printk here since xfs_bwrite already printks about xfs
+	 * shutdown if it has shutdown already.
+	 */
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return XFS_ERROR(EIO);
+
+	/*
 	 * Doing anything during the async pass would be counterproductive.
 	 */
 	if (!wait)
-- 
1.7.10.3

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

Re: [PATCH v4] Stop periodic syncing if filesystem is already shutdown.

[Dave Chinner]

On Thu, May 31, 2012 at 11:37:34PM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> This is to prevent xfs_log_force from running ad-infinitum (due to xfs_sync)
> till umount if the disk has been forcefully unplugged.

I'm not sure where versions 2 and 3 went - I haven't seen them....

> 
> This is also 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).
> 
> To fix it, added return statements to xfs_log_force and xfs_fs_sync_fs if the
> filesystem is already shutdown -- based on XFS_FORCED_SHUTDOWN.
> 
> To simulate it, mount an xfs filesystem located on external disk, and then pull
> the power to the disk.
> 
> Tested it on latest linus tree.
> 
> 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.
> 
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>   ---
> ---
>  fs/xfs/xfs_log.c   |    7 +++++++
>  fs/xfs/xfs_super.c |    7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 6db1fef..e4192b2 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2932,6 +2932,13 @@ xfs_log_force(
>  {
>  	int	error;
>  
> +	/*
> +	 * No need to printk here since xfs_bwrite already printks about xfs
> +	 * shutdown if it has shutdown already.
> +	 */
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return;
> +
>  	error = _xfs_log_force(mp, flags, NULL);
>  	if (error)
>  		xfs_warn(mp, "%s: error %d returned.", __func__, error);

I don't think this is the right place for the check. Depending on
the type of error that caused the shutdown, the log may still be
functioning and we need to be able to force the log so we don't lose
all the changes in memory. The internal log implementation has all
the checks necessary to avoid writing to the log if the log has
already been shut down, which is why you keep seeing the error
message.

What it appears to me is that you want to stop the error message
from being emitted when the log is shut down. The way to do that is
this:

 	error = _xfs_log_force(mp, flags, NULL);
-	if (error)
+	if (error && !XFS_FORCED_SHUTDOWN(mp))
 		xfs_warn(mp, "%s: error %d returned.", __func__, error);

Rather than avoining calling _xfs_log_force() altogether.

> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index dab9a5f..b0f6041 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1010,6 +1010,13 @@ xfs_fs_sync_fs(
>  	int			error;
>  
>  	/*
> +	 * No need to printk here since xfs_bwrite already printks about xfs
> +	 * shutdown if it has shutdown already.
> +	 */
> +	if (XFS_FORCED_SHUTDOWN(mp))
> +		return XFS_ERROR(EIO);


I think if someone runs sync or calls syncfs() we should emit an
error message indicating that it failed. We can't return an error to
sync(), so we are limited to writing to the syslog.  This is not
called periodically, so I don't see any problem with emitting an
error message here. Being verbose in error states is desirable - you
can never have too much information in the logs when something goes
wrong....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: Re: [PATCH v4] Stop periodic syncing if filesystem is already shutdown.

[Raghavendra D Prabhu]

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

Hi,


* On Tue, Jun 05, 2012 at 04:06:17PM +1000, Dave Chinner <david@fromorbit.com> wrote:
>On Thu, May 31, 2012 at 11:37:34PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> This is to prevent xfs_log_force from running ad-infinitum (due to xfs_sync)
>> till umount if the disk has been forcefully unplugged.
>
>I'm not sure where versions 2 and 3 went - I haven't seen them....

Actually I hadn't numbered them, but they are here http://oss.sgi.com/archives/xfs/2012-05/msg00065.html (starting from that thread). Sorry for that.

>
>>
>> This is also 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).
>>
>> To fix it, added return statements to xfs_log_force and xfs_fs_sync_fs if the
>> filesystem is already shutdown -- based on XFS_FORCED_SHUTDOWN.
>>
>> To simulate it, mount an xfs filesystem located on external disk, and then pull
>> the power to the disk.
>>
>> Tested it on latest linus tree.
>>
>> 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.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>   ---
>> ---
>>  fs/xfs/xfs_log.c   |    7 +++++++
>>  fs/xfs/xfs_super.c |    7 +++++++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
>> index 6db1fef..e4192b2 100644
>> --- a/fs/xfs/xfs_log.c
>> +++ b/fs/xfs/xfs_log.c
>> @@ -2932,6 +2932,13 @@ xfs_log_force(
>>  {
>>  	int	error;
>>
>> +	/*
>> +	 * No need to printk here since xfs_bwrite already printks about xfs
>> +	 * shutdown if it has shutdown already.
>> +	 */
>> +	if (XFS_FORCED_SHUTDOWN(mp))
>> +		return;
>> +
>>  	error = _xfs_log_force(mp, flags, NULL);
>>  	if (error)
>>  		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>
>I don't think this is the right place for the check. Depending on
>the type of error that caused the shutdown, the log may still be
>functioning and we need to be able to force the log so we don't lose
>all the changes in memory. The internal log implementation has all
>the checks necessary to avoid writing to the log if the log has
>already been shut down, which is why you keep seeing the error
>message.
I went through the function now and noticed a couple of EIOs 
returned, makes sense.
>
>What it appears to me is that you want to stop the error message
>from being emitted when the log is shut down. The way to do that is
>this:
>
> 	error = _xfs_log_force(mp, flags, NULL);
>-	if (error)
>+	if (error && !XFS_FORCED_SHUTDOWN(mp))
> 		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>
>Rather than avoining calling _xfs_log_force() altogether.
Yes, this should do.
>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index dab9a5f..b0f6041 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1010,6 +1010,13 @@ xfs_fs_sync_fs(
>>  	int			error;
>>
>>  	/*
>> +	 * No need to printk here since xfs_bwrite already printks about xfs
>> +	 * shutdown if it has shutdown already.
>> +	 */
>> +	if (XFS_FORCED_SHUTDOWN(mp))
>> +		return XFS_ERROR(EIO);
>
>
>I think if someone runs sync or calls syncfs() we should emit an
>error message indicating that it failed. We can't return an error to
>sync(), so we are limited to writing to the syslog.  This is not
>called periodically, so I don't see any problem with emitting an
>error message here. Being verbose in error states is desirable - you
>can never have too much information in the logs when something goes
>wrong....
Thanks, I look into this.

>
>Cheers,
>
>Dave.
>-- 
>Dave Chinner
>david@fromorbit.com
>
>_______________________________________________
>xfs mailing list
>xfs@oss.sgi.com
>http://oss.sgi.com/mailman/listinfo/xfs
>




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