Re: [PATCH 4/4] ext4: fix suboptimal seek_{data,hole} extents traversial

[Theodore Ts'o <tytso@mit.edu>]

On Wed, Dec 17, 2014 at 06:06:27PM +0300, Dmitry Monakhov wrote:
> I'm working on that. Sorry for delay. Delay is caused by confusion of
> existing seek_xxx implementation (even w/o my changes)
> For example: it is not obvious to believe that if bh is (bh_unwritten |
> bh_uptodate) then it contains valid data. IMHO it is just a fallocated
> area.  I already have simple fix for actual regression. But I try to
> review,optimize and retest all related logic.

Hi Dmitry,

Any update on how your testing is going?  I was wondering if I could
take a look at your fix just so I can understand what is going on.

> FYI: Currently test failed only on EXT4 w/o extents but w/ -odelalloc enabled.
> This configuration is absent in xfstest-bld where conf/ext3 is EXT4 w/o
> extents and w/ nodelalloc. That is why we oversee this from very beginning.

One of the reasons why I'm having trouble understanding what is
happening is that I can *only* reproduce the test if I use

./kvm-xfstests -c ext3 generic/285

If I run it by hand, I get this instead:

# xfstests/src/seek_sanity_test  /mnt/test
File system magic#: 0xef53
Allocation size: 4096
Kernel does not support llseek(2) extensions SEEK_HOLE and/or SEEK_DATA. Aborting.

... which is what I would expect, since this test should be refusing
to run in the first place if extents aren't available.  Also, if
extents aren't available, then we shouldn't be seeing *any* fallocated
space, so if the fault is in handling (bh_unwritten | bh_uptodate),
then this case shouldn't be happening at all on a file system w/o
extents, right?

One of the reasons why I am worrying is now that it's post -rc1, more
people will be testing out the kernel, and I'm concerned that I don't
understand what the risk exposure might be, especially since there are
programs like /bin/cp that assume SEEK_DATA and SEEK_HOLE work
correctly, and early testers might suffer data loss if they aren't
working correctly.

So I'm trying to determine whether this is something where we
understand what is going on so we can fix the bug, or whether I'm
better off reverting the commit and waiting until we have fully
characterized the bug.

Thanks,

					- Ted