Re: [PATCH] (and bad attr2 bug) - pack xfs_sb_t for 64-bit arches

Russell Cattelan
2006-11-17T15:54:52Z

David Chinner wrote:

>
>@@ -135,9 +136,12 @@
> __uint8_t sb_shared_vn; /* shared version number */
> xfs_extlen_t sb_inoalignmt; /* inode chunk alignment, fsblocks */
> __uint32_t sb_unit; /* stripe or raid unit */
>- __uint32_t sb_width; /* stripe or raid width */
>+ __uint32_t sb_width; /* stripe or raid width */
> __uint8_t sb_dirblklog; /* log2 of dir block size (fsbs) */
>- __uint8_t sb_dummy[7]; /* padding */
>+ __uint8_t sb_logsectlog; /* log2 of the log sector size */
>+ __uint16_t sb_logsectsize; /* sector size for the log, bytes */
>+ __uint32_t sb_logsunit; /* stripe unit size for the log */
>+ __uint32_t sb_features2; /* additional feature bits */
> } xfs_sb_t;
>
>So before the sector size > 512 bytes code, there was padding to push the
>superblock out to 64bit alignement so that sb_dirblklog was correctly aligned.
>The xfs_sb_info structure:
>
> { offsetof(xfs_sb_t, sb_unit), 0 },
> { offsetof(xfs_sb_t, sb_width), 0 },
> { offsetof(xfs_sb_t, sb_dirblklog), 0 },
>- { offsetof(xfs_sb_t, sb_dummy), 1 },
>+ { offsetof(xfs_sb_t, sb_logsectlog), 0 },
>+ { offsetof(xfs_sb_t, sb_logsectsize),0 },
> { offsetof(xfs_sb_t, sb_logsunit), 0 },
>+ { offsetof(xfs_sb_t, sb_features2), 0 },
> { sizeof(xfs_sb_t), 0 }
> };
>
>had the sb_dummy field as "no translate" so it effectively ignored it
>but it ensured that sb_dirblklog was sized correctly.
>
>The real bug here was whoever removed the dummy field and did not
>replace that with a comment ot say that the xfs_sb strucutre needs
>to be padded to 64 bits to ensure translation worked properly
>on 64 bit systems.
>
>I'd prefer explicit padding (with warning comments) over packing
>the structure. Thoughts?
>
>
That seems safer to me and the comments will make the next person to
muck with
the structure think about padding.

>Cheers,
>
>Dave.
>
>