From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Price <anprice@redhat.com>
Date: Tue, 16 Feb 2016 13:20:14 +0000
Subject: [Cluster-devel] Review of gfs2-utils test failure and gfs2_dirent
	changes
Message-ID: <56C3220E.2020807@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi,

As a recent build failure[0] happened due to changes in the layout of 
struct gfs2_dirent causing a (broken) test to fail, I felt it would be a 
good idea to step through it carefully to make sure the struct changes 
haven't introduced a subtle bug.

The package build failure was caused by the libgfs2/meta.c unit test 
failing with this error message:

     gfs2_dirent: __pad: offset is 28, expected 26
     gfs2_dirent: size mismatch between struct 40 and fields 38

The fact that it was even looking at the __pad field was curious as the 
code generating the values was

     #ifdef GFS2_HAS_DE_RAHEAD
     F(de_rahead)
     RF(__pad2)
     #else
     RF(__pad)
     #endif

and GFS2_HAS_DE_RAHEAD should have been defined at that point. It turned 
out that this was due to meta.c not #including clusterautoconfig.h which 
contains the GFS2_HAS_* defines set by autoconf.

So why did the offset of __pad change, and the struct size (appear to) 
change? Originally the __pad field occupied the last 14 bytes of the struct:

     struct gfs2_dirent {
             ...
     // Offset 26:
             __u8 __pad[14];
     };

And then the de_rahead field was added, leaving the __pad field 
unchanged and unioned with the new field and padding:

     struct gfs2_dirent {
             ...
     // Offset 26:
             union {
                     __u8 __pad[14];
                     struct {
                             __be16 de_rahead;
                             __u8 pad2[12];
                     };
             };
     };

This didn't cause a test failure because __pad was still 14 bytes and 
where it was expected to be, but then the de_cookie change[1] moved 
things around slightly:

     struct gfs2_dirent {
             ...
     // Offset 26:
             __be16 de_rahead;
             union {
                     __u8 __pad[12];
                     struct {
                             __u32 de_cookie; /* ondisk value not used */
                             __u8 pad3[8];
                     };
              };
     };

So de_rahead is still at offset 26, no longer unioned with __pad but 
still occupying the first 2 bytes of the 14, and __pad has become 2 
bytes smaller to reflect that. This caused the test to fail because it 
knew nothing about de_rahead now lying outside of the union and so still 
expected __pad to be at offset 26 (calculated additively).

In conclusion, it doesn't look like there's a bug in gfs2 here as the 
offsets and sizes of the used fields haven't changed, and the test 
failure was entirely due to the buggy test.

Andy

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1307532
[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=471f3db