possible dev branch regression - xfstest 285/1k

From previous discussions, we expect 285 to fail in the ext3 (nodelalloc,

I'm seeing Xfstest 285 consistently fail for the 1k test case using the latest dev branch while running on both x86 and ARM. Subtest 08 is the problem. From the test output:

08. Test file with unwritten extents, only have unwritten pages

08.01 SEEK_HOLE expected 0 or 4194304, got 11264.                 FAIL
08.02 SEEK_HOLE expected 1 or 4194304, got 11264.                 FAIL
08.03 SEEK_DATA expected 10240 or 10240, got 0.                   FAIL
08.04 SEEK_DATA expected 10240 or 10240, got 1.                   FAIL

From previous discussions, we expect 285 to fail in the ext3 (nodelalloc, no flex_bg, and no extents) test case, but in subtest 07. It still does that.

In the dev branch, reverting 4f42f80a8f - "ext4: use s_extent_max_zeroout_kb value as number of kb" - results in success for 285 in the 1k test case.

I'm seeing Xfstest 285 consistently fail for the 1k test case using the latest dev branch while running on both x86 and ARM. Subtest 08 is the problem. From the test output:

08. Test file with unwritten extents, only have unwritten pages

08.01 SEEK_HOLE expected 0 or 4194304, got 11264.                 FAIL
08.02 SEEK_HOLE expected 1 or 4194304, got 11264.                 FAIL
08.03 SEEK_DATA expected 10240 or 10240, got 0.                   FAIL
08.04 SEEK_DATA expected 10240 or 10240, got 1.                   FAIL

From previous discussions, we expect 285 to fail in the ext3 (nodelalloc, no flex_bg, and no extents) test case, but in subtest 07. It still does that.

In the dev branch, reverting 4f42f80a8f - "ext4: use s_extent_max_zeroout_kb value as number of kb" - results in success for 285 in the 1k test case.

I see what's going on. First of all it isn't a bug. :-) Please let me describe why it happens.

In this commit (4f42f80a8f), it tries to fix a bug that we never zero out an unwritten extent. So after applied it, when an unwritten extent is converted, it could be zeroed out. In xfstests #285 subtest 08 it preallocates an unwritten extent which is 4MB. Then it writes some data at offset 10 * blocksize, which the length is one blocksize, and calles sync_file_range(2) to flush it.

Quoted 10 lines · Zheng Liu

I see what's going on. First of all it isn't a bug. :-) Please let me describe why it happens.

In this commit (4f42f80a8f), it tries to fix a bug that we never zero out an unwritten extent. So after applied it, when an unwritten extent is converted, it could be zeroed out. In xfstests #285 subtest 08 it preallocates an unwritten extent which is 4MB. Then it writes some data at offset 10 * blocksize, which the length is one blocksize, and calles sync_file_range(2) to flush it.

Specifically, we are now honoring the default setting which sets the max_zeroout_kb value to be 32. With a 4k block file system, if we were to zeroout the extent, we would have to zero out 40k, which is greater than 32k, so resulting file after pwrite(fd, 4096, 40960) looks like this:

