linux-next: manual merge of the xfs tree with the vfs tree

linux-next: manual merge of the xfs tree with the vfs tree

[Stephen Rothwell]

Hi all,

Today's linux-next merge of the xfs tree got a conflict in
fs/xfs/linux-2.6/xfs_super.c between commits
4a295406e025bb7c8241ea956ec1b84830499e96 ("make sure data is on disk
before calling ->write_inode") and
716c28c0bc8bcbdd26e819f38dfc8fdfaafc0289 ("pass writeback_control to
->write_inode") from the vfs tree and commit
07fec73625dc0db6f9aed68019918208a2ca53f5 ("xfs: log changed inodes
instead of writing them synchronously") from the xfs tree.

I fixed it up (I think - see below) and can carry the fix as necessary.
What other file systems are doing for these conflicts is to merge in the
"write_inode" branch of Al Viro's vfs tree
(git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git) which Al
has said will not be rebased.  (Both those commits are in that branch.)
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc fs/xfs/linux-2.6/xfs_super.c
index 1e90797,25ea240..0000000
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@@ -1042,33 -1074,59 +1074,55 @@@ xfs_fs_write_inode
  	if (XFS_FORCED_SHUTDOWN(mp))
  		return XFS_ERROR(EIO);
  
- 	/*
- 	 * Bypass inodes which have already been cleaned by
- 	 * the inode flush clustering code inside xfs_iflush
- 	 */
- 	if (xfs_inode_clean(ip))
- 		goto out;
- 
- 	/*
- 	 * We make this non-blocking if the inode is contended, return
- 	 * EAGAIN to indicate to the caller that they did not succeed.
- 	 * This prevents the flush path from blocking on inodes inside
- 	 * another operation right now, they get caught later by xfs_sync.
- 	 */
 -	if (sync) {
 -		error = xfs_wait_on_pages(ip, 0, -1);
 -		if (error)
 -			goto out;
 -
 +	if (wbc->sync_mode == WB_SYNC_ALL) {
+ 		/*
+ 		 * Make sure the inode has hit stable storage.  By using the
+ 		 * log and the fsync transactions we reduce the IOs we have
+ 		 * to do here from two (log and inode) to just the log.
+ 		 *
+ 		 * Note: We still need to do a delwri write of the inode after
+ 		 * this to flush it to the backing buffer so that bulkstat
+ 		 * works properly if this is the first time the inode has been
+ 		 * written.  Because we hold the ilock atomically over the
+ 		 * transaction commit and the inode flush we are guaranteed
+ 		 * that the inode is not pinned when it returns. If the flush
+ 		 * lock is already held, then the inode has already been
+ 		 * flushed once and we don't need to flush it again.  Hence
+ 		 * the code will only flush the inode if it isn't already
+ 		 * being flushed.
+ 		 */
  		xfs_ilock(ip, XFS_ILOCK_SHARED);
- 		xfs_iflock(ip);
- 
- 		error = xfs_iflush(ip, XFS_IFLUSH_SYNC);
+ 		if (ip->i_update_core) {
+ 			error = xfs_log_inode(ip);
+ 			if (error)
+ 				goto out_unlock;
+ 		}
  	} else {
- 		error = EAGAIN;
+ 		/*
+ 		 * We make this non-blocking if the inode is contended, return
+ 		 * EAGAIN to indicate to the caller that they did not succeed.
+ 		 * This prevents the flush path from blocking on inodes inside
+ 		 * another operation right now, they get caught later by xfs_sync.
+ 		 */
  		if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
  			goto out;
- 		if (xfs_ipincount(ip) || !xfs_iflock_nowait(ip))
- 			goto out_unlock;
+ 	}
  
- 		error = xfs_iflush(ip, XFS_IFLUSH_ASYNC_NOBLOCK);
+ 	if (xfs_ipincount(ip) || !xfs_iflock_nowait(ip))
+ 		goto out_unlock;
+ 
+ 	/*
+ 	 * Now we have the flush lock and the inode is not pinned, we can check
+ 	 * if the inode is really clean as we know that there are no pending
+ 	 * transaction completions, it is not waiting on the delayed write
+ 	 * queue and there is no IO in progress.
+ 	 */
+ 	if (xfs_inode_clean(ip)) {
+ 		xfs_ifunlock(ip);
+ 		error = 0;
+ 		goto out_unlock;
  	}
+ 	error = xfs_iflush(ip, 0);
  
   out_unlock:
  	xfs_iunlock(ip, XFS_ILOCK_SHARED);

Re: linux-next: manual merge of the xfs tree with the vfs tree

[Al Viro]

On Mon, Feb 15, 2010 at 12:27:40PM +1100, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the xfs tree got a conflict in
> fs/xfs/linux-2.6/xfs_super.c between commits
> 4a295406e025bb7c8241ea956ec1b84830499e96 ("make sure data is on disk
> before calling ->write_inode") and
> 716c28c0bc8bcbdd26e819f38dfc8fdfaafc0289 ("pass writeback_control to
> ->write_inode") from the vfs tree and commit
> 07fec73625dc0db6f9aed68019918208a2ca53f5 ("xfs: log changed inodes
> instead of writing them synchronously") from the xfs tree.
> 
> I fixed it up (I think - see below) and can carry the fix as necessary.
> What other file systems are doing for these conflicts is to merge in the
> "write_inode" branch of Al Viro's vfs tree
> (git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git) which Al
> has said will not be rebased.  (Both those commits are in that branch.)

Actually, I'd cheerfully rebased that sucker (to e.g. write_inode2); it has
grown a trivial conflict with mainline after one of gfs2 merges and it's
annoying to fix it up after each for-next rebase.

So I'd rather put a rebased variant and switched the for-next to using that,
if people who'd pulled it already are OK with that.

Rebase v. merge (Was: Re: linux-next: manual merge of the xfs tree with the vfs tree)

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi Al,

On Mon, 15 Feb 2010 03:44:17 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
> Actually, I'd cheerfully rebased that sucker (to e.g. write_inode2); it has
> grown a trivial conflict with mainline after one of gfs2 merges and it's
> annoying to fix it up after each for-next rebase.
> 
> So I'd rather put a rebased variant and switched the for-next to using that,
> if people who'd pulled it already are OK with that.

Just out of interest, is there some reason you didn't just merge Linus'
tree (or the subset that caused the conflict) into the write-inode
branch.  That would have meant that you still had a nonrebasing branch
that others could use.  Now anyone who has merged your write_inode branch
needs to rebuild their trees using you new write-rebase2 branch or risk
causing conflicts in linux-next or Linus' tree when their tree's are
merged.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: Rebase v. merge (Was: Re: linux-next: manual merge of the xfs tree with the vfs tree)

[Al Viro]

On Tue, Feb 16, 2010 at 10:16:26AM +1100, Stephen Rothwell wrote:
> Hi Al,
> 
> On Mon, 15 Feb 2010 03:44:17 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >
> > Actually, I'd cheerfully rebased that sucker (to e.g. write_inode2); it has
> > grown a trivial conflict with mainline after one of gfs2 merges and it's
> > annoying to fix it up after each for-next rebase.
> > 
> > So I'd rather put a rebased variant and switched the for-next to using that,
> > if people who'd pulled it already are OK with that.
> 
> Just out of interest, is there some reason you didn't just merge Linus'
> tree (or the subset that caused the conflict) into the write-inode
> branch.  That would have meant that you still had a nonrebasing branch
> that others could use.  Now anyone who has merged your write_inode branch
> needs to rebuild their trees using you new write-rebase2 branch or risk
> causing conflicts in linux-next or Linus' tree when their tree's are
> merged.

Branch in question still doesn't exist; that was a question, not a description
of what I've already done.  I guess I can do what you describe, but...  Yuck.
Multiple merges from mainline can create one hell of a mess down the road.
I had to deal with results of exactly that when dwmw2 had dumped the audit
tree into my lap and it had been a huge mess that took quite a while to
untangle ;-/

The same goes for modifications hidden in merge commit, BTW.  I know that
Linus seems to be OK with that kind of thing, but... every time I run into
that is when some change is not to be found in git log -p ;-/

Oh, well...  I'll probably do that merge of mainline back into write_inode
and try hard to avoid anything similar in the next cycles.