From: William Koh <kkc6196@fb.com>
To: Andreas Dilger <adilger@dilger.ca>
CC: "Theodore Ts'o" <tytso@mit.edu>,
        linux-ext4 <linux-ext4@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>, Kernel Team <Kernel-team@fb.com>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Trond Myklebust <trond.myklebust@primarydata.com>
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+OkO552aALR7fE6I7HxsA//+tlgA=
Date: Thu, 29 Jun 2017 04:37:14 +0000
Message-ID: <7CD38230-D961-428F-B2E9-2C0E28CAF442@fb.com>
References: <c9f7090b2a58364998ad4a49cfdac379b905c7e9.1498687302.git.kkc6196@fb.com>
 <20A40B3C-E179-432B-B56F-BDAAF0CD2E1F@dilger.ca>
In-Reply-To: <20A40B3C-E179-432B-B56F-BDAAF0CD2E1F@dilger.ca>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <8A81596E499CC54E8AAC53CE7D405AA9@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jun 2017 04:37:14.2002
 (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-29_03:,,
 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v5T4bhZW031607
Content-Length: 3275
Lines: 89

On 6/28/17, 7:32 PM, "Andreas Dilger" <adilger@dilger.ca> wrote:

    On Jun 28, 2017, at 4:06 PM, Kyungchan Koh <kkc6196@fb.com> 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.
    
    I'd agree with this change to avoid assigning generation == 0 to real inodes.
    
    Also, the separate question arises about whether we need to allow file handle
    lookup with generation == 0?  That allows FID guessing easily, while requiring
    a non-zero generation makes that a lot harder.
    
    What are the cases where generation == 0 are used?

Honestly, I’m not too sure. I just noticed that generation 0 was a special
case from reading the code.

    > 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.
    > 
    > Signed-off-by: Kyungchan Koh <kkc6196@fb.com>
    > 
    > 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)
    
    This should be marked "unlikely()" since it happens at most once every 4B
    file creations (though likely even less since it is unlikely that so many
    files will be created in a single mount).

Got it.
    
    > +		inode->i_generation = sbi->s_next_generation++;
    > +}
    > +
    > 
    > 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	}
    > 	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);
    > 
    > 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,
    > 
    > 	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);
    > 
    
    
    Cheers, Andreas

This is applicable to many fs, including ext2, ext4, exofs, jfs, and f2fs.
Therefore, a shared helper in linux/fs.h will allow for easy changes
in all fs. Is there any reason that might be a bad idea?

Best,
Kyungchan Koh