% filefrag -v /u1/foo08
Filesystem type is: ef53
File size of /u1/foo08 is 4194304 (1024 blocks of 4096 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

   0:        0..       9:    1852416..   1852425:     10:             unwritten
   1:       10..      10:    1852426..   1852426:      1:            
   2:       11..    1023:    1852427..   1853439:   1013:             unwritten,eof

/u1/foo08: 1 extent found

With a 1k block file system, we only need to zero out 10k, which is less than 32k, and so after pwrite(fd, 1024, 10240), the file looks like this:

% filefrag -v /mnt/foo08
Filesystem type is: ef53
File size of /mnt/foo08 is 4194304 (4096 blocks of 1024 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

   0:        0..      10:      81921..     81931:     11:            
   1:       11..    4095:      81932..     86016:   4085:             unwritten,eof

/mnt/foo08: 1 extent found

If we run src/seek_sanity_test by hand, we can make it happy by setting the following configuration option before we run it:

echo 0 > /sys/fs/ext4/<dev>/extent_max_zeroout_kb

I'm not sure what's the best way to make xfstest #285 happy, though.

One way might be to change the test so that instead of writing the data at offset bufsize*10, we change it so it writes the data at offset bufsize*40, and change the expected values accordingly. The other would be to add some kind of ext4-specific hack to test #285 which manually sets the extent_max_zeroout_kb tuning parameter after the file system is mounted.

I'm not sure which is more likely to be accepted by the xfstests maintainers. I suspect the former, but they may not like either solution, in which case we might have to disable 285 for ext4 and create an ext4-specific test.

Quoted 10 lines · Zheng Liu

I see what's going on. First of all it isn't a bug. :-) Please let me describe why it happens.

In this commit (4f42f80a8f), it tries to fix a bug that we never zero out an unwritten extent. So after applied it, when an unwritten extent is converted, it could be zeroed out. In xfstests #285 subtest 08 it preallocates an unwritten extent which is 4MB. Then it writes some data at offset 10 * blocksize, which the length is one blocksize, and calles sync_file_range(2) to flush it.

Specifically, we are now honoring the default setting which sets the max_zeroout_kb value to be 32. With a 4k block file system, if we were to zeroout the extent, we would have to zero out 40k, which is greater than 32k, so resulting file after pwrite(fd, 4096, 40960) looks like this:

% filefrag -v /u1/foo08
Filesystem type is: ef53
File size of /u1/foo08 is 4194304 (1024 blocks of 4096 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

   0:        0..       9:    1852416..   1852425:     10:             unwritten
   1:       10..      10:    1852426..   1852426:      1:            
   2:       11..    1023:    1852427..   1853439:   1013:             unwritten,eof

/u1/foo08: 1 extent found

With a 1k block file system, we only need to zero out 10k, which is less than 32k, and so after pwrite(fd, 1024, 10240), the file looks like this:

% filefrag -v /mnt/foo08
Filesystem type is: ef53
File size of /mnt/foo08 is 4194304 (4096 blocks of 1024 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

   0:        0..      10:      81921..     81931:     11:            
   1:       11..    4095:      81932..     86016:   4085:             unwritten,eof

/mnt/foo08: 1 extent found

If we run src/seek_sanity_test by hand, we can make it happy by setting the following configuration option before we run it:

echo 0 > /sys/fs/ext4/<dev>/extent_max_zeroout_kb

I'm not sure what's the best way to make xfstest #285 happy, though.

One way might be to change the test so that instead of writing the data at offset bufsize*10, we change it so it writes the data at offset bufsize*40, and change the expected values accordingly. The other would be to add some kind of ext4-specific hack to test #285 which manually sets the extent_max_zeroout_kb tuning parameter after the file system is mounted.

I'm not sure which is more likely to be accepted by the xfstests maintainers. I suspect the former, but they may not like either solution, in which case we might have to disable 285 for ext4 and create an ext4-specific test.

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 31 lines · Theodore Ts'o

Quoted 10 lines · Zheng Liu

I see what's going on. First of all it isn't a bug. :-) Please let me describe why it happens.

In this commit (4f42f80a8f), it tries to fix a bug that we never zero out an unwritten extent. So after applied it, when an unwritten extent is converted, it could be zeroed out. In xfstests #285 subtest 08 it preallocates an unwritten extent which is 4MB. Then it writes some data at offset 10 * blocksize, which the length is one blocksize, and calles sync_file_range(2) to flush it.

Specifically, we are now honoring the default setting which sets the max_zeroout_kb value to be 32. With a 4k block file system, if we were to zeroout the extent, we would have to zero out 40k, which is greater than 32k, so resulting file after pwrite(fd, 4096, 40960) looks like this:

% filefrag -v /u1/foo08
Filesystem type is: ef53
File size of /u1/foo08 is 4194304 (1024 blocks of 4096 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

  0:        0..       9:    1852416..   1852425:     10:             unwritten
  1:       10..      10:    1852426..   1852426:      1:            
  2:       11..    1023:    1852427..   1853439:   1013:             unwritten,eof

/u1/foo08: 1 extent found

With a 1k block file system, we only need to zero out 10k, which is less than 32k, and so after pwrite(fd, 1024, 10240), the file looks like this:

Quoted 7 lines

% filefrag -v /mnt/foo08
Filesystem type is: ef53
File size of /mnt/foo08 is 4194304 (4096 blocks of 1024 bytes)
ext: logical_offset: physical_offset: length: expected: flags:

  0:        0..      10:      81921..     81931:     11:            
  1:       11..    4095:      81932..     86016:   4085:             unwritten,eof

/mnt/foo08: 1 extent found

So the issue is just that the test is looking for actual holes in specific locations , but the fs chose to allocate zero-filled blocks instead?

Quoted 5 lines

If we run src/seek_sanity_test by hand, we can make it happy by setting the following configuration option before we run it:

echo 0 > /sys/fs/ext4/<dev>/extent_max_zeroout_kb

The test could do this too, right?

_need_to_be_root

and:

if [ "$FSTYP" == "ext4" ]; then
ORIG_ZEROOUT_KB=cat /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb echo 0 > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb
fi

and put it back to default in _cleanup:

echo $ORIG_ZEROOUT_KB > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb

That way we'd be testing seek hole correctness w/o being subject to the vagaries in allocator behavior.

The test could do this too, right?

_need_to_be_root

and:

if [ "$FSTYP" == "ext4" ]; then
ORIG_ZEROOUT_KB=cat /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb echo 0 > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb
fi

and put it back to default in _cleanup:

echo $ORIG_ZEROOUT_KB > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb

That way we'd be testing seek hole correctness w/o being subject to the vagaries in allocator behavior.

On Mon, Mar 18, 2013 at 11:10:51AM -0500, Eric Sandeen wrote:

Quoted 15 lines

The test could do this too, right?

_need_to_be_root

and:

if [ "$FSTYP" == "ext4" ]; then
ORIG_ZEROOUT_KB=cat /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb echo 0 > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb
fi

and put it back to default in _cleanup:

echo $ORIG_ZEROOUT_KB > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb

Quoted 2 lines

That way we'd be testing seek hole correctness w/o being subject to the vagaries in allocator behavior.

Yeah, the question is whether it would be more acceptable to put ext4-specific hacks like this into the test, or to modify src/seek_sanity_test.c so that it writes the test block-size block using pwrite at offset blocksize*42 instead of offset blocksize*10.

I had assumed putting hacks which tweaked sysfs tunables into the xfstest script itself would be frowned upon, but if that's considered OK, that would be great.

- Ted

Quoted 1 line · Theodore Ts'o

On Mon, Mar 18, 2013 at 11:10:51AM -0500, Eric Sandeen wrote:

<previous discussion thread about test 285 SEEK_HOLE test breaking on ext4 due to change in opportunistic hole-filling behavior and how to make it work again on ext4, and mention of sysctl which makes it pass>

Quoted 15 lines

Quoted 15 lines

The test could do this too, right?

_need_to_be_root

and:

if [ "$FSTYP" == "ext4" ]; then
ORIG_ZEROOUT_KB=cat /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb echo 0 > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb
fi

and put it back to default in _cleanup:

echo $ORIG_ZEROOUT_KB > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb

Quoted 7 lines

Quoted 2 lines

That way we'd be testing seek hole correctness w/o being subject to the vagaries in allocator behavior.

Yeah, the question is whether it would be more acceptable to put ext4-specific hacks like this into the test, or to modify src/seek_sanity_test.c so that it writes the test block-size block using pwrite at offset blocksize*42 instead of offset blocksize*10.

That seems like more of an obtuse hack, since it depends on current default behavior, right?

Explicitly setting the zeroout to 0, with a comment as to why, should make it clear to the reader of the test I think.

I'll have to look, xfs speculative preallocation fills in holes in some cases as well, I'm not certain how it behaves on this test.

But we could put in a specific tuning for xfs as well if needed.

If it becomes clear that every fs requires tuning to not opportunistically fill in holes, then maybe we should make it non-generic, and only support filesystems we've tested or tuned to work with the testcase.

Quoted 3 lines

I had assumed putting hacks which tweaked sysfs tunables into the xfstest script itself would be frowned upon, but if that's considered OK, that would be great.

I don't see any real problem with it, myself.

cc xfs list to see if there are any objections...

Hi Eric,

Quoted 34 lines · Eric Sandeen

Quoted 1 line · Theodore Ts'o

On Mon, Mar 18, 2013 at 11:10:51AM -0500, Eric Sandeen wrote:

<previous discussion thread about test 285 SEEK_HOLE test breaking on ext4 due to change in opportunistic hole-filling behavior and how to make it work again on ext4, and mention of sysctl which makes it pass>

Quoted 15 lines

Quoted 15 lines

The test could do this too, right?

_need_to_be_root

and:

if [ "$FSTYP" == "ext4" ]; then
ORIG_ZEROOUT_KB=cat /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb echo 0 > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb
fi

and put it back to default in _cleanup:

echo $ORIG_ZEROOUT_KB > /sys/fs/ext4/$TEST_DEV/extent_max_zeroout_kb

Quoted 7 lines

Quoted 2 lines

That way we'd be testing seek hole correctness w/o being subject to the vagaries in allocator behavior.

Yeah, the question is whether it would be more acceptable to put ext4-specific hacks like this into the test, or to modify src/seek_sanity_test.c so that it writes the test block-size block using pwrite at offset blocksize*42 instead of offset blocksize*10.

That seems like more of an obtuse hack, since it depends on current default behavior, right?

Quoted 5 lines

Explicitly setting the zeroout to 0, with a comment as to why, should make it clear to the reader of the test I think.

I'll have to look, xfs speculative preallocation fills in holes in some cases as well, I'm not certain how it behaves on this test.

My impression was that we are not zeroing holes, but I'd also have to look to be sure. ;)

Quoted 10 lines

But we could put in a specific tuning for xfs as well if needed.

If it becomes clear that every fs requires tuning to not opportunistically fill in holes, then maybe we should make it non-generic, and only support filesystems we've tested or tuned to work with the testcase.

Quoted 3 lines

I had assumed putting hacks which tweaked sysfs tunables into the xfstest script itself would be frowned upon, but if that's considered OK, that would be great.

Quoted 3 lines

I don't see any real problem with it, myself.

cc xfs list to see if there are any objections...

Seems like the options being discussed so far are:

1) make the test fs specific

2) filesystem specific hacks to disable opportunistic zeroing of holes

3) modify the test output to work with current ext4 default behavior

