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

[Raghavendra D Prabhu <raghu.prabhu13@gmail.com>]

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

Hi,


* On Tue, Jun 05, 2012 at 04:06:17PM +1000, Dave Chinner <david@fromorbit.com> wrote:
>On Thu, May 31, 2012 at 11:37:34PM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> This is to prevent xfs_log_force from running ad-infinitum (due to xfs_sync)
>> till umount if the disk has been forcefully unplugged.
>
>I'm not sure where versions 2 and 3 went - I haven't seen them....

Actually I hadn't numbered them, but they are here http://oss.sgi.com/archives/xfs/2012-05/msg00065.html (starting from that thread). Sorry for that.

>
>>
>> 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 fix it, added return statements to xfs_log_force and xfs_fs_sync_fs if the
>> filesystem is already shutdown -- based on XFS_FORCED_SHUTDOWN.
>>
>> To simulate it, mount an xfs filesystem located on external disk, and then pull
>> the power to the disk.
>>
>> Tested it on latest linus tree.
>>
>> 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)
>>
>> ---
>>
>> 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.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> Tested-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>   ---
>> ---
>>  fs/xfs/xfs_log.c   |    7 +++++++
>>  fs/xfs/xfs_super.c |    7 +++++++
>>  2 files changed, 14 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);
>
>I don't think this is the right place for the check. Depending on
>the type of error that caused the shutdown, the log may still be
>functioning and we need to be able to force the log so we don't lose
>all the changes in memory. The internal log implementation has all
>the checks necessary to avoid writing to the log if the log has
>already been shut down, which is why you keep seeing the error
>message.
I went through the function now and noticed a couple of EIOs 
returned, makes sense.
>
>What it appears to me is that you want to stop the error message
>from being emitted when the log is shut down. The way to do that is
>this:
>
> 	error = _xfs_log_force(mp, flags, NULL);
>-	if (error)
>+	if (error && !XFS_FORCED_SHUTDOWN(mp))
> 		xfs_warn(mp, "%s: error %d returned.", __func__, error);
>
>Rather than avoining calling _xfs_log_force() altogether.
Yes, this should do.
>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index dab9a5f..b0f6041 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1010,6 +1010,13 @@ xfs_fs_sync_fs(
>>  	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 XFS_ERROR(EIO);
>
>
>I think if someone runs sync or calls syncfs() we should emit an
>error message indicating that it failed. We can't return an error to
>sync(), so we are limited to writing to the syslog.  This is not
>called periodically, so I don't see any problem with emitting an
>error message here. Being verbose in error states is desirable - you
>can never have too much information in the logs when something goes
>wrong....
Thanks, I look into this.

>
>Cheers,
>
>Dave.
>-- 
>Dave Chinner
>david@fromorbit.com
>
>_______________________________________________
>xfs mailing list
>xfs@oss.sgi.com
>http://oss.sgi.com/mailman/listinfo/xfs
>




Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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