diff mbox

xfstest-bld generic/018 fails due to e4defrag issue

Message ID CAGW2f1Fn2caBtQPjZnXyMdVf9H6wGvS_0YrLReggwujKZXL8CQ@mail.gmail.com
State Accepted, archived
Headers show

Commit Message

jon ernst April 10, 2014, 4:13 a.m. UTC
On Wed, Apr 9, 2014 at 6:03 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Wed, Apr 09, 2014 at 01:37:04AM -0400, jon ernst wrote:
>> running latest xfstest-bld with latest ext4 kernel "dev"
>> branch(ad6599ab3a).I always get generic/018 failed.
>> Then I took closer look and found out this issue.
>
> That's a renamed tested; it was previously shared/218.  It's a test
> which is known to fail for ext4, since its idea of how a defrag
> program should work is slightly different from how e4defrag works:
>
> shared/218 7s ...   [20:48:32] [20:48:39] - output mismatch (see /results/results-4k/shared/218.out.bad)
>     --- tests/shared/218.out   2014-04-01 18:46:39.000000000 +0000
>     +++ /results/results-4k/shared/218.out.bad               2014-04-03 20:48:39.795694518 +0000
>     @@ -10,7 +10,7 @@
>      After: 1
>      Write backwards sync, but contiguous - should defrag to 1 extent
>      Before: 10
>     -After: 1
>     +After: 10
>      Write backwards sync leaving holes - defrag should do nothing
>      Before: 16
>     ...
>     (Run 'diff -u tests/shared/218.out /results/results-4k/shared/218.out.bad'  to see the entire diff)
>
> What you are seeing is something very different, though.
>
>> Even though the file does exist. e4defrag complains about:
>>
>> (this output comes from kvm guest machine)
>> > e4defrag -v /vdf/testfile
>> Can't get super block info: Success
>> "/vdf/testfile"
>>
>> Is this a known issue or something I did wrong.
>
> Unfortunately, e4defrag has horrible error handling, so we can't see
> the error code properly, so we can't see why it's failing, but this is
> from an attempt to open the file system to get some low-level
> information.
>
> How is /etc/mtab set up on your test machine?  It looks like it failed
> to find block device for the file system in question.
>
>                                          - Ted

I found the root cause of this failure.

The failure case happens on "bigalloc" testing option.
ext2fs_open failed due to EXT2_FLAG_64BITS is not being set in testing
rootfs image. So ext2fs_open in e4defrag.c returns err: 2133571465.

Because bigalloc requires cluster-aware bitfield operations, which
means we need EXT2_FLAG_64BITS.
I see e2image.c creates image always with EXT2_FLAG_64BITS flag. It is
safe to do same thing for e4defrag in my opinion. Please correct me if
I am wrong.




[PATCH] e4defrag: open fs with EXT2_FLAG_64BITS flag

Signed-off-by: Jon Ernst <jonernst07@gmail.com>
---
 misc/e4defrag.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

                                if (mode_flag & DETAIL) {

Comments

Theodore Ts'o April 10, 2014, 1:56 p.m. UTC | #1
On Thu, Apr 10, 2014 at 12:13:49AM -0400, jon ernst wrote:
> 
> Because bigalloc requires cluster-aware bitfield operations, which
> means we need EXT2_FLAG_64BITS.
> I see e2image.c creates image always with EXT2_FLAG_64BITS flag. It is
> safe to do same thing for e4defrag in my opinion. Please correct me if
> I am wrong.

Um.... I *think* so.  e4defrag is one of the less well
tested/maintained parts of e2fsprogs, as well as the kernel-side code
which supports e4defrag.  I can't think of any reason why there would
be any 32-bit dependencies in the kernel side code, although someone
should probably do a quick audit of the e4defrag code to make sure
it's not using blk_t where it should be using blk64_t, or have other
32-bit dependencies.

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick Wong April 10, 2014, 6:42 p.m. UTC | #2
On Thu, Apr 10, 2014 at 09:56:37AM -0400, Theodore Ts'o wrote:
> On Thu, Apr 10, 2014 at 12:13:49AM -0400, jon ernst wrote:
> > 
> > Because bigalloc requires cluster-aware bitfield operations, which
> > means we need EXT2_FLAG_64BITS.
> > I see e2image.c creates image always with EXT2_FLAG_64BITS flag. It is
> > safe to do same thing for e4defrag in my opinion. Please correct me if
> > I am wrong.
> 
> Um.... I *think* so.  e4defrag is one of the less well
> tested/maintained parts of e2fsprogs, as well as the kernel-side code
> which supports e4defrag.  I can't think of any reason why there would
> be any 32-bit dependencies in the kernel side code, although someone
> should probably do a quick audit of the e4defrag code to make sure
> it's not using blk_t where it should be using blk64_t, or have other
> 32-bit dependencies.

From a quick visual inspection and a sparse bitwise check, e4defrag looks 64bit
clean.

--D
> 
> 					- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o April 11, 2014, 3:07 a.m. UTC | #3
On Thu, Apr 10, 2014 at 11:42:15AM -0700, Darrick J. Wong wrote:
> 
> From a quick visual inspection and a sparse bitwise check, e4defrag looks 64bit
> clean.

Thanks for checking!  I've applied Jon's patch.

       	   	      	   	   	 - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jon ernst April 11, 2014, 3:10 a.m. UTC | #4
On Thu, Apr 10, 2014 at 11:07 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Thu, Apr 10, 2014 at 11:42:15AM -0700, Darrick J. Wong wrote:
>>
>> From a quick visual inspection and a sparse bitwise check, e4defrag looks 64bit
>> clean.
>
> Thanks for checking!  I've applied Jon's patch.
>
>                                          - Ted
Thanks Ted, I have also checked e4defrag.c with eyeballs. All physical
blocks are represented by 64bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index 620f4e7..c5a2754 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -1794,7 +1794,7 @@  int main(int argc, char *argv[])

                if (current_uid == ROOT_UID) {
                        /* Get super block info */
-                       ret = ext2fs_open(dev_name, 0, 0, block_size,
+                       ret = ext2fs_open(dev_name,EXT2_FLAG_64BITS,
0, block_size,
                                        unix_io_manager, &fs);
                        if (ret) {