[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: Re: [PATCH v4] Stop periodic syncing if filesystem is already shutdown.
From: Raghavendra D Prabhu <raghu.prabhu13@xxxxxxxxx>
Date: Tue, 5 Jun 2012 12:04:57 +0530
Cc: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :x-operating-system:x-editor:user-agent; bh=kSkF+Tofb5kXNVu3G0GT2Pie5FOPX5+yLgrKY7QM87A=; b=i3LAiP3C8rSFMnqSK/fr59wM+evsSvXvZ7WBaY9fBqy9+9HknT2d73NXhHxCCd7CZU heI1CCkR+DzRmHra8kL+lT0xfqASVIuwj9I/gDa1qDwSogEptYWg+PhH6p+oN7uiUwXR 1CkoXZoIE/DMv3M4cIMsEtue1ZrfN3Fb6GzexkIrC8dSUGaUCbijGcqMw/+H7MKvOM5D Tllr8Kf5T9qQds1qa3IiNf5QEV0rsSasQpwrLxA644f8zT6abbFiBHkZIob8OgbvtZd8 ShSLICKN1nMRGX+Zhbdsbe5Pr4Dk+bTBIOlV0v8x22ZtsI7dT8UNM2lKdq0QD+dVkTOg W0ig==
In-reply-to: <20120605060617.GG4347@dastard>
Mail-followup-to: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <d585aa328fc296e20bd3a180c2c3dbc4c77c45df.1338487073.git.rprabhu@xxxxxxxxxxx> <20120605060617.GG4347@dastard>
User-agent: Mutt/1.5.21 (2011-07-01)

* On Tue, Jun 05, 2012 at 04:06:17PM +1000, Dave Chinner <david@xxxxxxxxxxxxx> 
On Thu, May 31, 2012 at 11:37:34PM +0530, raghu.prabhu13@xxxxxxxxx wrote:
From: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>

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 


Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and 
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@xxxxxxxxxxx>
Tested-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
 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
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

        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
Thanks, I look into this.


Dave Chinner

xfs mailing list

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

Attachment: pgpYUzAQfsvHR.pgp
Description: PGP signature

<Prev in Thread] Current Thread [Next in Thread>