It might be hard to find a tuning to produce identical output for xfs and ext4 (option 3), and option 1 and 2 are a also bit clunky.

How about option 4) fs-specific test output?

We wouldn't have multiple copies of the same test laying around, and ext4 could still run with default settings.

I know that Ted has already asked "what is an extent", but that's also missing the point. An extent is defined, just like for on-disk extent records, as a region of a file that is both logically and physically contiguous. From that, a fragmented file is a file that is logically contiguous but physically disjointed, and a sparse file is one that is logically disjointed. i.e. it is the relationship between extents that defines "sparse" and "fragmented", not the definition of an extent itself.

Looking at the test itself, then. The backwards synchronous write trick that is used by 218? That's an underhanded trick to make XFS create a fragmented file. We are not testing that the defragmenter knows that it's a backwards written file - we are testing that it sees the file as logically contiguous and physically disjointed, and then defragments it successfully.

Quoted 8 lines · Dave Chinner

I know that Ted has already asked "what is an extent", but that's also missing the point. An extent is defined, just like for on-disk extent records, as a region of a file that is both logically and physically contiguous. From that, a fragmented file is a file that is logically contiguous but physically disjointed, and a sparse file is one that is logically disjointed. i.e. it is the relationship between extents that defines "sparse" and "fragmented", not the definition of an extent itself.

