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

Raghavendra D Prabhu
2012-05-09T20:34:50Z

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

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@wnohang.net>
Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
---
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@gmail.com> wrote:
>
>Hi,
>
>
>* On Tue, May 08, 2012 at 09:53:21AM +1000, Dave Chinner <david@fromorbit.com> wrote:
>>On Mon, May 07, 2012 at 02:44:07PM +0530, raghu.prabhu13@gmail.com wrote:
>>>From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>>
>>>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@fromorbit.com
>>
>
>
>
>
>
>
>
>
>Regards,
>--
>Raghavendra Prabhu
>GPG Id : 0xD72BE977
>Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
>www: wnohang.net