xfs
[Top] [All Lists]

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

To: raghu.prabhu13@xxxxxxxxx
Subject: Re: [PATCH] [PATCH] Stop periodic syncing if filesystem is already shutdown.
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 8 May 2012 09:53:21 +1000
Cc: xfs@xxxxxxxxxxx, Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
In-reply-to: <a27cbce4f4b35c2a8aee1b58d88c22381fe70ccf.1336378182.git.rprabhu@xxxxxxxxxxx>
References: <a27cbce4f4b35c2a8aee1b58d88c22381fe70ccf.1336378182.git.rprabhu@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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.

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@xxxxxxxxxxxxx

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