xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: Re: [PATCH] [PATCH] Stop periodic syncing if filesystem is already shutdown.
From: Raghavendra D Prabhu <raghu.prabhu13@xxxxxxxxx>
Date: Thu, 10 May 2012 02:04:44 +0530
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :x-operating-system:x-editor:user-agent; bh=Cu1mC5MlnXOPBuE9if+3Teb42WrJP6tDmVF9m7vkdpY=; b=XkrwQDTOOpzhHTlE6BXb3jPR9GWAA4Z/hrFMlXUo+Vtjb99C88R/JRr8pDkcNHlX7l C4TFdTGgtVxm2tOkUC9fBE3x4HyDZgvQnATaeYrewZS6W7oej22p1Mo4hsxaqscFnCuY VTg52rVVE3nAH7Pv0IUOzxoSFgjccwa7jUvj8Y2FLmVoxK/vjk0BO1011G2QFhY/6IDJ xRSwTC9Do7RMJ1tW4psFCpBIhfw5gFhscnW21Z2iSuGp6vS5/YUOwYtrMTVtL1/JvDyr g/HSSCLGd0lCIEaBb3navTzkPRZISF1pXk50HlT3zL+Y5UALbUCUYvMJslg+V0Oe1txG 8HjA==
In-reply-to: <20120509200746.GA29360@Xye>
Mail-followup-to: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
References: <a27cbce4f4b35c2a8aee1b58d88c22381fe70ccf.1336378182.git.rprabhu@xxxxxxxxxxx> <20120507235321.GD5091@dastard> <20120509200746.GA29360@Xye>
User-agent: Mutt/1.5.21 (2011-07-01)
Hi,

Resent with changes I had mentioned earlier.

================================================

[PATCH] 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 simulate it, mount an xfs filesystem located on external disk, and then pull
the power to the disk (non-usb powered disk).

Tested it on latest linus tree.
Now, the kernel log 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)

Signed-off-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
Tested-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
---
---
 fs/xfs/xfs_log.c |    7 +++++++
 1 file changed, 7 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);
--
1.7.10.1


* On Thu, May 10, 2012 at 01:37:46AM +0530, Raghavendra D Prabhu 
<raghu.prabhu13@xxxxxxxxx> wrote:

Hi,


* On Tue, May 08, 2012 at 09:53:21AM +1000, Dave Chinner <david@xxxxxxxxxxxxx> 
wrote:
On Mon, May 07, 2012 at 02:44:07PM +0530, raghu.prabhu13@xxxxxxxxx wrote:
From: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>

This is to prevent syncing from running ad-infinitum till umount if the disk 
has been forcefully unplugged.

This is to prevent messages like these from being displayed.
.....
---
fs/xfs/xfs_sync.c |   12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/fs/xfs/xfs_sync.c b/fs/xfs/xfs_sync.c
index 205ebcb..7ec412c 100644
--- a/fs/xfs/xfs_sync.c
+++ b/fs/xfs/xfs_sync.c
@@ -460,6 +460,12 @@ xfs_sync_worker(
                                        struct xfs_mount, m_sync_work);
        int             error;

+       if (!xfs_fs_writable(mp)) {
+               xfs_err(mp, "Filesystem not writable / already shutdown.");
+               xfs_syncd_stop(mp);
+               return;
+       }
+

That is going to kill the xfssyncd on read only and frozen
filesystems as well as shutdowns, so this is certainly not correct.
The xfs_sync_worker should continue to run until the filesystem is
unmounted, even if it does nothing when it runs.

Yes, in the next patch I had sent -- http://article.gmane.org/gmane.comp.file-systems.xfs.general/45568 -- I removed xfs_syncd_stop from there and added it under xfs_bwrite.

(PS: Adding xfs_syncd_stop in xfs_sync_worker was a very bad decision - it sent few of my kworkers to 'D' state and nearly corrupted that fs)



Indeed, all that is needed in xfs_sync_worker() is this:

-       if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
+       if (!xfs_fs_writable(mp)) {

and the error message won't appear. It fixes the problem for the
shutdown case, as well as handles frozen and read-only filesystems
correctly.
I realized from your earlier comment that, calling xfs_syncd_stop under xfs_bwrite won't be good either;
       so I was thinking of just checking for

XFS_FORCED_SHUTDOWN(mp) in xfs_log_force and bailing out if already shutdown.

which should take care of xfs_sync_worker, xfs_flush_worker, xfs_iflush and xfs_unmountfs


   Does this sound good?


                /* dgc: errors ignored here */
                if (mp->m_super->s_frozen == SB_UNFROZEN &&
@@ -551,6 +557,12 @@ xfs_flush_worker(
        struct xfs_mount *mp = container_of(work,
                                        struct xfs_mount, m_flush_work);

+       if (!xfs_fs_writable(mp)) {
+               xfs_err(mp, "Filesystem not writable / already shutdown.");
+               xfs_syncd_stop(mp);
+               return;
+       }
+
        xfs_sync_data(mp, SYNC_TRYLOCK);
        xfs_sync_data(mp, SYNC_TRYLOCK | SYNC_WAIT);

This is not necessary, either, because xfs_sync_data() has shutdown
checks and xfs_flush_worker() should never be called on a shutdown
filesystem....
Yeah, I realized that (sent a patch after this http://article.gmane.org/gmane.comp.file-systems.xfs.general/45568).

Cheers,

Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx









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






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

Attachment: pgpcnq2VEYMIn.pgp
Description: PGP signature

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