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

Dave Chinner
2012-05-07T23:53:24Z

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.

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.

> /* 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....

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com