[PATCH 0/5] fix sync (test 182, grub)

[PATCH 0/5] fix sync (test 182, grub)

[Christoph Hellwig]

Revisit Dave's prototype to make sync equivalent to freeze, that is make
sure not only we have all data on disk, but also the metadata in the right
place and not requite a log recovery.  That fixes test 182 and should also
help with the frequent grub complaints.

The patches require Jan Kara's sync rewrite on -fsdevel and lkml
(http://lkml.indiana.edu/hypermail/linux/kernel/0904.2/03643.html)
so that sync actually calls into the filesystem in the correct order
for the various parts of sync activity.

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

[PATCH 1/5] xfs: remove ->write_super and stop maintaining ->s_dirt

[Christoph Hellwig]

[-- Attachment #1: xfs-stop-maintaining-s_dirt --]
[-- Type: text/plain, Size: 2578 bytes --]

the write_super method is used for

 (1) writing back the superblock periodically from pdflush
 (2) called just before ->sync_fs for data integerity syncs
 (3) just before ->put_super

We don't need (1) because we have our own peridoc writeout through xfssyncd,
we don't need (2) because xfs_fs_sync_fs performs a proper synchronous
superblock writeout after all other data and metadata has been written out,
and we don't need (3) because we synchronously write the superblock in
->put_super once the filesystem is fully shut down.

Also remove ->s_dirt tracking as it's only used to decide when too call
->write_super.


Signed-off-by: Christoph Hellwig <hch@lst.de>

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:33:36.051949562 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_super.c	2009-04-26 10:34:19.962075469 +0200
@@ -1104,15 +1104,6 @@ xfs_fs_put_super(
 	kfree(mp);
 }
 
-STATIC void
-xfs_fs_write_super(
-	struct super_block	*sb)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		xfs_sync_fsdata(XFS_M(sb), 0);
-	sb->s_dirt = 0;
-}
-
 STATIC int
 xfs_fs_sync_super(
 	struct super_block	*sb,
@@ -1137,7 +1128,6 @@ xfs_fs_sync_super(
 		error = xfs_quiesce_data(mp);
 	else
 		error = xfs_sync_fsdata(mp, 0);
-	sb->s_dirt = 0;
 
 	if (unlikely(laptop_mode)) {
 		int	prev_sync_seq = mp->m_sync_seq;
@@ -1443,7 +1433,6 @@ xfs_fs_fill_super(
 
 	XFS_SEND_MOUNT(mp, DM_RIGHT_NULL, mtpt, mp->m_fsname);
 
-	sb->s_dirt = 1;
 	sb->s_magic = XFS_SB_MAGIC;
 	sb->s_blocksize = mp->m_sb.sb_blocksize;
 	sb->s_blocksize_bits = ffs(sb->s_blocksize) - 1;
@@ -1533,7 +1522,6 @@ static struct super_operations xfs_super
 	.write_inode		= xfs_fs_write_inode,
 	.clear_inode		= xfs_fs_clear_inode,
 	.put_super		= xfs_fs_put_super,
-	.write_super		= xfs_fs_write_super,
 	.sync_fs		= xfs_fs_sync_super,
 	.freeze_fs		= xfs_fs_freeze,
 	.statfs			= xfs_fs_statfs,
Index: linux-2.6/fs/xfs/xfs_trans.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_trans.c	2009-04-26 10:33:25.534949105 +0200
+++ linux-2.6/fs/xfs/xfs_trans.c	2009-04-26 10:33:31.760951563 +0200
@@ -628,8 +628,6 @@ xfs_trans_apply_sb_deltas(
 		xfs_trans_log_buf(tp, bp, offsetof(xfs_dsb_t, sb_icount),
 				  offsetof(xfs_dsb_t, sb_frextents) +
 				  sizeof(sbp->sb_frextents) - 1);
-
-	tp->t_mountp->m_super->s_dirt = 1;
 }
 
 /*

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

Re: [PATCH 1/5] xfs: remove ->write_super and stop maintaining ->s_dirt

[Eric Sandeen]

Christoph Hellwig wrote:

> the write_super method is used for
>
>  (1) writing back the superblock periodically from pdflush
>  (2) called just before ->sync_fs for data integerity syncs
>  (3) just before ->put_super
>
> We don't need (1) because we have our own peridoc writeout through xfssyncd,
> we don't need (2) because xfs_fs_sync_fs performs a proper synchronous
> superblock writeout after all other data and metadata has been written out,
> and we don't need (3) because we synchronously write the superblock in
> ->put_super once the filesystem is fully shut down.
>
> Also remove ->s_dirt tracking as it's only used to decide when too call
> ->write_super.
>
Just to double check, what about sync_filesystems():

                if (sb->s_root && (wait || sb->s_dirt))
                        sb->s_op->sync_fs(sb, wait);

if we lose s_dirt does that mean we are potentially doing one less ->sync_fs 
here when called with wait = 0, and is that ok?  (/me waves hands about
sync; sync; sync magic) :)

-Eric

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

Re: [PATCH 1/5] xfs: remove ->write_super and stop maintaining ->s_dirt

[Eric Sandeen]

Eric Sandeen wrote:
> Christoph Hellwig wrote:
> 
>> the write_super method is used for
>>
>>  (1) writing back the superblock periodically from pdflush
>>  (2) called just before ->sync_fs for data integerity syncs
>>  (3) just before ->put_super
>>
>> We don't need (1) because we have our own peridoc writeout through xfssyncd,
>> we don't need (2) because xfs_fs_sync_fs performs a proper synchronous
>> superblock writeout after all other data and metadata has been written out,
>> and we don't need (3) because we synchronously write the superblock in
>> ->put_super once the filesystem is fully shut down.
>>
>> Also remove ->s_dirt tracking as it's only used to decide when too call
>> ->write_super.
>>
> Just to double check, what about sync_filesystems():
> 
>                 if (sb->s_root && (wait || sb->s_dirt))
>                         sb->s_op->sync_fs(sb, wait);
> 
> if we lose s_dirt does that mean we are potentially doing one less ->sync_fs 
> here when called with wait = 0, and is that ok?  (/me waves hands about
> sync; sync; sync magic) :)
> 
> -Eric

gah, never mind, I forgot that 0/5 talked about Jan's patches, and here:

http://lkml.indiana.edu/hypermail/linux/kernel/0904.2/03642.html

takes care of this concern.

-Eric

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

Re: [PATCH 1/5] xfs: remove ->write_super and stop maintaining ->s_dirt

[Eric Sandeen]

Christoph Hellwig wrote:

> the write_super method is used for
> 
>  (1) writing back the superblock periodically from pdflush
>  (2) called just before ->sync_fs for data integerity syncs
>  (3) just before ->put_super
> 
> We don't need (1) because we have our own peridoc writeout through xfssyncd,
> we don't need (2) because xfs_fs_sync_fs performs a proper synchronous
> superblock writeout after all other data and metadata has been written out,
> and we don't need (3) because we synchronously write the superblock in
> ->put_super once the filesystem is fully shut down.
> 
> Also remove ->s_dirt tracking as it's only used to decide when too call
> ->write_super.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

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

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

[Christoph Hellwig]

[-- Attachment #1: xfs-cleanup-sync_fs --]
[-- Type: text/plain, Size: 2867 bytes --]

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@lst.de>


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
+	 * superblock would be contra-productive as we are going to redirty
+	 * when writing out other data and metadata (and writing out a single
+	 * block is quite fast anyway.
+	 *
+	 * Try to asynchronously kick of quota syncing at least.
 	 */
-	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;
+	}
+
+	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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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@lst.de>
> 
> 
> 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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

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

[Christoph Hellwig]

On Sun, May 10, 2009 at 12:51:22PM -0500, Eric Sandeen wrote:
> Is it worth keeping a comment about this still being similar to the
> freeze path?  (xfs_quiesce_data)

Don't think so.  The commonality is in xfs_quiesce_data, and both
calling this functions for the majority of the work makes that quite
obvious.

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

[PATCH 3/5] xfs: make inodes dirty before issuing I/O

[Christoph Hellwig]

[-- Attachment #1: xfs-mark-inode-dirty-early --]
[-- Type: text/plain, Size: 2470 bytes --]

To make sure they get properly waited on in sync when I/O is in flight and
we latter need to update the inode size.



Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_aops.c	2009-04-26 10:33:05.556127371 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_aops.c	2009-04-26 10:37:23.137953826 +0200
@@ -186,19 +186,37 @@ xfs_destroy_ioend(
 }
 
 /*
+ * If the end of the current ioend is beyond the current EOF,
+ * return the new EOF value, otherwise zero.
+ */
+STATIC xfs_fsize_t
+xfs_ioend_new_eof(
+	xfs_ioend_t		*ioend)
+{
+	xfs_inode_t		*ip = XFS_I(ioend->io_inode);
+	xfs_fsize_t		isize;
+	xfs_fsize_t		bsize;
+
+	bsize = ioend->io_offset + ioend->io_size;
+	isize = MAX(ip->i_size, ip->i_new_size);
+	isize = MIN(isize, bsize);
+	return isize > ip->i_d.di_size ? isize : 0;
+}
+
+/*
  * Update on-disk file size now that data has been written to disk.
  * The current in-memory file size is i_size.  If a write is beyond
  * eof i_new_size will be the intended file size until i_size is
  * updated.  If this write does not extend all the way to the valid
  * file size then restrict this update to the end of the write.
  */
+
 STATIC void
 xfs_setfilesize(
 	xfs_ioend_t		*ioend)
 {
 	xfs_inode_t		*ip = XFS_I(ioend->io_inode);
 	xfs_fsize_t		isize;
-	xfs_fsize_t		bsize;
 
 	ASSERT((ip->i_d.di_mode & S_IFMT) == S_IFREG);
 	ASSERT(ioend->io_type != IOMAP_READ);
@@ -206,14 +224,9 @@ xfs_setfilesize(
 	if (unlikely(ioend->io_error))
 		return;
 
-	bsize = ioend->io_offset + ioend->io_size;
-
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-
-	isize = MAX(ip->i_size, ip->i_new_size);
-	isize = MIN(isize, bsize);
-
-	if (ip->i_d.di_size < isize) {
+	isize = xfs_ioend_new_eof(ioend);
+	if (isize) {
 		ip->i_d.di_size = isize;
 		ip->i_update_core = 1;
 		ip->i_update_size = 1;
@@ -405,10 +418,16 @@ xfs_submit_ioend_bio(
 	struct bio	*bio)
 {
 	atomic_inc(&ioend->io_remaining);
-
 	bio->bi_private = ioend;
 	bio->bi_end_io = xfs_end_bio;
 
+	/*
+	 * if the I/O is beyond EOF we mark the inode dirty immediately
+	 * but don't update the inode size until I/O completion.
+	 */
+	if (xfs_ioend_new_eof(ioend))
+		xfs_mark_inode_dirty_sync(XFS_I(ioend->io_inode));
+
 	submit_bio(WRITE, bio);
 	ASSERT(!bio_flagged(bio, BIO_EOPNOTSUPP));
 	bio_put(bio);

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

Re: [PATCH 3/5] xfs: make inodes dirty before issuing I/O

[Eric Sandeen]

Christoph Hellwig wrote:
> To make sure they get properly waited on in sync when I/O is in flight and
> we latter need to update the inode size.
> 

maybe mention the new helper in the changelog just for completeness...

> 
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_aops.c	2009-04-26 10:33:05.556127371 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_aops.c	2009-04-26 10:37:23.137953826 +0200
> @@ -186,19 +186,37 @@ xfs_destroy_ioend(
>  }
>  
>  /*
> + * If the end of the current ioend is beyond the current EOF,
> + * return the new EOF value, otherwise zero.
> + */
> +STATIC xfs_fsize_t
> +xfs_ioend_new_eof(
> +	xfs_ioend_t		*ioend)
> +{
> +	xfs_inode_t		*ip = XFS_I(ioend->io_inode);
> +	xfs_fsize_t		isize;
> +	xfs_fsize_t		bsize;
> +
> +	bsize = ioend->io_offset + ioend->io_size;
> +	isize = MAX(ip->i_size, ip->i_new_size);
> +	isize = MIN(isize, bsize);
> +	return isize > ip->i_d.di_size ? isize : 0;
> +}
> +
> +/*
>   * Update on-disk file size now that data has been written to disk.
>   * The current in-memory file size is i_size.  If a write is beyond
>   * eof i_new_size will be the intended file size until i_size is
>   * updated.  If this write does not extend all the way to the valid
>   * file size then restrict this update to the end of the write.
>   */
> +
>  STATIC void
>  xfs_setfilesize(
>  	xfs_ioend_t		*ioend)
>  {
>  	xfs_inode_t		*ip = XFS_I(ioend->io_inode);
>  	xfs_fsize_t		isize;
> -	xfs_fsize_t		bsize;
>  
>  	ASSERT((ip->i_d.di_mode & S_IFMT) == S_IFREG);
>  	ASSERT(ioend->io_type != IOMAP_READ);
> @@ -206,14 +224,9 @@ xfs_setfilesize(
>  	if (unlikely(ioend->io_error))
>  		return;
>  
> -	bsize = ioend->io_offset + ioend->io_size;
> -
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -
> -	isize = MAX(ip->i_size, ip->i_new_size);
> -	isize = MIN(isize, bsize);
> -
> -	if (ip->i_d.di_size < isize) {
> +	isize = xfs_ioend_new_eof(ioend);
> +	if (isize) {

It strikes me as a little odd to potentially get back "isize == 0" here
when nothing about the size is 0.  Would it make more sense to rename
this variable to "new_isize" or something?

>  		ip->i_d.di_size = isize;
>  		ip->i_update_core = 1;
>  		ip->i_update_size = 1;
> @@ -405,10 +418,16 @@ xfs_submit_ioend_bio(
>  	struct bio	*bio)
>  {
>  	atomic_inc(&ioend->io_remaining);
> -
>  	bio->bi_private = ioend;
>  	bio->bi_end_io = xfs_end_bio;
>  
> +	/*
> +	 * if the I/O is beyond EOF we mark the inode dirty immediately
           ^If (uber-nitpick, in akpm-mode today I guess!)

> +	 * but don't update the inode size until I/O completion.
> +	 */

Maybe extend this comment a bit to say -why- you are doing this, not
just -what- you are doing?

> +	if (xfs_ioend_new_eof(ioend))
> +		xfs_mark_inode_dirty_sync(XFS_I(ioend->io_inode));
> +
>  	submit_bio(WRITE, bio);
>  	ASSERT(!bio_flagged(bio, BIO_EOPNOTSUPP));
>  	bio_put(bio);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

[PATCH 4/5] xfs: make sure xfs_sync_fsdata covers the log

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-xfs_sync_fsdata --]
[-- Type: text/plain, Size: 2195 bytes --]

We want to always cover the log after writing out the superblock, and
in case of a synchronous writeout make sure we actually wait for the
log to be covered.  That way a filesystem that has been sync()ed can
be considered clean by log recovery.

Index: linux-2.6/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:08:01.468074092 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:28:37.049956907 +0200
@@ -208,11 +210,15 @@ xfs_sync_inodes(
 STATIC int
 xfs_commit_dummy_trans(
 	struct xfs_mount	*mp,
-	uint			log_flags)
+	uint			flags)
 {
 	struct xfs_inode	*ip = mp->m_rootip;
 	struct xfs_trans	*tp;
 	int			error;
+	int			log_flags = XFS_LOG_FORCE;
+
+	if (flags & SYNC_WAIT)
+		log_flags |= XFS_LOG_SYNC;
 
 	/*
 	 * Put a dummy transaction in the log to tell recovery
@@ -230,13 +236,12 @@ xfs_commit_dummy_trans(
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 	xfs_trans_ihold(tp, ip);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-	/* XXX(hch): ignoring the error here.. */
 	error = xfs_trans_commit(tp, 0);
-
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 
+	/* the log force ensures this transaction is pushed to disk */
 	xfs_log_force(mp, 0, log_flags);
-	return 0;
+	return error;
 }
 
 int
@@ -276,6 +281,7 @@ xfs_sync_fsdata(
 		 */
 		if (XFS_BUF_ISPINNED(bp))
 			xfs_log_force(mp, 0, XFS_LOG_FORCE);
+		xfs_flush_buftarg(mp->m_ddev_targp, 1);
 	}
 
 
@@ -284,7 +290,10 @@ xfs_sync_fsdata(
 	else
 		XFS_BUF_ASYNC(bp);
 
-	return xfs_bwrite(mp, bp);
+	error = xfs_bwrite(mp, bp);
+	if (!error && xfs_log_need_covered(mp))
+		error = xfs_commit_dummy_trans(mp, (flags & SYNC_WAIT));
+	return error;
 
  out_brelse:
 	xfs_buf_relse(bp);
@@ -469,8 +478,6 @@ xfs_sync_worker(
 		/* dgc: errors ignored here */
 		error = XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
 		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
-		if (xfs_log_need_covered(mp))
-			error = xfs_commit_dummy_trans(mp, XFS_LOG_FORCE);
 	}
 	mp->m_sync_seq++;
 	wake_up(&mp->m_wait_single_sync_task);

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

Re: [PATCH 4/5] xfs: make sure xfs_sync_fsdata covers the log

[Eric Sandeen]

Christoph Hellwig wrote:

> We want to always cover the log after writing out the superblock, and
> in case of a synchronous writeout make sure we actually wait for the
> log to be covered.  That way a filesystem that has been sync()ed can
> be considered clean by log recovery.

This one looks fine to me

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: linux-2.6/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:08:01.468074092 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:28:37.049956907 +0200
> @@ -208,11 +210,15 @@ xfs_sync_inodes(
>  STATIC int
>  xfs_commit_dummy_trans(
>  	struct xfs_mount	*mp,
> -	uint			log_flags)
> +	uint			flags)
>  {
>  	struct xfs_inode	*ip = mp->m_rootip;
>  	struct xfs_trans	*tp;
>  	int			error;
> +	int			log_flags = XFS_LOG_FORCE;
> +
> +	if (flags & SYNC_WAIT)
> +		log_flags |= XFS_LOG_SYNC;
>  
>  	/*
>  	 * Put a dummy transaction in the log to tell recovery
> @@ -230,13 +236,12 @@ xfs_commit_dummy_trans(
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ihold(tp, ip);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -	/* XXX(hch): ignoring the error here.. */
>  	error = xfs_trans_commit(tp, 0);
> -
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  
> +	/* the log force ensures this transaction is pushed to disk */
>  	xfs_log_force(mp, 0, log_flags);
> -	return 0;
> +	return error;
>  }
>  
>  int
> @@ -276,6 +281,7 @@ xfs_sync_fsdata(
>  		 */
>  		if (XFS_BUF_ISPINNED(bp))
>  			xfs_log_force(mp, 0, XFS_LOG_FORCE);
> +		xfs_flush_buftarg(mp->m_ddev_targp, 1);
>  	}
>  
>  
> @@ -284,7 +290,10 @@ xfs_sync_fsdata(
>  	else
>  		XFS_BUF_ASYNC(bp);
>  
> -	return xfs_bwrite(mp, bp);
> +	error = xfs_bwrite(mp, bp);
> +	if (!error && xfs_log_need_covered(mp))
> +		error = xfs_commit_dummy_trans(mp, (flags & SYNC_WAIT));
> +	return error;
>  
>   out_brelse:
>  	xfs_buf_relse(bp);
> @@ -469,8 +478,6 @@ xfs_sync_worker(
>  		/* dgc: errors ignored here */
>  		error = XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
>  		error = xfs_sync_fsdata(mp, SYNC_BDFLUSH);
> -		if (xfs_log_need_covered(mp))
> -			error = xfs_commit_dummy_trans(mp, XFS_LOG_FORCE);
>  	}
>  	mp->m_sync_seq++;
>  	wake_up(&mp->m_wait_single_sync_task);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

[PATCH 5/5] xfs: fix xfs_quiesce_data

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-xfs_quiesce_data --]
[-- Type: text/plain, Size: 1266 bytes --]

We need to do a synchronous xfs_sync_fsdata to make sure the superblock
actually is on disk when we return.  While we're at it also remove the
superflous SYNC_BDFLUSH flag to xfs_sync_inodes, and move the xfs_filestream_flush
call later [hch: why?  seems unrelated].


Index: linux-2.6/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:46:17.112949525 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:48:19.713979813 +0200
@@ -323,16 +323,16 @@ xfs_quiesce_data(
 	int error;
 
 	/* push non-blocking */
-	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_BDFLUSH);
+	xfs_sync_inodes(mp, SYNC_DELWRI);
 	XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
-	xfs_filestream_flush(mp);
 
-	/* push and block */
+	/* push and block till complete */
 	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_WAIT|SYNC_IOWAIT);
 	XFS_QM_DQSYNC(mp, SYNC_WAIT);
+	xfs_filestream_flush(mp);
 
 	/* write superblock and hoover up shutdown errors */
-	error = xfs_sync_fsdata(mp, 0);
+	error = xfs_sync_fsdata(mp, SYNC_WAIT);
 
 	/* flush data-only devices */
 	if (mp->m_rtdev_targp)

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

Re: [PATCH 5/5] xfs: fix xfs_quiesce_data

[Eric Sandeen]

Christoph Hellwig wrote:

> We need to do a synchronous xfs_sync_fsdata to make sure the superblock
> actually is on disk when we return.  While we're at it also remove the
> superflous SYNC_BDFLUSH flag to xfs_sync_inodes, and move the xfs_filestream_flush
> call later [hch: why?  seems unrelated].

What makes SYNC_BDFLUSH "superfluous?" ... oh ... because nothing in
that callchain below ever looks for it.  Could we make that more obvious
in the changelog?

I guess I too would like a reason for the filestream_flush move
somewhere in the changelog...

Adding the SYNC_WAIT looks good to me though :)

-Eric

> 
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:46:17.112949525 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:48:19.713979813 +0200
> @@ -323,16 +323,16 @@ xfs_quiesce_data(
>  	int error;
>  
>  	/* push non-blocking */
> -	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_BDFLUSH);
> +	xfs_sync_inodes(mp, SYNC_DELWRI);
>  	XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
> -	xfs_filestream_flush(mp);
>  
> -	/* push and block */
> +	/* push and block till complete */
>  	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_WAIT|SYNC_IOWAIT);
>  	XFS_QM_DQSYNC(mp, SYNC_WAIT);
> +	xfs_filestream_flush(mp);
>  
>  	/* write superblock and hoover up shutdown errors */
> -	error = xfs_sync_fsdata(mp, 0);
> +	error = xfs_sync_fsdata(mp, SYNC_WAIT);
>  
>  	/* flush data-only devices */
>  	if (mp->m_rtdev_targp)
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

Re: [PATCH 5/5] xfs: fix xfs_quiesce_data

[Christoph Hellwig]

On Sun, May 10, 2009 at 01:37:58PM -0500, Eric Sandeen wrote:
> Christoph Hellwig wrote:
> 
> > We need to do a synchronous xfs_sync_fsdata to make sure the superblock
> > actually is on disk when we return.  While we're at it also remove the
> > superflous SYNC_BDFLUSH flag to xfs_sync_inodes, and move the xfs_filestream_flush
> > call later [hch: why?  seems unrelated].
> 
> What makes SYNC_BDFLUSH "superfluous?" ... oh ... because nothing in
> that callchain below ever looks for it.  Could we make that more obvious
> in the changelog?

Ok.

> I guess I too would like a reason for the filestream_flush move
> somewhere in the changelog...

Hehe, maybe Dave can chime in.  Or I can test if it actually affects
anything and maybe move it out to another patch.  The lack of
reliability of the filesystreams tests doesn't make this any easier to
test.

> 
> Adding the SYNC_WAIT looks good to me though :)
> 
> -Eric
> 
> > 
> > Index: linux-2.6/fs/xfs/linux-2.6/xfs_sync.c
> > ===================================================================
> > --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:46:17.112949525 +0200
> > +++ linux-2.6/fs/xfs/linux-2.6/xfs_sync.c	2009-04-26 10:48:19.713979813 +0200
> > @@ -323,16 +323,16 @@ xfs_quiesce_data(
> >  	int error;
> >  
> >  	/* push non-blocking */
> > -	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_BDFLUSH);
> > +	xfs_sync_inodes(mp, SYNC_DELWRI);
> >  	XFS_QM_DQSYNC(mp, SYNC_BDFLUSH);
> > -	xfs_filestream_flush(mp);
> >  
> > -	/* push and block */
> > +	/* push and block till complete */
> >  	xfs_sync_inodes(mp, SYNC_DELWRI|SYNC_WAIT|SYNC_IOWAIT);
> >  	XFS_QM_DQSYNC(mp, SYNC_WAIT);
> > +	xfs_filestream_flush(mp);
> >  
> >  	/* write superblock and hoover up shutdown errors */
> > -	error = xfs_sync_fsdata(mp, 0);
> > +	error = xfs_sync_fsdata(mp, SYNC_WAIT);
> >  
> >  	/* flush data-only devices */
> >  	if (mp->m_rtdev_targp)
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

Re: [PATCH 5/5] xfs: fix xfs_quiesce_data

[Dave Chinner]

On Mon, May 11, 2009 at 04:15:11PM -0400, Christoph Hellwig wrote:
> On Sun, May 10, 2009 at 01:37:58PM -0500, Eric Sandeen wrote:
> > I guess I too would like a reason for the filestream_flush move
> > somewhere in the changelog...
> 
> Hehe, maybe Dave can chime in.  Or I can test if it actually affects
> anything and maybe move it out to another patch.  The lack of
> reliability of the filesystreams tests doesn't make this any easier to
> test.

(Just catching up on my mail backlog)

I think that the filestream_flush() call should actually be after
the data flush. filestream_flush() is used to clear the filestream
association cache which holds references to the inodes.

Where the flush is currently placed is destroying the association
that defines the AG the data should be written to before the data is
written. As a result it may not end up in the AG carefully
associated with the inode during the write() syscall.

This may be one of the reasons for the filestreams tests failing
frequently....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 5/5] xfs: fix xfs_quiesce_data

[Christoph Hellwig]

On Thu, Jun 04, 2009 at 07:45:12PM +1000, Dave Chinner wrote:
> On Mon, May 11, 2009 at 04:15:11PM -0400, Christoph Hellwig wrote:
> > On Sun, May 10, 2009 at 01:37:58PM -0500, Eric Sandeen wrote:
> > > I guess I too would like a reason for the filestream_flush move
> > > somewhere in the changelog...
> > 
> > Hehe, maybe Dave can chime in.  Or I can test if it actually affects
> > anything and maybe move it out to another patch.  The lack of
> > reliability of the filesystreams tests doesn't make this any easier to
> > test.
> 
> (Just catching up on my mail backlog)
> 
> I think that the filestream_flush() call should actually be after
> the data flush. filestream_flush() is used to clear the filestream
> association cache which holds references to the inodes.
> 
> Where the flush is currently placed is destroying the association
> that defines the AG the data should be written to before the data is
> written. As a result it may not end up in the AG carefully
> associated with the inode during the write() syscall.
> 
> This may be one of the reasons for the filestreams tests failing
> frequently....

Makes sense.  I stil get reliable failures on 171 and 172, but the
others seem to pass with these changes.

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

Re: [PATCH 0/5] fix sync (test 182, grub)

[Christoph Hellwig]

Can I motivate anyone for some reviews?

On Sun, Apr 26, 2009 at 10:03:05AM -0400, Christoph Hellwig wrote:
> Revisit Dave's prototype to make sync equivalent to freeze, that is make
> sure not only we have all data on disk, but also the metadata in the right
> place and not requite a log recovery.  That fixes test 182 and should also
> help with the frequent grub complaints.
> 
> The patches require Jan Kara's sync rewrite on -fsdevel and lkml
> (http://lkml.indiana.edu/hypermail/linux/kernel/0904.2/03643.html)
> so that sync actually calls into the filesystem in the correct order
> for the various parts of sync activity.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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