xfs
[Top] [All Lists]

Re: [PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH v5] Stop xfs_do_force_shutdown / messages fron xfs_log_force if filesystem is already shutdown.
From: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Date: Tue, 11 Sep 2012 13:47:06 -0300
In-reply-to: <76577fa31ab2aa750994c1bc6a1b6f44beb03116.1347002369.git.rprabhu@xxxxxxxxxxx>
Mail-followup-to: xfs@xxxxxxxxxxx
References: <76577fa31ab2aa750994c1bc6a1b6f44beb03116.1347002369.git.rprabhu@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
Hi, particularly, these messages are annoying to me too, and it's great to see
this patch, have some comments though.

> This is to prevent xfs_log_force from printing error message continuously (due
> to xfs_sync and others) till umount if the disk has been forcefully unplugged.
> 
> This is 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). Also, xfs_do_force_shutdown is called
> repeatedly on an already shutdown filesystem through xfs_fs_sync_fs.
> 
> To fix this, added condition over XFS_FORCED_SHUTDOWN to xfs_log_force and
> xfs_fs_sync_fs.
> 
> To simulate it, mount an xfs filesystem located on external disk, and then 
> pull
> the power to the disk.
> 
> Now, the dmesg 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)
> 

To be honest this commit message looks a little confusing to me, I'd prefer
something a little more direct and clear, but this is just my humble opinion.

I'd say something like:

Prevents xfs_log_force from print continuously error messages after filesystem
has been shutdown:

[ 3873.009329] XFS (sdb3): xfs_log_force: error 5 returned.

> ---
> 
> Version 1: Removed calling xfs_syncd_stop from xfs_sync_worker.
> Version 2: Removed calling xfs_fs_writable in xfs_sync_worker and 
> xfs_flush_worker.
> Version 3: Removed calling xfs_syncd_stop in xfs_bwrite.
> Version 4: Added return statements to xfs_log_force and xfs_fs_sync_fs.
> Version 5: As per suggestion, added xfs_warn to xfs_fs_sync_fs and removed EIO
> return in xfs_log_force
> 
>  Signed-off-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
>  Tested-by: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>
> ---
>  fs/xfs/xfs_log.c   | 5 ++++-
>  fs/xfs/xfs_super.c | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 7f4f937..161c925 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3002,7 +3002,10 @@ xfs_log_force(
>  
>       trace_xfs_log_force(mp, 0);
>       error = _xfs_log_force(mp, flags, NULL);
> -     if (error)
> +     /*
> +      * Avoid warning when the filesystem has already shutdown.
> +      */
> +     if (error && !XFS_FORCED_SHUTDOWN(mp))
>               xfs_warn(mp, "%s: error %d returned.", __func__, error);
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index bdaf4cb..ebafc99 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -943,6 +943,11 @@ xfs_fs_sync_fs(
>       if (!wait)
>               return 0;
>  
> +     if (XFS_FORCED_SHUTDOWN(mp)) {
> +             xfs_warn(mp, "Sync called: filesystem already shutdown");
> +             return XFS_ERROR(EIO);
> +     }
> +

Looks like this will keep printing error messages every time a sync() happens,
either via flusher threads or manually by user once xfs_fs_sync_fs() will
still be called for it.

I would suggest to add some static variable here to check if the filesystem has
been already shutted down before print the message again, to also avoid message
flooding due xfs_fs_sync_fs() calls although I'm not sure another people will
agree with it.

Despit my comments, this looks good for me

Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>

-- 
--Carlos

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