Dave --- I think we're talking about two different tests. This particular test is xfstest #285.

The test in question is subtest #8, which preallocates a 4MB file, and then writes a block filled with 'a' which is sized to the file system block size, at offset 10*fs_block_size. It then checks to make sure SEEK_HOLE and SEEK_DATA is what it expects.

Quoted 6 lines

Looking at the test itself, then. The backwards synchronous write trick that is used by 218? That's an underhanded trick to make XFS create a fragmented file. We are not testing that the defragmenter knows that it's a backwards written file - we are testing that it sees the file as logically contiguous and physically disjointed, and then defragments it successfully.

What I was saying --- in the other mail thread --- is that it's open to question whether a file which is being written via a random-write pattern, resulting in a physically contiguous, but not contiguous from a logical block number point of view, is worth defragging or not. It all depends on whether the file is likely to be read sequentially in the future, or whether it will continue to be accessed via a random access pattern. In the latter case, it might not be worth defragging the file.

In fact, I tend to agree with the argument we might as well attempt to make the file logically contiguous so that it's efficient to read the file sequentially. But the people at Fujitsu who wrote the algorithms in e2defrag had gone out of their way to detect this case and avoid defragging the file so long as the physical blocks in use were contiguous --- and I believe that's also a valid design decision.

Depending on how we resolve this particular design question, we can then decide whether we need to make test #218 fs specific or not. There was no thought design choics made by ext4 should drive changes in how the defragger works in xfs or btrfs, or vice versa.

So I was looking for discussion by the ext4 developers; I was not requesting any changes from the XFS developers with respect to test #218. (Not yet; and perhaps not ever.)

Quoted 6 lines · Ben Myers

Hi Eric,

Quoted 2 lines · Eric Sandeen

Quoted 1 line · Theodore Ts'o

On Mon, Mar 18, 2013 at 11:10:51AM -0500, Eric Sandeen wrote:

still run with default settings.

And when the default settings change, or some other bug fix comes along?

