xfs
[Top] [All Lists]

Re: [PATCH 2/5] xfs: cleanup ->sync_fs

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: cleanup ->sync_fs
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Sun, 10 May 2009 12:51:22 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090426140707.713299000@xxxxxxxxxxxxxxxxxxxxxx>
References: <20090426140305.113371000@xxxxxxxxxxxxxxxxxxxxxx> <20090426140707.713299000@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (Macintosh/20090302)
Christoph Hellwig wrote:

> Sort out ->sync_fs to not perform a superblock writeback for the wait = 0 case
> as that is just an optional first pass and the superblock will be written back
> properly in the next call with wait = 1.  Instead perform an opportunistic
> quota writeback to have less work later.  Also remove the freeze special case
> as we do a proper wait = 1 call in the freeze code anyway.
> 
> Also rename the function to xfs_fs_sync_fs to match the normal naming
> convention, update comments and avoid calling into the laptop_mode logic on
> an error.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> 
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_super.c       2009-04-26 
> 10:39:20.433949442 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_super.c    2009-04-26 10:43:31.297949640 
> +0200
> @@ -1105,7 +1105,7 @@ xfs_fs_put_super(
>  }
>  
>  STATIC int
> -xfs_fs_sync_super(
> +xfs_fs_sync_fs(
>       struct super_block      *sb,
>       int                     wait)
>  {
> @@ -1113,23 +1113,23 @@ xfs_fs_sync_super(
>       int                     error;
>  
>       /*
> -      * Treat a sync operation like a freeze.  This is to work
> -      * around a race in sync_inodes() which works in two phases
> -      * - an asynchronous flush, which can write out an inode
> -      * without waiting for file size updates to complete, and a
> -      * synchronous flush, which wont do anything because the
> -      * async flush removed the inode's dirty flag.  Also
> -      * sync_inodes() will not see any files that just have
> -      * outstanding transactions to be flushed because we don't
> -      * dirty the Linux inode until after the transaction I/O
> -      * completes.
> +      * Not much we can do fort the first async pass.  Writing out the
                                 ^ typo
> +      * superblock would be contra-productive as we are going to redirty
                               ^ "counter-productive" is more common
> +      * when writing out other data and metadata (and writing out a single
> +      * block is quite fast anyway.
                                      ^ add a ")"
> +      *
> +      * Try to asynchronously kick of quota syncing at least.
                                      ^ "off?"
>        */
> -     if (wait || unlikely(sb->s_frozen == SB_FREEZE_WRITE))
> -             error = xfs_quiesce_data(mp);
> -     else
> -             error = xfs_sync_fsdata(mp, 0);
> +     if (!wait) {
> +             XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
> +             return 0;
> +     }
> +

Is it worth keeping a comment about this still being similar to the
freeze path?  (xfs_quiesce_data)

> +     error = xfs_quiesce_data(mp);
> +     if (error)
> +             return -error;
>  
> -     if (unlikely(laptop_mode)) {
> +     if (laptop_mode) {
>               int     prev_sync_seq = mp->m_sync_seq;
>  
>               /*
> @@ -1148,7 +1148,7 @@ xfs_fs_sync_super(
>                               mp->m_sync_seq != prev_sync_seq);
>       }
>  
> -     return -error;
> +     return 0;
>  }
>  
>  STATIC int
> @@ -1522,7 +1522,7 @@ static struct super_operations xfs_super
>       .write_inode            = xfs_fs_write_inode,
>       .clear_inode            = xfs_fs_clear_inode,
>       .put_super              = xfs_fs_put_super,
> -     .sync_fs                = xfs_fs_sync_super,
> +     .sync_fs                = xfs_fs_sync_fs,
>       .freeze_fs              = xfs_fs_freeze,
>       .statfs                 = xfs_fs_statfs,
>       .remount_fs             = xfs_fs_remount,
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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