Date: Mon, 18 May 2009 17:28:14 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ulf Lilleengen <ulf.lilleengen@gmail.com> Cc: freebsd-fs@freebsd.org Subject: Re: MFC of svn commit: r187395 - head/sys/gnu/fs/ext2fs Message-ID: <20090518160049.E2212@besplex.bde.org> In-Reply-To: <20090518055040.GA3501@carrot.geeknest.org> References: <20090518055040.GA3501@carrot.geeknest.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 18 May 2009, Ulf Lilleengen wrote: > I was wondering if there is anything against MFCing this change, as it has > not yet happened? Yes, there is. > I saw there was some discussion around the patch, but it > fixes at least these PRs 77826, 81568 and 105093 according to Aditya Sarawgi. > I saw the problems outlined in > http://lists.freebsd.org/pipermail/svn-src-head/2009-January/002937.html > > So I haven't gone ahead with it. > > Log: > - Obtain inode sizes and location of the first inode based on the contents > of superblock rather than using hardcoded values. This fixes ext2fs on > filesystems with inode sized other than 128. > > Submitted by: Alex Lyashkov <Alexey.Lyashkov at Sun.COM> (based on) > MFC after: 2 weeks The submitted patch was committed verbatim but was not suitable for committing verbatim. It turns the nearby comments and ifdef tangle in ext2_fs.h into nonsense, and is wrong (unlike for ifdefed-out code in the ifdef tangle) for the case of old file systems that don't have the inode size or the first inode number in the superblock. >From ext2_fs.h: % /* % * Note: under FreeBSD, the "user" versions of the following macros are % * used (and must be used) in most cases, because ((s)->u.ext2_sb.s_es is % * not accessible. This depends on __KERNEL__ not being defined for % * kernel builds under FreeBSD. % */ Most of this mail is an expansion of this comment. % ... % #ifdef notyet % #ifdef __KERNEL__ % #define EXT2_ADDR_PER_BLOCK_BITS(s) ((s)->u.ext2_sb.s_addr_per_block_bits) % #define EXT2_INODE_SIZE(s) ((s)->u.ext2_sb.s_inode_size) % #define EXT2_FIRST_INO(s) ((s)->u.ext2_sb.s_first_ino) % #else % #define EXT2_INODE_SIZE(s) (((s)->s_rev_level == EXT2_GOOD_OLD_REV) ? \ % EXT2_GOOD_OLD_INODE_SIZE : \ % (s)->s_inode_size) % #define EXT2_FIRST_INO(s) (((s)->s_rev_level == EXT2_GOOD_OLD_REV) ? \ % EXT2_GOOD_OLD_FIRST_INO : \ % (s)->s_first_ino) % #endif % #else /* !notyet */ Except for the "notyet" ifdef, this is identical with at least old versions of ext2_fs.h (same name) in Linux. The !__KERNEL__ case needs to be more complicated so that it can work without extra initialization in userland. The kernel case can be simpler because the kernel can do extra initialization consisting of initializing the "new" superblock fields in at least the in-core copy of the superblock. Native Linux ext2fs does this, but FreeBSD ext2fs doesn't. Thus FreeBSD ext2fs needs something like the userland code so as to support all values of s_rev_level. For other macros, it gets this by depending on Linux's spelling of *KERNEL* being different from FreeBSD's (__KERNEL__ in Linux and _KERNEL in FreeBSD) -- see the first comment. This doesn't quite work here, mainly because the FreeBSD macros didn't take an s parameter. So FreeBSD uses the notyet hack followed by further uglyness. The "fix" made things uglier than before by suppling an s parameter with different semantics so that the Linux macros still don't work. It still needs a third set of macros, and it gets these wrong by not making them either semantically or API-compatible with the above correct ones (it essentially duplicates the __KERNEL__ version of the ones above with a different API, but this gives wrong semantics because FreeBSD doesn't have the extra initialization). % #define EXT2_INODES_PER_BLOCK(s) ((s)->s_inodes_per_block) This is the old hack for accesses to the s_inodes_per_block field. It works even for old file systems because this field isn't in the on-disk superblock. This field is in the in-core superblock and is always initialized from other data in both Linux and FreeBSD. Linux doesn't even have a macro for it and neither should FreeBSD. Note that this macro has a different form than the ones above related to this -- in the above, s is has to be followed through u.ext2_sb to get to the on-disk superblock, while this macro gets data directly from the in-core superblock. % /* Should be sizeof(struct ext2_inode): */ This comment is now nonsense. One thing that the following values should NOT be is a fixed value given by either a literal or sizeof(). % #define EXT2_INODE_SIZE(s) ((s)->s_inode_size) % #define EXT2_FIRST_INO(s) ((s)->s_first_inode) % #endif /* notyet */ The values here used to be given by literals with no s parameter: $ #define EXT2_INODE_SIZE 128 $ #define EXT2_FIRST_INO 11 These were wrong for newer file systems with different values in the superblock. The new definitions are wrong for older file systems with no values in the superblock. Other parts of the commit consisted of adding the s parameter. These are not so bad, except they use an inconsistent API -- elsewhere, the s parameter is consistently a pointer to the in-core superblock, except in FreeBSD's version of these macros where it is a pointer to the on-disk superblock. This API difference is confusing and is the main thing that prevents direct use of the Linux macros (I think there are also some spelling differences like s_es instead of u.ext2_sb). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090518160049.E2212>