linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] ext4: fallocate insert/collapse range fixes
@ 2017-01-02 12:54 Roman Pen
  2017-01-02 12:54 ` [PATCH 1/3] ext4: Include forgotten start block on fallocate insert range Roman Pen
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Roman Pen @ 2017-01-02 12:54 UTC (permalink / raw)
  Cc: Roman Pen, Namjae Jeon, Theodore Ts'o, Andreas Dilger,
	linux-ext4, linux-kernel

Hi all.

For couple of days I've played with inserting and collapsing range using
fallocate and have found two nasty bugs:

1.  On right shift (insert range) start block is not included in the range
and hole appears at the wrong offset.  The bug can be easily reproduced by
the following test:

    ptr = malloc(4096);
    assert(ptr);

    fd = open("./ext4.file", O_CREAT | O_TRUNC | O_RDWR, 0600);
    assert(fd >= 0);

    rc = fallocate(fd, 0, 0, 8192);
    assert(rc == 0);
    for (i = 0; i < 2048; i++)
            *((unsigned short *)ptr + i) = 0xbeef;
    rc = pwrite(fd, ptr, 4096, 0);
    assert(rc == 4096);
    rc = pwrite(fd, ptr, 4096, 4096);
    assert(rc == 4096);

    for (block = 2; block < 1000; block++) {
            rc = fallocate(fd, FALLOC_FL_INSERT_RANGE, 4096, 4096);
            assert(rc == 0);

            for (i = 0; i < 2048; i++)
                    *((unsigned short *)ptr + i) = block;

            rc = pwrite(fd, ptr, 4096, 4096);
            assert(rc == 4096);
    }

After the test no zero blocks should appear (test always does pwrite() after
fallocate), but zero blocks do exist:

  $ hexdump ./ext4.file | grep '0000 0000'

This bug is targeted by the first patch in the set.

2.  Inside ext4_ext_shift_extents() function ext4_find_extent() is called
without EXT4_EX_NOCACHE flag, which should prevent cache population.  This
leads to outdated offsets in the extents tree and wrong data blocks, which
can be observed doing read().  That is also quite well reproduced by the
test above.

This is fixed by the second patch.

3.  Just a minor optimization: linear search of a extent inside a block is
replaced by a binsearch.  This is the third patch.

Roman Pen (3):
  ext4: Include forgotten start block on fallocate insert range
  ext4: Do not populate extents tree with outdated offsets while
    shifting extents
  ext4: Find desired extent in ext4_ext_shift_extents() using binsearch

 fs/ext4/extents.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Namjae Jeon <namjae.jeon@samsung.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: linux-ext4@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
-- 
2.10.2

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] ext4: Include forgotten start block on fallocate insert range
  2017-01-02 12:54 [PATCH 0/3] ext4: fallocate insert/collapse range fixes Roman Pen
@ 2017-01-02 12:54 ` Roman Pen
  2017-01-02 12:54 ` [PATCH 2/3] ext4: Do not populate extents tree with outdated offsets while shifting extents Roman Pen
  2017-01-02 12:54 ` [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch Roman Pen
  2 siblings, 0 replies; 11+ messages in thread
From: Roman Pen @ 2017-01-02 12:54 UTC (permalink / raw)
  Cc: Roman Pen, Namjae Jeon, Theodore Ts'o, Andreas Dilger,
	linux-ext4, linux-kernel

While doing 'insert range' start block should be also shifted right.
The bug can be easily reproduced by the following test:

    ptr = malloc(4096);
    assert(ptr);

    fd = open("./ext4.file", O_CREAT | O_TRUNC | O_RDWR, 0600);
    assert(fd >= 0);

    rc = fallocate(fd, 0, 0, 8192);
    assert(rc == 0);
    for (i = 0; i < 2048; i++)
            *((unsigned short *)ptr + i) = 0xbeef;
    rc = pwrite(fd, ptr, 4096, 0);
    assert(rc == 4096);
    rc = pwrite(fd, ptr, 4096, 4096);
    assert(rc == 4096);

    for (block = 2; block < 1000; block++) {
            rc = fallocate(fd, FALLOC_FL_INSERT_RANGE, 4096, 4096);
            assert(rc == 0);

            for (i = 0; i < 2048; i++)
                    *((unsigned short *)ptr + i) = block;

            rc = pwrite(fd, ptr, 4096, 4096);
            assert(rc == 4096);
    }

Because start block is not included in the range the hole appears at
the wrong offset (just after the desired offset) and the following
pwrite() overwrites already existent block, keeping hole untouched.

Simple way to verify wrong behaviour is to check zeroed blocks after
the test:

   $ hexdump ./ext4.file | grep '0000 0000'

The root cause of the bug is a wrong range (start, stop], where start
should be inclusive, i.e. [start, stop].

This patch fixes the problem by including start into the range.  But
not to break left shift (range collapse) stop points to the beginning
of the a block, not to the end.

The other not obvious change is an iterator check on validness in a
main loop.  Because iterator is unsigned the following corner case
should be considered with care: insert a block at 0 offset, when stop
variables overflows and never becomes less than start, which is 0.
To handle this special case iterator is set to NULL to indicate that
end of the loop is reached.

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Namjae Jeon <namjae.jeon@samsung.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: linux-ext4@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 fs/ext4/extents.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index c930a0110fb4..b4987ea2ca79 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5353,8 +5353,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 	if (!extent)
 		goto out;
 
-	stop = le32_to_cpu(extent->ee_block) +
-			ext4_ext_get_actual_len(extent);
+	stop = le32_to_cpu(extent->ee_block);
 
        /*
 	 * In case of left shift, Don't start shifting extents until we make
@@ -5393,8 +5392,12 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 	else
 		iterator = &stop;
 
-	/* Its safe to start updating extents */
-	while (start < stop) {
+	/*
+	 * Its safe to start updating extents.  Start and stop are unsigned, so
+	 * in case of right shift if extent with 0 block is reached, iterator
+	 * becomes NULL to indicate the end of the loop.
+	 */
+	while (iterator && start <= stop) {
 		path = ext4_find_extent(inode, *iterator, &path, 0);
 		if (IS_ERR(path))
 			return PTR_ERR(path);
@@ -5422,8 +5425,11 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 					ext4_ext_get_actual_len(extent);
 		} else {
 			extent = EXT_FIRST_EXTENT(path[depth].p_hdr);
-			*iterator =  le32_to_cpu(extent->ee_block) > 0 ?
-				le32_to_cpu(extent->ee_block) - 1 : 0;
+			if (le32_to_cpu(extent->ee_block) > 0)
+				*iterator = le32_to_cpu(extent->ee_block) - 1;
+			else
+				/* Beginning is reached, end of the loop */
+				iterator = NULL;
 			/* Update path extent in case we need to stop */
 			while (le32_to_cpu(extent->ee_block) < start)
 				extent++;
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] ext4: Do not populate extents tree with outdated offsets while shifting extents
  2017-01-02 12:54 [PATCH 0/3] ext4: fallocate insert/collapse range fixes Roman Pen
  2017-01-02 12:54 ` [PATCH 1/3] ext4: Include forgotten start block on fallocate insert range Roman Pen
@ 2017-01-02 12:54 ` Roman Pen
  2017-01-02 12:54 ` [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch Roman Pen
  2 siblings, 0 replies; 11+ messages in thread
From: Roman Pen @ 2017-01-02 12:54 UTC (permalink / raw)
  Cc: Roman Pen, Namjae Jeon, Theodore Ts'o, Andreas Dilger,
	linux-ext4, linux-kernel

Inside ext4_ext_shift_extents() function ext4_find_extent() is called
without EXT4_EX_NOCACHE flag, which should prevent cache population.

This leads to oudated offsets in the extents tree and wrong blocks
afterwards.

Patch fixes the problem providing EXT4_EX_NOCACHE flag for each
ext4_find_extents() call inside ext4_ext_shift_extents function.

Signed-off-by: Roman Pen <roman.penyaev@profitbricks.com>
Cc: Namjae Jeon <namjae.jeon@samsung.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: linux-ext4@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 fs/ext4/extents.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index b4987ea2ca79..9fbf92ca358c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5344,7 +5344,8 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 	ext4_lblk_t stop, *iterator, ex_start, ex_end;
 
 	/* Let path point to the last extent */
-	path = ext4_find_extent(inode, EXT_MAX_BLOCKS - 1, NULL, 0);
+	path = ext4_find_extent(inode, EXT_MAX_BLOCKS - 1, NULL,
+				EXT4_EX_NOCACHE);
 	if (IS_ERR(path))
 		return PTR_ERR(path);
 
@@ -5360,7 +5361,8 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 	 * sure the hole is big enough to accommodate the shift.
 	*/
 	if (SHIFT == SHIFT_LEFT) {
-		path = ext4_find_extent(inode, start - 1, &path, 0);
+		path = ext4_find_extent(inode, start - 1, &path,
+					EXT4_EX_NOCACHE);
 		if (IS_ERR(path))
 			return PTR_ERR(path);
 		depth = path->p_depth;
@@ -5398,7 +5400,8 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 	 * becomes NULL to indicate the end of the loop.
 	 */
 	while (iterator && start <= stop) {
-		path = ext4_find_extent(inode, *iterator, &path, 0);
+		path = ext4_find_extent(inode, *iterator, &path,
+					EXT4_EX_NOCACHE);
 		if (IS_ERR(path))
 			return PTR_ERR(path);
 		depth = path->p_depth;
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-02 12:54 [PATCH 0/3] ext4: fallocate insert/collapse range fixes Roman Pen
  2017-01-02 12:54 ` [PATCH 1/3] ext4: Include forgotten start block on fallocate insert range Roman Pen
  2017-01-02 12:54 ` [PATCH 2/3] ext4: Do not populate extents tree with outdated offsets while shifting extents Roman Pen
@ 2017-01-02 12:54 ` Roman Pen
  2017-01-03 14:40   ` Theodore Ts'o
  2 siblings, 1 reply; 11+ messages in thread
From: Roman Pen @ 2017-01-02 12:54 UTC (permalink / raw)
  Cc: Roman Pen, Namjae Jeon, Theodore Ts'o, Andreas Dilger,
	linux-ext4, linux-kernel

The aim of this patch is to optimize a search of an extent while
doing right shift using binsearch.

Cc: Namjae Jeon <namjae.jeon@samsung.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: linux-ext4@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 fs/ext4/extents.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9fbf92ca358c..f65cc2762780 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5433,10 +5433,15 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
 			else
 				/* Beginning is reached, end of the loop */
 				iterator = NULL;
-			/* Update path extent in case we need to stop */
-			while (le32_to_cpu(extent->ee_block) < start)
-				extent++;
-			path[depth].p_ext = extent;
+			if (le32_to_cpu(extent->ee_block) < start)
+				/*
+				 * Desired extent is somewhere in the middle,
+				 * do binsearch and update a path with it.
+				 */
+				ext4_ext_binsearch(inode, &path[depth], start);
+			else
+				/* Set the first extent */
+				path[depth].p_ext = extent;
 		}
 		ret = ext4_ext_shift_path_extents(path, shift, inode,
 				handle, SHIFT);
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-02 12:54 ` [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch Roman Pen
@ 2017-01-03 14:40   ` Theodore Ts'o
  2017-01-03 20:44     ` Roman Penyaev
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2017-01-03 14:40 UTC (permalink / raw)
  To: Roman Pen; +Cc: Namjae Jeon, Andreas Dilger, linux-ext4, linux-kernel

On Mon, Jan 02, 2017 at 01:54:50PM +0100, Roman Pen wrote:
> The aim of this patch is to optimize a search of an extent while
> doing right shift using binsearch.
> 
> Cc: Namjae Jeon <namjae.jeon@samsung.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
> Cc: linux-ext4@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

I would really appreciate it if patches that touch sensitive code (and
the extents manipulation code is an example of code which is fairly
subtle and fragile) are tested using xfstests first before you submit
them for review.  I've done a lot of work to make using xfstests
simple and easy for ext4 developers.  See:

	http://thunk.org/gce-xfstests

(especially the last slide :-).

BEGIN TEST 4k: Ext4 4k block Mon Jan  2 23:06:02 EST 2017
Failures: generic/061 generic/063 generic/075 generic/091 generic/112 generic/127 generic/231 generic/263 generic/389
BEGIN TEST 1k: Ext4 1k block Mon Jan  2 23:56:29 EST 2017
Failures: ext4/307 generic/013 generic/014 generic/016 generic/018 generic/020 generic/021 generic/022 generic/023
generic/024 generic/025 generic/028 generic/035 generic/036 generic/058 generic/060 generic/061 generic/063 generic/067
generic/070 generic/072 generic/074 generic/075 generic/077 generic/078 generic/080 generic/081 generic/082 generic/086
generic/087 generic/088 generic/089 generic/091 generic/092 generic/100 generic/112 generic/113 generic/114 generic/117
generic/123 generic/124 generic/126 generic/127 generic/131 generic/133 generic/184 generic/192 generic/193 generic/198
generic/207 generic/208 generic/209 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/221
generic/228 generic/231 generic/233 generic/236 generic/237 generic/239 generic/240 generic/241 generic/245 generic/246
generic/247 generic/248 generic/249 generic/255 generic/256 generic/257 generic/258 generic/263 generic/269 generic/270
generic/273 generic/285 generic/286 generic/299 generic/300 generic/306 generic/308 generic/309 generic/310 generic/313
generic/314 generic/315 generic/316 generic/323 generic/355 generic/360 generic/361 generic/375 generic/378 generic/389
generic/391 shared/298

The 1k test failures look extremely scary, but that's because
generic/013 corrupted the file system, and caused all of the
subsequent tests using the test device to fail.  Of course, patches
_shouldn't_ be corrupting file systems.  That's a regression which
makes everyone said.  :-)

						- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-03 14:40   ` Theodore Ts'o
@ 2017-01-03 20:44     ` Roman Penyaev
  2017-01-03 22:15       ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Roman Penyaev @ 2017-01-03 20:44 UTC (permalink / raw)
  To: Theodore Ts'o, Roman Pen, Namjae Jeon, Andreas Dilger,
	linux-ext4, linux-kernel

On Tue, Jan 3, 2017 at 3:40 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Mon, Jan 02, 2017 at 01:54:50PM +0100, Roman Pen wrote:
>> The aim of this patch is to optimize a search of an extent while
>> doing right shift using binsearch.
>>
>> Cc: Namjae Jeon <namjae.jeon@samsung.com>
>> Cc: "Theodore Ts'o" <tytso@mit.edu>
>> Cc: Andreas Dilger <adilger.kernel@dilger.ca>
>> Cc: linux-ext4@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>
> I would really appreciate it if patches that touch sensitive code (and
> the extents manipulation code is an example of code which is fairly
> subtle and fragile) are tested using xfstests first before you submit
> them for review.  I've done a lot of work to make using xfstests
> simple and easy for ext4 developers.  See:
>
>         http://thunk.org/gce-xfstests
>
> (especially the last slide :-).

Thanks.  Those slides that's exactly what I needed.

>
> BEGIN TEST 4k: Ext4 4k block Mon Jan  2 23:06:02 EST 2017
> Failures: generic/061 generic/063 generic/075 generic/091 generic/112 generic/127 generic/231 generic/263 generic/389
> BEGIN TEST 1k: Ext4 1k block Mon Jan  2 23:56:29 EST 2017
> Failures: ext4/307 generic/013 generic/014 generic/016 generic/018 generic/020 generic/021 generic/022 generic/023
> generic/024 generic/025 generic/028 generic/035 generic/036 generic/058 generic/060 generic/061 generic/063 generic/067
> generic/070 generic/072 generic/074 generic/075 generic/077 generic/078 generic/080 generic/081 generic/082 generic/086
> generic/087 generic/088 generic/089 generic/091 generic/092 generic/100 generic/112 generic/113 generic/114 generic/117
> generic/123 generic/124 generic/126 generic/127 generic/131 generic/133 generic/184 generic/192 generic/193 generic/198
> generic/207 generic/208 generic/209 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/221
> generic/228 generic/231 generic/233 generic/236 generic/237 generic/239 generic/240 generic/241 generic/245 generic/246
> generic/247 generic/248 generic/249 generic/255 generic/256 generic/257 generic/258 generic/263 generic/269 generic/270
> generic/273 generic/285 generic/286 generic/299 generic/300 generic/306 generic/308 generic/309 generic/310 generic/313
> generic/314 generic/315 generic/316 generic/323 generic/355 generic/360 generic/361 generic/375 generic/378 generic/389
> generic/391 shared/298
>
> The 1k test failures look extremely scary, but that's because
> generic/013 corrupted the file system, and caused all of the
> subsequent tests using the test device to fail.  Of course, patches
> _shouldn't_ be corrupting file systems.  That's a regression which
> makes everyone said.  :-)
>

True, I ran only my own tests for inserting/collapsing big amount of
blocks.

My xfstests output is the following:

(I had to say that right now I am testing on 4.4.28 kernel and testing
 on latest sources taken from linux-next will require some time, but of
 course I will retest and send up-to-date results)

---
Failures: ext4/302 ext4/303 ext4/304 generic/061 generic/063
generic/075 generic/079 generic/091 generic/112 generic/127
generic/252 generic/263
Failed 12 of 200 tests
---

Many of tests were ignored, because of reflink which is not supported, also
there are some failures and I will try to figure out is that related to my
changes, but still I do not see massive corruptions.

Quick questions regarding xfstests:

1. What is the optimal size for $TEST_DEV ? I see these seconds numbers on a
small Vm (disk is 16gb):

ext4/007         22s

and these numbers on a server with 2TB disk:

ext4/007         547s

007 test does e2fsck, so it depends on $TEST_DEV size.  So what optimal
size to choose to cover all corner cases but not to wait forever?

2.  I see many of the tests are ignored.  Do we have some perfect run,
reference, Standard, to see how many tests run on up-to-date kernel?
E.g. I see generic/038 has this output:
   "[not run] This test requires at least 10GB free".

Increasing size of $SCRATCH partition will bring the test to live.
So would be nice to have some reference numbers that those tests
are expected to run and to pass.

--
Roman

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-03 20:44     ` Roman Penyaev
@ 2017-01-03 22:15       ` Theodore Ts'o
  2017-01-04 18:32         ` Roman Penyaev
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2017-01-03 22:15 UTC (permalink / raw)
  To: Roman Penyaev; +Cc: Namjae Jeon, Andreas Dilger, linux-ext4, linux-kernel

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

On Tue, Jan 03, 2017 at 09:44:15PM +0100, Roman Penyaev wrote:
> 
> (I had to say that right now I am testing on 4.4.28 kernel and testing
>  on latest sources taken from linux-next will require some time, but of
>  course I will retest and send up-to-date results)
> 
> ---
> Failures: ext4/302 ext4/303 ext4/304 generic/061 generic/063
> generic/075 generic/079 generic/091 generic/112 generic/127
> generic/252 generic/263
> Failed 12 of 200 tests

You didn't say what file system configuration you're using, but I'm
assuming it's a default ext4 4k configuration?  One of the things
about my kvm-xfstests and gce-xfstests setup is that I test a range of
file system configurations, and there are specific exclude files to
skip certain known failures.  Currently we are skipping the following
tests globally (from kvm-xfstests/test-appliances/files/root/fs/ext4/exclude):

# generic/223 tests file alignment, which works on ext4 only by
# accident because we're not RAID stripe aware yet, and works at all
# because we have bias towards aligning on power-of-two block numbers.
# It is a flaky test for some configurations, so skip it.
generic/223

# ext4/304 fails for all configurations, and this appears to be at
# test or fio bug.
#
ext4/304

A recent run of v4.10-rc2 shows that on the 4k configuration, we're only
failing one test:

BEGIN TEST 4k: Ext4 4k block Tue Jan  3 00:20:39 EST 2017
Failures: generic/389

Somewhat older test runs (late in the 4.9 development cycle, before
4.9 final was finally shipped), I saw test failures of generic/082 and
generic/095 as well.

> 1. What is the optimal size for $TEST_DEV ? I see these seconds numbers on a
> small Vm (disk is 16gb):
> 
> ext4/007         22s

With kvm-xfstests and gce-xfstests I normally use a 5G device for
test_dev and scratch_dev except for the bigalloc test configuration
where I use a 20GB device.

> 2.  I see many of the tests are ignored.  Do we have some perfect run,
> reference, Standard, to see how many tests run on up-to-date kernel?
> E.g. I see generic/038 has this output:
>    "[not run] This test requires at least 10GB free".

I currently don't publish one.  What I normally do for myself is run
gce-xfstests at the beginning of the development cycle (e.g., versus
4.10-rc2) and then look for regressions.  The problem is sometimes
regressions are sometimes caused by new tests being added, or changes
pulled in via other trees.

In general the goal is to reduce the number of test failures down to
zero, or else, just suppress them using exclude files, but I haven't
had time to look at a lot of the more recent test failures.  Some of
them, especially for the 1k test case, seem to be quota releated, and
may very be test bugs where the golden output of the tests assume a 4k
block size.  This is definitely true for the bigalloc test cases,
where a number of the failures are known test bugs that we haven't had
time to address.

> Increasing size of $SCRATCH partition will bring the test to live.
> So would be nice to have some reference numbers that those tests
> are expected to run and to pass.

Sure; on my to do list is to update the published kvm-xfstests and
gce-xfstests images to something newer, and when I do that, I can
publish "official" reference test outputs using gce-xfstests.  I've
attached a gce-xfstests output.  It has a huge number of failures
because in the encryption test case because the kernel which was used
for this test run was missing a fix which only landed in Linus's tree
today (see commit fe4f6c801c03b: "fscrypt: fix the
test_dummy_encryption mount option").

You'll see that modulo the encryption failures and some failures on
the 1k config case which I need to track down and iron out, we're in
pretty good shape.  Below please find the summary; attached please
find the full log file.

CMDLINE: full
FSTESTIMG: gce-xfstests/xfstests-201612072310
FSTESTVER: e2fsprogs	v1.43.3-30-g8df85fb (Sun, 4 Sep 2016 21:32:35 -0400)
FSTESTVER: fio		fio-2.14-45-g43f248c (Mon, 24 Oct 2016 20:48:43 -0600)
FSTESTVER: quota		81aca5c (Tue, 12 Jul 2016 16:15:45 +0200)
FSTESTVER: xfsprogs	v4.8.0-rc3 (Mon, 3 Oct 2016 14:25:45 +1100)
FSTESTVER: xfstests-bld	71223ea (Wed, 7 Dec 2016 22:20:14 -0500)
FSTESTVER: xfstests	linux-v3.8-1266-g8d57865 (Wed, 7 Dec 2016 22:56:52 -0500)
FSTESTVER: kernel	4.10.0-rc2-ext4-00001-geb590c0248f4 #176 SMP Tue Jan 3 00:17:59 EST 2017 x86_64
FSTESTCFG: "all"
FSTESTSET: "-g auto"
FSTESTEXC: ""
FSTESTOPT: "aex"
MNTOPTS: ""
CPUS: "2"
MEM: "7477.96"
MEM: 7680 MB (Max capacity)
BEGIN TEST 4k: Ext4 4k block Tue Jan  3 00:20:39 EST 2017
Failures: generic/389
BEGIN TEST 1k: Ext4 1k block Tue Jan  3 01:17:12 EST 2017
Failures: ext4/307 generic/018 generic/076 generic/077 generic/117 generic/233 generic/256 generic/269 generic/270 generic/273 generic/299 generic/300 generic/361 generic/389
BEGIN TEST ext3: Ext4 4k block emulating ext3 Tue Jan  3 02:18:13 EST 2017
Failures: generic/382 generic/389
BEGIN TEST encrypt: Ext4 encryption Tue Jan  3 03:10:27 EST 2017
Failures: ext4/001 ext4/002 ext4/003 ext4/020 ext4/021 ext4/022 ext4/271 ext4/306 ext4/307 ext4/308 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/008 generic/009 generic/010 generic/012 generic/014 generic/015 generic/016 generic/017 generic/020 generic/021 generic/022 generic/023 generic/024 generic/025 generic/028 generic/029 generic/030 generic/031 generic/032 generic/033 generic/034 generic/035 generic/037 generic/039 generic/040 generic/041 generic/053 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/062 generic/063 generic/064 generic/065 generic/066 generic/067 generic/069 generic/070 generic/071 generic/073 generic/074 generic/075 generic/077 generic/078 generic/080 generic/084 generic/086 generic/087 generic/088 generic/089 generic/090 generic/098 generic/100 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/109 generic/112 generic/117 generic/120 generic/123 generic/124 generic/126 generic/128 generic/129 generic/132 generic/141 generic/169 generic/177 generic/192 generic/193 generic/213 generic/215 generic/228 generic/234 generic/236 generic/237 generic/241 generic/245 generic/246 generic/247 generic/248 generic/249 generic/255 generic/256 generic/257 generic/274 generic/275 generic/280 generic/286 generic/294 generic/306 generic/307 generic/309 generic/313 generic/316 generic/319 generic/321 generic/322 generic/325 generic/335 generic/336 generic/337 generic/340 generic/341 generic/342 generic/343 generic/344 generic/345 generic/346 generic/354 generic/361 generic/371 generic/375 generic/376 generic/377 generic/378 generic/382 generic/389 generic/391 generic/393 shared/001 shared/272 shared/298
BEGIN TEST nojournal: Ext4 4k block w/ no journal Tue Jan  3 03:28:25 EST 2017
Failures: ext4/301 generic/389
BEGIN TEST ext3conv: Ext4 4k block w/nodelalloc and no flex_bg Tue Jan  3 04:19:52 EST 2017
Failures: generic/347 generic/389
BEGIN TEST adv: Ext4 advanced features (inline_data, metadata_csum, 64bit) Tue Jan  3 05:12:40 EST 2017
Failures: generic/389
BEGIN TEST dioread_nolock: Ext4 4k block w/dioread_nolock Tue Jan  3 06:04:48 EST 2017
Failures: generic/389
BEGIN TEST data_journal: Ext4 4k block w/data=journal Tue Jan  3 06:56:19 EST 2017
Failures: generic/347 generic/389
BEGIN TEST bigalloc: Ext4 4k block w/bigalloc Tue Jan  3 08:12:33 EST 2017
Failures: ext4/004 generic/204 generic/219 generic/235 generic/273 generic/389
BEGIN TEST bigalloc_1k: Ext4 1k block w/bigalloc Tue Jan  3 09:01:49 EST 2017
Failures: ext4/004 generic/204 generic/235 generic/273 generic/389

							- Ted

[-- Attachment #2: gce-xfstests.output.xz --]
[-- Type: application/x-xz, Size: 39760 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-03 22:15       ` Theodore Ts'o
@ 2017-01-04 18:32         ` Roman Penyaev
  2017-01-04 23:58           ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Roman Penyaev @ 2017-01-04 18:32 UTC (permalink / raw)
  To: Theodore Ts'o, Roman Penyaev, Namjae Jeon, Andreas Dilger,
	linux-ext4, linux-kernel

On Tue, Jan 3, 2017 at 11:15 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jan 03, 2017 at 09:44:15PM +0100, Roman Penyaev wrote:
>>
>> (I had to say that right now I am testing on 4.4.28 kernel and testing
>>  on latest sources taken from linux-next will require some time, but of
>>  course I will retest and send up-to-date results)
>>
>> ---
>> Failures: ext4/302 ext4/303 ext4/304 generic/061 generic/063
>> generic/075 generic/079 generic/091 generic/112 generic/127
>> generic/252 generic/263
>> Failed 12 of 200 tests
>
> You didn't say what file system configuration you're using, but I'm
> assuming it's a default ext4 4k configuration?

Yep, that was the default run, 4K block.

> One of the things
> about my kvm-xfstests and gce-xfstests setup is that I test a range of
> file system configurations, and there are specific exclude files to
> skip certain known failures.  Currently we are skipping the following
> tests globally (from kvm-xfstests/test-appliances/files/root/fs/ext4/exclude):

[ cut ]

> You'll see that modulo the encryption failures and some failures on
> the 1k config case which I need to track down and iron out, we're in
> pretty good shape.  Below please find the summary; attached please
> find the full log file.

Thanks a lot.  Perfect reference. That is quite enough for a good start.

This kvm-xfstests is a nice thingy.  That was pretty annoying to rebuild
xfsprogs on a debian jessie, which does not have 'xfs_io -c finsert' by
default.

Theodore, recently I sent an ext4/023 xfstest test.  New test reproduces
the problem with a wrong right shift.  I tried to make the test explicit,
but not sure succeeded or not.  Probably, md5sum is not a good choice to
make things clear, but hexdump of 1000 blocks is quite large (~80K).
Could you please take a look?

Now I am running kvm-xfstests, will figure out what I've done wrong with
recent ext4 patches.

--
Roman

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-04 18:32         ` Roman Penyaev
@ 2017-01-04 23:58           ` Theodore Ts'o
  2017-01-05  8:02             ` Roman Penyaev
  2017-01-06 20:21             ` Roman Penyaev
  0 siblings, 2 replies; 11+ messages in thread
From: Theodore Ts'o @ 2017-01-04 23:58 UTC (permalink / raw)
  To: Roman Penyaev; +Cc: Namjae Jeon, Andreas Dilger, linux-ext4, linux-kernel

It looks like the original (before your patch) 1k failures due to a
bug introduced via the block.git tree, which has since been fixed in
Linus's mainline tree as of today.  It wouldn't surprise me if the bug
interacted poorly your changes, so things will probably better with
your patches applied directly on top of the tip of Linus's tree.

That being said, it looks like there were still regressions introduced
on the 4k configuration, so I'm in the middle of rerunning my baseline
and trying out your patches as well.

Cheers,

					- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-04 23:58           ` Theodore Ts'o
@ 2017-01-05  8:02             ` Roman Penyaev
  2017-01-06 20:21             ` Roman Penyaev
  1 sibling, 0 replies; 11+ messages in thread
From: Roman Penyaev @ 2017-01-05  8:02 UTC (permalink / raw)
  To: Theodore Ts'o, Roman Penyaev, Namjae Jeon, Andreas Dilger,
	linux-ext4, linux-kernel

On Thu, Jan 5, 2017 at 12:58 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> It looks like the original (before your patch) 1k failures due to a
> bug introduced via the block.git tree, which has since been fixed in
> Linus's mainline tree as of today.  It wouldn't surprise me if the bug
> interacted poorly your changes, so things will probably better with
> your patches applied directly on top of the tip of Linus's tree.
>
> That being said, it looks like there were still regressions introduced
> on the 4k configuration, so I'm in the middle of rerunning my baseline
> and trying out your patches as well.

It seems that some of the "finsert" tests from xfstests are broken by my
patches.  Now I am able to run all configurations with a kvm-xfstests
help, so will take a look deeply.

--
Roman

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch
  2017-01-04 23:58           ` Theodore Ts'o
  2017-01-05  8:02             ` Roman Penyaev
@ 2017-01-06 20:21             ` Roman Penyaev
  1 sibling, 0 replies; 11+ messages in thread
From: Roman Penyaev @ 2017-01-06 20:21 UTC (permalink / raw)
  To: Theodore Ts'o, Roman Penyaev, Namjae Jeon, Andreas Dilger,
	linux-ext4, linux-kernel

On Thu, Jan 5, 2017 at 12:58 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> It looks like the original (before your patch) 1k failures due to a
> bug introduced via the block.git tree, which has since been fixed in
> Linus's mainline tree as of today.  It wouldn't surprise me if the bug
> interacted poorly your changes, so things will probably better with
> your patches applied directly on top of the tip of Linus's tree.
>
> That being said, it looks like there were still regressions introduced
> on the 4k configuration, so I'm in the middle of rerunning my baseline
> and trying out your patches as well.

I found a bug in my third patch, where I try to optimize linear search.
I missed the fact, that ext4_ext_binsearch() searches for the closest
extent from the left, but this linear search code does search for closest
extent from the right.

I ran the 'kvm-xfstests.sh -c 4k -g auto' against b25ead75e7b6 4.10-rc2
with and without my changes.  No failures.

1k configuration was run against latest Linus's mainline.  Also no
regressions were found: these two "generic/270, generic/273" fail
regardless my changes.

I will resend the patchset without latest patch shortly.

--
Roman

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-01-06 20:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-02 12:54 [PATCH 0/3] ext4: fallocate insert/collapse range fixes Roman Pen
2017-01-02 12:54 ` [PATCH 1/3] ext4: Include forgotten start block on fallocate insert range Roman Pen
2017-01-02 12:54 ` [PATCH 2/3] ext4: Do not populate extents tree with outdated offsets while shifting extents Roman Pen
2017-01-02 12:54 ` [PATCH 3/3] ext4: Find desired extent in ext4_ext_shift_extents() using binsearch Roman Pen
2017-01-03 14:40   ` Theodore Ts'o
2017-01-03 20:44     ` Roman Penyaev
2017-01-03 22:15       ` Theodore Ts'o
2017-01-04 18:32         ` Roman Penyaev
2017-01-04 23:58           ` Theodore Ts'o
2017-01-05  8:02             ` Roman Penyaev
2017-01-06 20:21             ` Roman Penyaev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).