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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

Attachment: pgpHOmxWq7dFB.pgp
Description: PGP signature

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

[Index of Archives]     [Linux XFS Devel]     [Linux Filesystem Development]     [Filesystem Testing]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux