From owner-freebsd-fs@FreeBSD.ORG Wed Dec 3 22:56:06 2008 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 89CD61065686 for ; Wed, 3 Dec 2008 22:56:06 +0000 (UTC) (envelope-from josh.carroll@gmail.com) Received: from mail-gx0-f19.google.com (mail-gx0-f19.google.com [209.85.217.19]) by mx1.freebsd.org (Postfix) with ESMTP id 1AC368FC16 for ; Wed, 3 Dec 2008 22:56:05 +0000 (UTC) (envelope-from josh.carroll@gmail.com) Received: by gxk12 with SMTP id 12so2646223gxk.19 for ; Wed, 03 Dec 2008 14:56:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:reply-to :to:subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=CKmFJiglEyYkVnUsBUrNi5JrrmvcjWprUKCwjrAMMq4=; b=sKzOGJ3zYns0AE75S+X6ZQsAkvaIYC0rgq6X96iz1fRBrtnsBSGHhMMnVl1QrC6eTD YI6uy0mVntkCG0jQtH8p8yQTwYBimuHEvH5mr3wmbWf++TH/w1Z7q1nQdgk+v3GVLGkY a14JG3U0ZOHw+CJdrrsQruI/w6Qeckn0plkFQ= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:reply-to:to:subject:cc:in-reply-to :mime-version:content-type:content-transfer-encoding :content-disposition:references; b=vWoqPd9IKRLO6hfksmxgTXFJ6nYrzUR4egmvmo/pxhZCGcd+g6k/lmv0+odT/XwzqV EVH7hRm/cyKsEYcmgZ3N+mtTcGWOzW8RYAGlT4bPcPkZ2LP5WyIw+wrXLrIn89G07dtu naxtVZ5C6f8Hjpex34K6BkNqi7OfPtA+B8Tdg= Received: by 10.151.142.2 with SMTP id u2mr26950388ybn.4.1228344823942; Wed, 03 Dec 2008 14:53:43 -0800 (PST) Received: by 10.150.123.1 with HTTP; Wed, 3 Dec 2008 14:53:43 -0800 (PST) Message-ID: <8cb6106e0812031453j6dc2f2f4i374145823c084eaa@mail.gmail.com> Date: Wed, 3 Dec 2008 17:53:43 -0500 From: "Josh Carroll" To: "Kostik Belousov" In-Reply-To: <20081125150342.GL2042@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <8cb6106e0811241129o642dcf28re4ae177c8ccbaa25@mail.gmail.com> <20081125140601.GH2042@deviant.kiev.zoral.com.ua> <8cb6106e0811250617q5fffb41exe20dfb8314fc4a9d@mail.gmail.com> <20081125142827.GI2042@deviant.kiev.zoral.com.ua> <8cb6106e0811250657q6fdf08b0x1e94f35fd0a7ed4f@mail.gmail.com> <20081125150342.GL2042@deviant.kiev.zoral.com.ua> Cc: freebsd-fs@freebsd.org, FreeBSD Stable Subject: Re: ext2 inode size patch - RE: PR kern/124621 X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: josh.carroll@gmail.com List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Dec 2008 22:56:06 -0000 > Ok, I describe my concern once more. I do not object against the checking > of the inode size. But, if inode size is changed, then some data is added > to the inode, that could (and usually does, otherwise why extend it ?) > change intrerpetation of the inode. Thus, we need a verification of the > fact that simply ignoring added fields does not damage filesystem or > cause user data corruption. Verification != testing. Let me take another crack at explaining why I think this patch is not dangerous. The inode size is determined by reading the following member: __u16 s_inode_size; of the ext2_super_block structure. I took a look at the Linux 2.6.27.7 kernel source, and indeed they do something very similar if not the same: #define EXT2_INODE_SIZE(s) (EXT2_SB(s)->s_inode_size) If you compare to what I did: #define EXT2_INODE_SIZE(s) ((s)->u.ext2_sb.s_inode_size) This is really the same thing, since EXT2_SB is defined (in the Linux kernel src as): #ifdef __KERNEL__ #include static inline struct ext2_sb_info *EXT2_SB(struct super_block *sb) { return sb->s_fs_info; } And struct ext2_sb_info has the following member: int s_inode_size; Essentially, the changes I made simply query the existing information from the filesystem, which is what the Linux kernel does as well. The inode size, blocks per group, etc are all defined at filesystem creation time by mke2fs and it ensures the sanity of the relationship between the inodes/blocks/block groups. The first diagram here: http://sunsite.nus.sg/LDP/LDP/tlk/node95.html Makes it clear that as long as the number of inodes per block and the blocks per group is is sane during fs creation, looking up the inode size as my patch does is fine, since the creation of the filesystem is ensures a correct by construction situation. We're simply reading the size of the inode based on the filesystem. I hope this is sufficient to convince some further thought about committing this. For those interested in the relevant Linux kernel source, you can have a look at line 105 of include/linux/ext2_fs.h from the linux-2.6.27.7 kernel source. Thanks, Josh