Re: [PATCH] gfs2 endianness bug: be16 assigned to be32 field

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Sat, 2006-10-14 at 11:32 -0700, Linus Torvalds wrote:
> 
> On Sat, 14 Oct 2006, Al Viro wrote:
> >
> > -	leaf->lf_dirent_format = cpu_to_be16(GFS2_FORMAT_DE);
> > +	leaf->lf_dirent_format = cpu_to_be32(GFS2_FORMAT_DE);
> 
> Hmm. Doesn't this change the on-disk format on a LE machine (eg x86)?
> 
> In other words, this change makes me nervous. A quick grep seems to 
> indicate that nothing actually _uses_ this field, so maybe we don't really 
> care, but I think we should double-check that this is what the GFS2 people 
> really want.
> 
> If we don't want to change the format on a LE machine, then maybe the 
> gfs2_leaf structure should be changed to be
> 
> 	..
> 	__be16 lf_dirent_format;
> 	__be16 lf_unused;
> 	..
> 
> which should keep the bits in the same position on LE.
> 
Its a tricky question, but I think on balance that Al's proposed fix is
the right one...

> Regardless, the old code was clearly wrong, since it gives different 
> on-disk format for a big-endian and a little-endian machine. Al's fix is 
> proper, but perhaps people would prefer something that breaks the BE 
> format rather than the LE format. Hmm?
> 
> Steven?
> 
> 		Linus

As you say, this field isn't actually used by anything. Its whole reason
for existing is to be forwards compatible with GFS1. So I'd be inclined
to accept the patch as proposed in order that as few filesystems as
possible will have the incorrect format. We'll make a note of this
problem and ensure that fsck can fix up the error and if we need to use
this field in future, then we'll take into account that there may be
some filesystems with the incorrect format number.

I think its fairly unlikely that we will need to use it in the near
future though... Its original purpose was to allow future changes in the
metadata format, but this is also allowed for by the mh_format field in
the common metadata header, so I'm not entirely sure why a second format
field was included in this structure (the directory leaf header).

Steve.