So, let's step back a moment and ask ourselves what the test is actaully trying to test. zero-out is not what it is trying to test, nor is it trying to test specific file layouts. This is a basic *defragmenter* sanity test. SO, we're testing 2 things:

Sorry about this - I've mixed up my threads about ext4 having problems with zero-out being re-enabled. I thought this was a cross-post of the 218 issue....

However, the same reasoning can be applied to 285 - the file sizes, the size of the holes and the size of the data is all completely arbitrary. If we make the holes in the files larger, then the zero-out problem simply goes away.

Quoted 8 lines · Dave Chinner

Sorry about this - I've mixed up my threads about ext4 having problems with zero-out being re-enabled. I thought this was a cross-post of the 218 issue....

However, the same reasoning can be applied to 285 - the file sizes, the size of the holes and the size of the data is all completely arbitrary. If we make the holes in the files larger, then the zero-out problem simply goes away.

Right. That was my observation. We can either make the holes larger, by changing:

   pwrite(fd, buf, bufsize, bufsize*10);

to

   pwrite(fd, buf, bufsize, bufsize*42);

... and then changing the expected values returned by SEEK_HOLE/SEEK_DATA. (By the way; this only matters when we are testing 1k blocks; if we are using a 4k block size in ext4, the test currently passes.)

Or we could set some ext4-specific tuning parameters into the #218 shell script, if the file system in question was ext4.

I had assumed that folks would prefer making the holes larger, but Eric seemed to prefer the second choice as a better one.

Hmm.... Another possibility is to define a directory structure where each test would look for the existence of some file such as fscust/<fs>/<test>, and so if fscust/ext4/218 exists, it would get sourced, and this would define potential hook functions that would get called after the file system is mounted. This way, the file system specific stuff is kept out of the way of the test script. Would that make adding fs-specific tuning/setup for tests more palatable?

From an architectural POV, I think that if we need filesystem

Quoted 8 lines · Dave Chinner

Sorry about this - I've mixed up my threads about ext4 having problems with zero-out being re-enabled. I thought this was a cross-post of the 218 issue....

However, the same reasoning can be applied to 285 - the file sizes, the size of the holes and the size of the data is all completely arbitrary. If we make the holes in the files larger, then the zero-out problem simply goes away.

Right. That was my observation. We can either make the holes larger, by changing:

   pwrite(fd, buf, bufsize, bufsize*10);

to

   pwrite(fd, buf, bufsize, bufsize*42);

... and then changing the expected values returned by SEEK_HOLE/SEEK_DATA. (By the way; this only matters when we are testing 1k blocks; if we are using a 4k block size in ext4, the test currently passes.)

Or we could set some ext4-specific tuning parameters into the #218

shell script, if the file system in question was ext4.

I had assumed that folks would prefer making the holes larger, but Eric seemed to prefer the second choice as a better one.

Date: Mon, 18 Mar 2013 21:28:22 -0500
From: Eric Sandeen <[email protected]>
To: Theodore Ts'o <[email protected]>
Cc: Dave Chinner <[email protected]>, Eric Whitney <[email protected]>,
Eric Sandeen <[email protected]>, Ben Myers <[email protected]>, [email protected], xfs-oss <[email protected]>
Subject: Re: possible dev branch regression - xfstest 285/1k

Quoted 17 lines · Theodore Ts'o

Quoted 8 lines · Dave Chinner

Sorry about this - I've mixed up my threads about ext4 having problems with zero-out being re-enabled. I thought this was a cross-post of the 218 issue....

However, the same reasoning can be applied to 285 - the file sizes, the size of the holes and the size of the data is all completely arbitrary. If we make the holes in the files larger, then the zero-out problem simply goes away.

Right. That was my observation. We can either make the holes larger, by changing:

   pwrite(fd, buf, bufsize, bufsize*10);

to

Quoted 8 lines

   pwrite(fd, buf, bufsize, bufsize*42);

... and then changing the expected values returned by SEEK_HOLE/SEEK_DATA. (By the way; this only matters when we are testing 1k blocks; if we are using a 4k block size in ext4, the test currently passes.)

Or we could set some ext4-specific tuning parameters into the #218

285! :)

Quoted 2 lines

shell script, if the file system in question was ext4.

Quoted 2 lines

I had assumed that folks would prefer making the holes larger, but Eric seemed to prefer the second choice as a better one.

Ok, after the discussion I'm convinced too. Stretching out the allocation to avoid fill-in probably makes sense. But maybe not "42" - how about something much larger, so that any "reasonable" filesystem wouldn't even consider zeroing the range in between?

