xfs
[Top] [All Lists]

Re: [PATCH] fix dir2 shortform structures on ARM old ABI

To: xfs-oss <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH] fix dir2 shortform structures on ARM old ABI
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 02 May 2008 15:55:45 -0500
In-reply-to: <480E89B5.8070006@xxxxxxxxxxx>
References: <47DB4181.7040603@xxxxxxxxxxx> <480E89B5.8070006@xxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Thunderbird 2.0.0.12 (X11/20080226)
Eric Sandeen wrote:
> Eric Sandeen wrote:
>> This should fix the longstanding issues with xfs and old ABI
>> arm boxes, which lead to various asserts and xfs shutdowns,
>> and for which an (incorrect) patch has been floating around
>> for years.  (Said patch made ARM internally consistent, but
>> altered the normal xfs on-disk format such that it looked
>> corrupted on other architectures):
>> http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
> 
> ping again...

ping #3...



> There is still the cache flushing issue, I guess, but I think the
> on-disk alignment is still worth fixing.
> 
> Oh, I'm sorry Jeff - I meant "fixing."  :)
> 
> Thanks,
> -Eric
> 
>> Old ABI ARM has interesting packing & padding; for example
>> on ARM old abi:
>>
>> struct xfs_dir2_sf_entry {
>>      __uint8_t                  namelen;      /*     0     1 */
>>
>>      /* XXX 3 bytes hole, try to pack */
>>
>>      xfs_dir2_sf_off_t          offset;       /*     4     4 */
>>      __uint8_t                  name[1];      /*     8     1 */
>>
>>      /* XXX 3 bytes hole, try to pack */
>>
>>      xfs_dir2_inou_t            inumber;      /*    12     8 */
>>
>>      /* size: 20, cachelines: 1 */
>>      /* sum members: 14, holes: 2, sum holes: 6 */
>>      /* last cacheline: 20 bytes */
>> };
>>
>> but on x86:
>>
>> struct xfs_dir2_sf_entry {
>>      __uint8_t                  namelen;      /*     0     1 */
>>      xfs_dir2_sf_off_t          offset;       /*     1     2 */
>>      __uint8_t                  name[1];      /*     3     1 */
>>      xfs_dir2_inou_t            inumber;      /*     4     8 */
>>
>>      /* size: 12, cachelines: 1 */
>>      /* last cacheline: 12 bytes */
>> };
>>
>> ... this sort of discrepancy leads to problems.
>>
>> I've verified this patch by comparing the on-disk structure 
>> layouts using pahole from the dwarves package, as well as 
>> running through a bit of xfsqa under qemu-arm, modified so 
>> that the check/repair phase after each test actually executes 
>> check/repair from the x86 host, on the filesystem populated 
>> by the arm emulator.  Thus far it all looks good.
>>
>> There are 2 other structures with extra padding at the end, 
>> but they don't seem to cause trouble.  I suppose they could 
>> be packed as well: xfs_dir2_data_unused_t and xfs_dir2_sf_t.
>>
>> Note that userspace needs a similar treatment, and any
>> filesystems which were running with the previous rogue
>> "fix" will now see corruption (either in the kernel, or
>> during xfs_repair) with this fix properly in place; it 
>> may be worth teaching xfs_repair to identify and fix that 
>> specific issue.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
>>
>> ---
>>
>> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
>> ===================================================================
>> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
>> +++ linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
>> @@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
>>      return x;
>>  }
>>  
>> +/* ARM old ABI has some weird alignment/padding */
>> +#if defined(__arm__) && !defined(__ARM_EABI__)
>> +#define __arch_pack __attribute__((packed))
>> +#else
>> +#define __arch_pack
>> +#endif
>> +
>>  #endif /* __XFS_LINUX__ */
>> Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
>> ===================================================================
>> --- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
>> +++ linux-2.6.24/fs/xfs/xfs_dir2_sf.h
>> @@ -62,7 +62,7 @@ typedef union {
>>   * Normalized offset (in a data block) of the entry, really 
>> xfs_dir2_data_off_t.
>>   * Only need 16 bits, this is the byte offset into the single block form.
>>   */
>> -typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
>> +typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
>>  
>>  /*
>>   * The parent directory has a dedicated field, and the self-pointer must
>> @@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
>>      __uint8_t               count;          /* count of entries */
>>      __uint8_t               i8count;        /* count of 8-byte inode #s */
>>      xfs_dir2_inou_t         parent;         /* parent dir inode number */
>> -} xfs_dir2_sf_hdr_t;
>> +} __arch_pack xfs_dir2_sf_hdr_t;
>>  
>>  typedef struct xfs_dir2_sf_entry {
>>      __uint8_t               namelen;        /* actual name length */
>>      xfs_dir2_sf_off_t       offset;         /* saved offset */
>>      __uint8_t               name[1];        /* name, variable size */
>>      xfs_dir2_inou_t         inumber;        /* inode number, var. offset */
>> -} xfs_dir2_sf_entry_t;
>> +} __arch_pack xfs_dir2_sf_entry_t; 
>>  
>>  typedef struct xfs_dir2_sf {
>>      xfs_dir2_sf_hdr_t       hdr;            /* shortform header */
>>
>>
>>
> 
> 


<Prev in Thread] Current Thread [Next in Thread>