Re: [PATCH v4] ext4: fix indirect punch hole corruption

[Omar Sandoval <osandov@osandov.com>]

On Tue, Feb 10, 2015 at 07:37:20PM -0800, Omar Sandoval wrote:
> On Tue, Feb 10, 2015 at 08:59:23PM -0600, Chris J Arges wrote:
> > On 02/10/2015 03:44 PM, Omar Sandoval wrote:
> > > Commit 4f579ae7de56 (ext4: fix punch hole on files with indirect
> > > mapping) rewrote FALLOC_FL_PUNCH_HOLE for ext4 files with indirect
> > > mapping. However, there are bugs in several corner cases. This fixes 5
> > > distinct bugs:
> > > 
> > > 1. When there is at least one entire level of indirection between the
> > > start and end of the punch range and the end of the punch range is the
> > > first block of its level, we can't return early; we have to free the
> > > intervening levels.
> > > 
> > > 2. When the end is at a higher level of indirection than the start and
> > > ext4_find_shared returns a top branch for the end, we still need to free
> > > the rest of the shared branch it returns; we can't decrement partial2.
> > > 
> > > 3. When a punch happens within one level of indirection, we need to
> > > converge on an indirect block that contains the start and end. However,
> > > because the branches returned from ext4_find_shared do not necessarily
> > > start at the same level (e.g., the partial2 chain will be shallower if
> > > the last block occurs at the beginning of an indirect group), the walk
> > > of the two chains can end up "missing" each other and freeing a bunch of
> > > extra blocks in the process. This mismatch can be handled by first
> > > making sure that the chains are at the same level, then walking them
> > > together until they converge.
> > > 
> > > 4. When the punch happens within one level of indirection and
> > > ext4_find_shared returns a top branch for the start, we must free it,
> > > but only if the end does not occur within that branch.
> > > 
> > > 5. When the punch happens within one level of indirection and
> > > ext4_find_shared returns a top branch for the end, then we shouldn't
> > > free the block referenced by the end of the returned chain (this mirrors
> > > the different levels case).
> > > 
> > > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > > ---
> > > Okay, two more bugfixes folded in, all described in the commit message.
> > > I'm finally no longer seeing xfstest generic/270 cause corruptions, even
> > > after running it overnight, so hopefully this is it. Chris, would you
> > > mind trying this out?
> > >
> > 
> > Omar,
> > I've completed 80 iterations of this patch so far without failure!
> > Normally failures have occurred between 2-15 runs. Great job, and thanks
> > for your persistence in fixing this issue!
> > 
> > Tested-by: Chris J Arges <chris.j.arges@canonical.com>
> > 
> 
> Awesome, I was starting to run out of ideas ;) Thanks for all of your
> testing.
> 
> Lukáš, would you like to take a look at this?
> 
> Also, Ted and Andreas, would you prefer this all in one patch, or should
> I split out each individual fix into its own patch?
> 
> Thanks!
> -- 
> Omar

Hi,

I figure things are busy because of the merge window, but I wanted to
check on the status of this patch. I also have some regression tests for
xfstests ready to go.

Thanks!
-- 
Omar