-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Date: Mon, 18 Mar 2013 21:28:22 -0500
From: Eric Sandeen <[email protected]>
To: Theodore Ts'o <[email protected]>
Cc: Dave Chinner <[email protected]>, Eric Whitney <[email protected]>,
Eric Sandeen <[email protected]>, Ben Myers <[email protected]>, [email protected], xfs-oss <[email protected]>
Subject: Re: possible dev branch regression - xfstest 285/1k

Quoted 17 lines · Theodore Ts'o

Quoted 8 lines · Dave Chinner

Sorry about this - I've mixed up my threads about ext4 having problems with zero-out being re-enabled. I thought this was a cross-post of the 218 issue....

However, the same reasoning can be applied to 285 - the file sizes, the size of the holes and the size of the data is all completely arbitrary. If we make the holes in the files larger, then the zero-out problem simply goes away.

Right. That was my observation. We can either make the holes larger, by changing:

   pwrite(fd, buf, bufsize, bufsize*10);

to

Quoted 8 lines

   pwrite(fd, buf, bufsize, bufsize*42);

... and then changing the expected values returned by SEEK_HOLE/SEEK_DATA. (By the way; this only matters when we are testing 1k blocks; if we are using a 4k block size in ext4, the test currently passes.)

Or we could set some ext4-specific tuning parameters into the #218

285! :)

Quoted 2 lines

shell script, if the file system in question was ext4.

Quoted 2 lines

I had assumed that folks would prefer making the holes larger, but Eric seemed to prefer the second choice as a better one.

Ok, after the discussion I'm convinced too. Stretching out the allocation to avoid fill-in probably makes sense. But maybe not "42" - how about something much larger, so that any "reasonable" filesystem wouldn't even consider zeroing the range in between?

-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines

I'm seeing Xfstest 285 consistently fail for the 1k test case using the latest dev branch while running on both x86 and ARM. Subtest 08 is the problem. From the test output:

08. Test file with unwritten extents, only have unwritten pages

08.01 SEEK_HOLE expected 0 or 4194304, got 11264.                 FAIL
08.02 SEEK_HOLE expected 1 or 4194304, got 11264.                 FAIL
08.03 SEEK_DATA expected 10240 or 10240, got 0.                   FAIL
08.04 SEEK_DATA expected 10240 or 10240, got 1.                   FAIL

Quoted 6 lines

From previous discussions, we expect 285 to fail in the ext3 (nodelalloc, no flex_bg, and no extents) test case, but in subtest 07. It still does that.

In the dev branch, reverting 4f42f80a8f - "ext4: use s_extent_max_zeroout_kb value as number of kb" - results in success for 285 in the 1k test case.

Hi Eric,

I see what's going on. First of all it isn't a bug. :-) Please let me describe why it happens.

In this commit (4f42f80a8f), it tries to fix a bug that we never zero out an unwritten extent. So after applied it, when an unwritten extent is converted, it could be zeroed out. In xfstests #285 subtest 08 it preallocates an unwritten extent which is 4MB. Then it writes some data at offset 10 * blocksize, which the length is one blocksize, and calles sync_file_range(2) to flush it. So the call trace looks like:

ext4_fallocate()
->ext4_map_blocks()
[one unwritten extent is allocated]
ext4_file_write()
ext4_da_writepages()
->ext4_map_blocks() with EXT4_GET_BLOCKS_CREATE flag

    ->ext4_ext_handle_uninitialized_extents()
      ->ext4_ext_convert_to_initialized()

In ext4_ext_convert_to_initialized() it tries to zero out unwritten extent if condition is matched. Let's see what happens.

case a) 1k block size
max_zeroout: 32
ee_len: 4096
allocated: 4086
m_len: 1

In this case, the following condition is matched.

fs/ext4/extents.c:3310

else if (map->m_lblk - ee_block + map-m_len < max_zeroout)
10 - 0 + 1 < 32

So unwritten extent [0,11] will be converted to written. That is why 11264 (11 * 1k) is returned when we seek a hole from offset 0 and 1, and 0 and 1 are returned when we seek a data from offset 0 and 1.

case b) 4k block size
max_zeroout: 8
ee_len: 1024
allocated: 1014
m_len: 1

In this case, the above condition won't be matched.

else if (map->m_lblk - ee_block + map-m_len < max_zeroout)
10 - 0 + 1 < 8

So only one unwritten extent [10, 1] is converted, and the test can pass.