Re: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets

From: Dave Chinner <david@fromorbit.com>
To: Alex Elder <aelder@sgi.com>
Cc: Christoph Hellwig <hch@infradead.org>, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets
Date: Thu, 14 Jan 2010 10:03:22 +1100	[thread overview]
Message-ID: <20100113230322.GS17483@discord.disaster> (raw)
In-Reply-To: <1AB9A794DBDDF54A8A81BE2296F7BDFE012A6965@cf--amer001e--3.americas.sgi.com>

On Wed, Jan 13, 2010 at 04:32:12PM -0600, Alex Elder wrote:
> Christoph Hellwig wrote:
> >> +	TP_printk("dev %d:%d %s inode 0x%llx, %s format, num_extents %d, "
> >> +		  "Max in-fork extents %d, broot size %d, fork offset %d",
> > 
> > It would be nice to keep the
> > 
> > 	"dev %d:%d ino 0x%llx"
> > 
> > prefix as a convention so that all trace records are similar at their
> > beginning.
> 
> Perhaps:
>     +	TP_printk("dev %d:%d inode 0x%llx (%s), %s format, num_extents %d, " 
>                                           ^
>                                           +--- symbolic entry->which
> 
> >>  #undef TRACE_INCLUDE_PATH
> >> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
> >> @@ -53,7 +53,7 @@ xfs_swapext(
> >>  	xfs_swapext_t	*sxp)
> >>  {
> >>  	xfs_inode_t     *ip, *tip;
> >> -	struct file	*file, *target_file;
> >> +	struct file	*file, *tmp_file;
> > 
> > I think these xfs_swapext belong into a separate patch.  While they
> > make the code quite a bit more redable they're purely cleanups and
> > can wait for 2.6.34.  And while you're at it you might also want to
> > merge xfs_swap_extents into xfs_swapext - there's no need for that
> > split at all.
> 
> I agree.  The change here is good (it was confusing and wrong before).

I think this is all a bit crazy - I had to make this change before I
could understand the code clearly enough to fix the bug. I had to go
all the way back to xfs_fsr to work out what it was providing the
kernel to determine WTF the kernel code was doing.

Clarifying the temp/target inodes is especially important because
the bug fix results in enforcing the target/temp inode ordering by
checking the temp inode has less extents than the target otherwise
it will reject the swap. If you can't tell which inode is which,
how can you tell the code is correct?

> But Dave can you please re-submit this with only the critical changes
> so I can get them to Linus in this release cycle?  I think the trace
> addition is probably fine, just get rid of the gratuitous variable
> name change in xfs_swapext() and put that in a separate patch.

The tracing is there so if someone reports a problem with the new
code we can easily determine if the correct action was taken by
swap extents or whether there's some condition I haven't handled.

I'll split it out into three patches - the bug fix, the rename
and the tracing code so you can pick and chose which ones you want
to take first...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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