From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751815AbdF2A7K (ORCPT ); Wed, 28 Jun 2017 20:59:10 -0400 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:46492 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751701AbdF2A62 (ORCPT ); Wed, 28 Jun 2017 20:58:28 -0400 From: William Koh To: "Darrick J. Wong" CC: "tytso@mit.edu" , "adilger.kernel@dilger.ca" , "linux-ext4@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Kernel Team Subject: Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. Thread-Topic: [PATCH] fs: ext4: inode->i_generation not assigned 0. Thread-Index: AQHS8Frhf5zD+VA+OkO552aALR7fE6I7Ah6A//+NaAA= Date: Thu, 29 Jun 2017 00:58:17 +0000 Message-ID: <36EC19C1-8C8C-4B95-B263-8684BEDCE591@fb.com> References: <20170629004825.GA5865@birch.djwong.org> In-Reply-To: <20170629004825.GA5865@birch.djwong.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: oracle.com; dkim=none (message not signed) header.d=none;oracle.com; dmarc=none action=none header.from=fb.com; x-originating-ip: [2620:10d:c090:180::1:4632] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;CY4PR15MB1607;20:JKs7RLUAzOOnGghh6FfkJtLPlgRWWCHyZfXb/zvP8vz7dmi6B3PzRuAFWLo8R7+vbQ8MILt+xj+crwu0oeqq7zVyrfz6cUtDVe740H0pqFM+KnDSJ150M+uh6SKqaCT1nz/inlyE3CM/SDbmltY3ke1p8cgYj6dN93+cr9uizQQ= x-ms-office365-filtering-correlation-id: b9e0d7d4-7048-4d10-fb91-08d4be89edc6 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254075)(300000503095)(300135400095)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095);SRVR:CY4PR15MB1607; x-ms-traffictypediagnostic: CY4PR15MB1607: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(236129657087228)(67672495146484)(148574349560750)(146099531331640)(247924648384137); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(5005006)(8121501046)(100000703101)(100105400095)(93006095)(93001095)(10201501046)(3002001)(6041248)(20161123558100)(20161123560025)(20161123555025)(20161123564025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(6072148)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:CY4PR15MB1607;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:CY4PR15MB1607; x-forefront-prvs: 0353563E2B x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(39850400002)(39400400002)(39450400003)(39840400002)(39410400002)(377454003)(43544003)(24454002)(86362001)(189998001)(8676002)(53936002)(5660300001)(7736002)(14454004)(478600001)(102836003)(76176999)(25786009)(50986999)(6506006)(6116002)(36756003)(81166006)(6486002)(99286003)(54356999)(6512007)(77096006)(110136004)(3660700001)(305945005)(33656002)(2906002)(38730400002)(53546010)(229853002)(2900100001)(82746002)(2950100002)(6916009)(83716003)(6246003)(4326008)(3280700002)(6436002)(8936002)(461764006);DIR:OUT;SFP:1102;SCL:1;SRVR:CY4PR15MB1607;H:CY4PR15MB1606.namprd15.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <270361338485F14BB1C0F9F68BEB701B@namprd15.prod.outlook.com> MIME-Version: 1.0 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jun 2017 00:58:17.2187 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR15MB1607 X-OriginatorOrg: fb.com X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-28_15:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v5T0xTq5032129 On 6/28/17, 5:48 PM, "Darrick J. Wong" wrote: On Wed, Jun 28, 2017 at 03:06:42PM -0700, Kyungchan Koh wrote: > In fs/ext4/super.c, the function ext4_nfs_get_inode takes as input > "generation" that can be used to specify the generation of the inode to > be returned. When 0 is given as input, then inodes of any generation can > be returned. Therefore, generation 0 is a special case that should be > avoided when assigning generation to inodes. > > A new inline function, ext4_inode_set_gen, will take care of the > problem. Now, inodes cannot have a generation of 0, so this patch fixes > the issue. Forgive my ignorance, but why is generation == 0 a special case? From a quick scan of the code it seems that filesystems hand out handles to NFS with parent_{ino,gen} set (or zeroed). That implies that we have to check ino/gen for zeroes and garbage, but I don't see why you'd exempt gen == 0 from checking? (Really what I'm fishing for is whether or not there's some precedent for this that I don't know about.) --D > > Signed-off-by: Kyungchan Koh > --- > fs/ext4/ext4.h | 8 ++++++++ > fs/ext4/ialloc.c | 2 +- > fs/ext4/ioctl.c | 4 ++-- > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 3219154..74c6677 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -1549,6 +1549,14 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino) > ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count)); > } > > +static inline void ext4_inode_set_gen(struct inode *inode, > + struct ext4_sb_info *sbi) > +{ > + inode->i_generation = sbi->s_next_generation++; > + if (!inode->i_generation) > + inode->i_generation = sbi->s_next_generation++; > +} > + > /* > * Inode dynamic state flags > */ > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c > index 98ac2f1..d33f6f0 100644 > --- a/fs/ext4/ialloc.c > +++ b/fs/ext4/ialloc.c > @@ -1072,7 +1072,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir, > goto out; > } > spin_lock(&sbi->s_next_gen_lock); > - inode->i_generation = sbi->s_next_generation++; > + ext4_inode_set_gen(inode, sbi); > spin_unlock(&sbi->s_next_gen_lock); > > /* Precompute checksum seed for inode metadata */ > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 0c21e22..d52a467 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -160,8 +160,8 @@ static long swap_inode_boot_loader(struct super_block *sb, > inode->i_ctime = inode_bl->i_ctime = current_time(inode); > > spin_lock(&sbi->s_next_gen_lock); > - inode->i_generation = sbi->s_next_generation++; > - inode_bl->i_generation = sbi->s_next_generation++; > + ext4_inode_set_gen(inode, sbi); > + ext4_inode_set_gen(inode_bl, sbi); > spin_unlock(&sbi->s_next_gen_lock); > > ext4_discard_preallocations(inode); > -- > 2.9.3 > Generation == 0 seems to be a special case for many filesystems, not just ext4. For jfs, in jfs_nfs_get_inode, if the input generation is 0, then no inodes are returned. Such filesystems that seem to treat generation 0 as a special case nfs_get_inode that I have found so far are ext2, ext4, jfs, exofs, and f2fs. Therefore, I was actually thinking about implementing a shared helper in linux/fs.h that has the prototype “static inline void inode_set_gen(struct inode *inode, unsigned int *generation)” that can be used for all filesystems. For example, for jfs, I can do “inode_set_gen(inode, &JFS_SBI(sb)->gengen);” or for extX, I can do “inode_set_gen(inode, &EXTX_SB(sb)->s_next_generation);”. This allows a cleaner change of adding a few lines of code to linux/fs.h and replacing one to a few lines for each filesystem. I am open to both options, if anyone has a strong preference for either option. Best, Kyungchan Koh