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

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

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

 
Hi,


* On Tue, May 08, 2012 at 09:53:21AM +1000, Dave Chinner <david@fromorbit.com> wrote:
>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.

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@fromorbit.com
>








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