On 6/28/17, 5:48 PM, "Darrick J. Wong" <darrick.wong@xxxxxxxxxx> 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 <kkc6196@xxxxxx>
> ---
> 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
