From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932446AbXGKLud (ORCPT ); Wed, 11 Jul 2007 07:50:33 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1760329AbXGKLuX (ORCPT ); Wed, 11 Jul 2007 07:50:23 -0400 Received: from 74-0-229-162.T1.lbdsl.net ([74.0.229.162]:51226 "EHLO mail.clusterfs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759769AbXGKLuV (ORCPT ); Wed, 11 Jul 2007 07:50:21 -0400 Date: Wed, 11 Jul 2007 05:50:13 -0600 From: Andreas Dilger To: Andrew Morton Cc: cmm@us.ibm.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org, nfsv4@linux-nfs.org Subject: Re: [EXT4 set 4][PATCH 3/5] i_version:ext4 inode version read/store Message-ID: <20070711115013.GS6417@schatzie.adilger.int> Mail-Followup-To: Andrew Morton , cmm@us.ibm.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org, nfsv4@linux-nfs.org References: <1183275456.4010.129.camel@localhost.localdomain> <20070710163117.d14fce90.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20070710163117.d14fce90.akpm@linux-foundation.org> User-Agent: Mutt/1.4.1i X-GPG-Key: 1024D/0D35BED6 X-GPG-Fingerprint: 7A37 5D79 BF1B CECA D44F 8A29 A488 39F5 0D35 BED6 Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Jul 10, 2007 16:31 -0700, Andrew Morton wrote: > > This patch adds 64-bit inode version support to ext4. The lower 32 bits > > are stored in the osd1.linux1.l_i_version field while the high 32 bits > > are stored in the i_version_hi field newly created in the ext4_inode. > > So reading the code here does serve to answer the question I raised against > the earlier patch. A bit. > > I'd have thought that this patch and the one which adds i_version_hi should > be folded into a single diff? It could be - the original patch to reserve i_version_hi was submitted to before the patches were ready to avoid that space being used by something else. > > + if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) { > > + if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi)) > > + inode->i_version |= > > + (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32; > > > > > > > + } > > I don't quite see how the above two tests are sufficient to unambiguously > determine that the i_version_hi field is present on-disk. > > I guess we're implicitly assuming that if the on-disk inode is big enough > then it _must_ have i_version_hi in there? If so, why is the comparison > with EXT4_GOOD_OLD_INODE_SIZE needed? The GOOT_OLD_INODE_SIZE check is needed to know if i_extra_isize is even present or valid in the on-disk inode. > > @@ -2852,8 +2859,14 @@ static int ext4_do_update_inode(handle_t > > + raw_inode->i_disk_version = cpu_to_le32(inode->i_version); > > + if (ei->i_extra_isize) { > > + if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi)) { > > There's no comparison with EXT4_GOOD_OLD_INODE_SIZE here... Because this is the in-memory version and it is always valid (set to zero if there is extra space in the on-disk inode). Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc.