* [PATCH] fs: ext4: inode->i_generation not assigned 0. @ 2017-06-28 22:06 Kyungchan Koh 2017-06-29 0:48 ` Darrick J. Wong 2017-06-29 2:32 ` Andreas Dilger 0 siblings, 2 replies; 22+ messages in thread From: Kyungchan Koh @ 2017-06-28 22:06 UTC (permalink / raw) To: tytso, adilger.kernel, linux-ext4, linux-kernel; +Cc: kernel-team 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. Signed-off-by: Kyungchan Koh <kkc6196@fb.com> --- 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-28 22:06 [PATCH] fs: ext4: inode->i_generation not assigned 0 Kyungchan Koh @ 2017-06-29 0:48 ` Darrick J. Wong 2017-06-29 0:58 ` William Koh 2017-06-29 2:32 ` Andreas Dilger 1 sibling, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2017-06-29 0:48 UTC (permalink / raw) To: Kyungchan Koh Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, kernel-team 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@fb.com> > --- > 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 0:48 ` Darrick J. Wong @ 2017-06-29 0:58 ` William Koh 0 siblings, 0 replies; 22+ messages in thread From: William Koh @ 2017-06-29 0:58 UTC (permalink / raw) To: Darrick J. Wong Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, Kernel Team On 6/28/17, 5:48 PM, "Darrick J. Wong" <darrick.wong@oracle.com> 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@fb.com> > --- > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-28 22:06 [PATCH] fs: ext4: inode->i_generation not assigned 0 Kyungchan Koh 2017-06-29 0:48 ` Darrick J. Wong @ 2017-06-29 2:32 ` Andreas Dilger 2017-06-29 4:37 ` William Koh 1 sibling, 1 reply; 22+ messages in thread From: Andreas Dilger @ 2017-06-29 2:32 UTC (permalink / raw) To: Kyungchan Koh Cc: Theodore Ts'o, linux-ext4, lkml, kernel-team, J. Bruce Fields, linux-fsdevel, Trond Myklebust [-- Attachment #1: Type: text/plain, Size: 2653 bytes --] 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? > 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). > + 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 [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 2:32 ` Andreas Dilger @ 2017-06-29 4:37 ` William Koh 2017-06-29 4:59 ` Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: William Koh @ 2017-06-29 4:37 UTC (permalink / raw) To: Andreas Dilger Cc: Theodore Ts'o, linux-ext4, lkml, Kernel Team, J. Bruce Fields, linux-fsdevel, Trond Myklebust 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 4:37 ` William Koh @ 2017-06-29 4:59 ` Darrick J. Wong 2017-06-29 14:28 ` William Koh 2017-06-29 14:35 ` J. Bruce Fields 0 siblings, 2 replies; 22+ messages in thread From: Darrick J. Wong @ 2017-06-29 4:59 UTC (permalink / raw) To: William Koh Cc: Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, J. Bruce Fields, linux-fsdevel, Trond Myklebust, xfs [add linux-xfs to cc] On Thu, Jun 29, 2017 at 04:37:14AM +0000, William Koh wrote: > 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? AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other number. There is no special casing of zero in either filesystem. So now, my curiosity intrigued, I surveyed all the Linux filesystems that can export to NFS. I see that there are actually quite a few fs (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf, ufs) that treat zero as a special value meaning "ignore generation check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero special and always require a match; and still others (affs, befs, ceph, gfs2, jffs2, squashfs) that don't check at all. That to mean strongly suggests that more research is necessary to figure out why some of the filesystems that support i_generation reserve zero as a special value to disable generation checks and why others always require an exact match. Until we can recapture why things are they way they are, it doesn't make much sense to have a helper that only applies to half the filesystems. Granted, the contents of a file handle are generally left up to the individual filesystem, and the behaviors are very different, so I also don't see that much value in hoisting i_generation updates to the VFS level. I guess it wouldn't really matter if XFS stopped writing i_generation = 0 onto disk, but I'm too curious about this odd difference in behavior to let it go just yet. :) --D > > Best, > Kyungchan Koh > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 4:59 ` Darrick J. Wong @ 2017-06-29 14:28 ` William Koh 2017-06-29 14:35 ` J. Bruce Fields 1 sibling, 0 replies; 22+ messages in thread From: William Koh @ 2017-06-29 14:28 UTC (permalink / raw) To: Darrick J. Wong Cc: Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, J. Bruce Fields, linux-fsdevel, Trond Myklebust, xfs On 6/28/17, 9:59 PM, "Darrick J. Wong" <darrick.wong@oracle.com> wrote: [add linux-xfs to cc] On Thu, Jun 29, 2017 at 04:37:14AM +0000, William Koh wrote: > 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? AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other number. There is no special casing of zero in either filesystem. So now, my curiosity intrigued, I surveyed all the Linux filesystems that can export to NFS. I see that there are actually quite a few fs (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf, ufs) that treat zero as a special value meaning "ignore generation check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero special and always require a match; and still others (affs, befs, ceph, gfs2, jffs2, squashfs) that don't check at all. That to mean strongly suggests that more research is necessary to figure out why some of the filesystems that support i_generation reserve zero as a special value to disable generation checks and why others always require an exact match. Until we can recapture why things are they way they are, it doesn't make much sense to have a helper that only applies to half the filesystems. Granted, the contents of a file handle are generally left up to the individual filesystem, and the behaviors are very different, so I also don't see that much value in hoisting i_generation updates to the VFS level. I guess it wouldn't really matter if XFS stopped writing i_generation = 0 onto disk, but I'm too curious about this odd difference in behavior to let it go just yet. :) --D That makes sense. I’ll try to also look into this matter and send a newer patch with the most optimal fix to this issue. -Kyungchan Koh > > Best, > Kyungchan Koh > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 4:59 ` Darrick J. Wong 2017-06-29 14:28 ` William Koh @ 2017-06-29 14:35 ` J. Bruce Fields 2017-06-29 17:25 ` Darrick J. Wong 1 sibling, 1 reply; 22+ messages in thread From: J. Bruce Fields @ 2017-06-29 14:35 UTC (permalink / raw) To: Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Wed, Jun 28, 2017 at 09:59:40PM -0700, Darrick J. Wong wrote: > AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other > number. There is no special casing of zero in either filesystem. > > So now, my curiosity intrigued, I surveyed all the Linux filesystems > that can export to NFS. I see that there are actually quite a few fs > (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf, > ufs) that treat zero as a special value meaning "ignore generation > check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero > special and always require a match; and still others (affs, befs, ceph, > gfs2, jffs2, squashfs) that don't check at all. > > That to mean strongly suggests that more research is necessary to figure > out why some of the filesystems that support i_generation reserve zero > as a special value to disable generation checks and why others always > require an exact match. Until we can recapture why things are they way > they are, it doesn't make much sense to have a helper that only applies > to half the filesystems. >From a quick look at ext2; correct me if I got anything wrong: - it looks like this is *only* used by NFS fh->inode lookups, there's not some other internal use for this special case. - filehandles are never encoded with i_generation 0, so will never be returned to clients. So, this could only ever be used by an NFS client. But the only NFS client that could ever use it would be a non-standard client that knew this special feature of these particular filesystems. Sounds like at most a slightly useful tool for malicious clients attempting filehandle-guessing attacks. I'm probably missing something. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 14:35 ` J. Bruce Fields @ 2017-06-29 17:25 ` Darrick J. Wong 2017-06-29 18:30 ` J. Bruce Fields 0 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2017-06-29 17:25 UTC (permalink / raw) To: J. Bruce Fields Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Thu, Jun 29, 2017 at 10:35:51AM -0400, J. Bruce Fields wrote: > On Wed, Jun 28, 2017 at 09:59:40PM -0700, Darrick J. Wong wrote: > > AFAICT, i_generation == 0 in XFS and btrfs is just as valid as any other > > number. There is no special casing of zero in either filesystem. > > > > So now, my curiosity intrigued, I surveyed all the Linux filesystems > > that can export to NFS. I see that there are actually quite a few fs > > (ext[2-4], exofs, efs, fat, jfs, f2fs, isofs, nilfs2, reiserfs, udf, > > ufs) that treat zero as a special value meaning "ignore generation > > check"; others (xfs, btrfs, fuse, ntfs, ocfs2) that don't consider zero > > special and always require a match; and still others (affs, befs, ceph, > > gfs2, jffs2, squashfs) that don't check at all. > > > > That to mean strongly suggests that more research is necessary to figure > > out why some of the filesystems that support i_generation reserve zero > > as a special value to disable generation checks and why others always > > require an exact match. Until we can recapture why things are they way > > they are, it doesn't make much sense to have a helper that only applies > > to half the filesystems. > > From a quick look at ext2; correct me if I got anything wrong: > > - it looks like this is *only* used by NFS fh->inode lookups, > there's not some other internal use for this special case. > - filehandles are never encoded with i_generation 0, so will > never be returned to clients. > > So, this could only ever be used by an NFS client. But the only NFS > client that could ever use it would be a non-standard client that knew > this special feature of these particular filesystems. > > Sounds like at most a slightly useful tool for malicious clients > attempting filehandle-guessing attacks. > > I'm probably missing something. Was there ever a version of NFS (or more generally callers of the exportfs code) that couldn't deal with i_generation in the file handle, and therefore we invented this generation hack to work around the loss of the generation information? There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes with subtree_check (which seems to require one ino/gen pair for the file and a second pair for the file's parent) on NFSv2 because v2 doesn't provide enough space for all the file handle information, but that's the furthest I got with lazy-mining the git history. :) --D > > --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 17:25 ` Darrick J. Wong @ 2017-06-29 18:30 ` J. Bruce Fields 2017-06-29 18:50 ` J. Bruce Fields 0 siblings, 1 reply; 22+ messages in thread From: J. Bruce Fields @ 2017-06-29 18:30 UTC (permalink / raw) To: Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > Was there ever a version of NFS (or more generally callers of the > exportfs code) that couldn't deal with i_generation in the file handle, > and therefore we invented this generation hack to work around the loss > of the generation information? > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > with subtree_check (which seems to require one ino/gen pair for the file > and a second pair for the file's parent) on NFSv2 because v2 doesn't > provide enough space for all the file handle information, but that's the > furthest I got with lazy-mining the git history. :) There's a comment in fs/ext4/super.c:ext4_nfs_get_inode * Currently we don't know the generation for parent directory, so * a generation of 0 means "accept any" But I don't see that used. It was used once upon a time; I see it actually used in old 2.5 code in nfsd_get_dentry. Hm. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 18:30 ` J. Bruce Fields @ 2017-06-29 18:50 ` J. Bruce Fields 2017-07-04 4:04 ` Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: J. Bruce Fields @ 2017-06-29 18:50 UTC (permalink / raw) To: Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > > Was there ever a version of NFS (or more generally callers of the > > exportfs code) that couldn't deal with i_generation in the file handle, > > and therefore we invented this generation hack to work around the loss > > of the generation information? > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > > with subtree_check (which seems to require one ino/gen pair for the file > > and a second pair for the file's parent) on NFSv2 because v2 doesn't > > provide enough space for all the file handle information, but that's the > > furthest I got with lazy-mining the git history. :) > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode > > * Currently we don't know the generation for parent directory, so > * a generation of 0 means "accept any" > > But I don't see that used. > > It was used once upon a time; I see it actually used in old 2.5 code in > nfsd_get_dentry. Hm. Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: switch (fh_type) { case FILEID_INO32_GEN_PARENT: inode = get_inode(sb, fid->i32.parent_ino, (fh_len > 3 ? fid->i32.parent_gen : 0)); break; } I'm not sure under what conditions that filehandle encoding is used. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-06-29 18:50 ` J. Bruce Fields @ 2017-07-04 4:04 ` Darrick J. Wong 2017-07-05 1:15 ` J. Bruce Fields 0 siblings, 1 reply; 22+ messages in thread From: Darrick J. Wong @ 2017-07-04 4:04 UTC (permalink / raw) To: J. Bruce Fields Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Thu, Jun 29, 2017 at 02:50:22PM -0400, J. Bruce Fields wrote: > On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: > > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > > > Was there ever a version of NFS (or more generally callers of the > > > exportfs code) that couldn't deal with i_generation in the file handle, > > > and therefore we invented this generation hack to work around the loss > > > of the generation information? > > > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > > > with subtree_check (which seems to require one ino/gen pair for the file > > > and a second pair for the file's parent) on NFSv2 because v2 doesn't > > > provide enough space for all the file handle information, but that's the > > > furthest I got with lazy-mining the git history. :) > > > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode > > > > * Currently we don't know the generation for parent directory, so > > * a generation of 0 means "accept any" > > > > But I don't see that used. > > > > It was used once upon a time; I see it actually used in old 2.5 code in > > nfsd_get_dentry. Hm. > > Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: > > switch (fh_type) { > case FILEID_INO32_GEN_PARENT: > inode = get_inode(sb, fid->i32.parent_ino, > (fh_len > 3 ? fid->i32.parent_gen : 0)); > break; > } > > I'm not sure under what conditions that filehandle encoding is used. The best guess I can come up with is the old nfs_fhbase_old style handles, which (afaict) do not carry parent i_generation? --D > > --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-04 4:04 ` Darrick J. Wong @ 2017-07-05 1:15 ` J. Bruce Fields 2017-07-05 19:19 ` Darrick J. Wong 0 siblings, 1 reply; 22+ messages in thread From: J. Bruce Fields @ 2017-07-05 1:15 UTC (permalink / raw) To: Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Mon, Jul 03, 2017 at 09:04:46PM -0700, Darrick J. Wong wrote: > On Thu, Jun 29, 2017 at 02:50:22PM -0400, J. Bruce Fields wrote: > > On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: > > > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > > > > Was there ever a version of NFS (or more generally callers of the > > > > exportfs code) that couldn't deal with i_generation in the file handle, > > > > and therefore we invented this generation hack to work around the loss > > > > of the generation information? > > > > > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > > > > with subtree_check (which seems to require one ino/gen pair for the file > > > > and a second pair for the file's parent) on NFSv2 because v2 doesn't > > > > provide enough space for all the file handle information, but that's the > > > > furthest I got with lazy-mining the git history. :) > > > > > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode > > > > > > * Currently we don't know the generation for parent directory, so > > > * a generation of 0 means "accept any" > > > > > > But I don't see that used. > > > > > > It was used once upon a time; I see it actually used in old 2.5 code in > > > nfsd_get_dentry. Hm. > > > > Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: > > > > switch (fh_type) { > > case FILEID_INO32_GEN_PARENT: > > inode = get_inode(sb, fid->i32.parent_ino, > > (fh_len > 3 ? fid->i32.parent_gen : 0)); > > break; > > } > > > > I'm not sure under what conditions that filehandle encoding is used. > > The best guess I can come up with is the old nfs_fhbase_old style handles, > which (afaict) do not carry parent i_generation? Yeah, I just couldn't tell in the time I looked whether they could still be handed out. If not, then the only way they'd still be used is if a client had a server continually mounted while the server was upgraded from a kernel that still handed out the old filehandle. So if they haven't been given out for long enough it's possible nobody would notice if we dropped support. But, I didn't get far enough to figure that out. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-05 1:15 ` J. Bruce Fields @ 2017-07-05 19:19 ` Darrick J. Wong 2017-07-05 20:27 ` Theodore Ts'o 2017-07-05 20:49 ` J. Bruce Fields 0 siblings, 2 replies; 22+ messages in thread From: Darrick J. Wong @ 2017-07-05 19:19 UTC (permalink / raw) To: J. Bruce Fields Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Tue, Jul 04, 2017 at 09:15:34PM -0400, J. Bruce Fields wrote: > On Mon, Jul 03, 2017 at 09:04:46PM -0700, Darrick J. Wong wrote: > > On Thu, Jun 29, 2017 at 02:50:22PM -0400, J. Bruce Fields wrote: > > > On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: > > > > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > > > > > Was there ever a version of NFS (or more generally callers of the > > > > > exportfs code) that couldn't deal with i_generation in the file handle, > > > > > and therefore we invented this generation hack to work around the loss > > > > > of the generation information? > > > > > > > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > > > > > with subtree_check (which seems to require one ino/gen pair for the file > > > > > and a second pair for the file's parent) on NFSv2 because v2 doesn't > > > > > provide enough space for all the file handle information, but that's the > > > > > furthest I got with lazy-mining the git history. :) > > > > > > > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode > > > > > > > > * Currently we don't know the generation for parent directory, so > > > > * a generation of 0 means "accept any" > > > > > > > > But I don't see that used. > > > > > > > > It was used once upon a time; I see it actually used in old 2.5 code in > > > > nfsd_get_dentry. Hm. > > > > > > Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: > > > > > > switch (fh_type) { > > > case FILEID_INO32_GEN_PARENT: > > > inode = get_inode(sb, fid->i32.parent_ino, > > > (fh_len > 3 ? fid->i32.parent_gen : 0)); > > > break; > > > } > > > > > > I'm not sure under what conditions that filehandle encoding is used. > > > > The best guess I can come up with is the old nfs_fhbase_old style handles, > > which (afaict) do not carry parent i_generation? > > Yeah, I just couldn't tell in the time I looked whether they could still > be handed out. > > If not, then the only way they'd still be used is if a client had a > server continually mounted while the server was upgraded from a kernel > that still handed out the old filehandle. > > So if they haven't been given out for long enough it's possible nobody > would notice if we dropped support. > > But, I didn't get far enough to figure that out. Hmm, so looking back through prehistory, Linux prior to 2.3.51 (11 March 2000) gave out the old dentry style fhandles. After that, the kernel only gave out the new style handles that we still use today. In 2.4.6 (4 July 2001) the behavior was modified again to chain handle types, i.e. if the client passed in an old style handle then it would get another old style handle back. The changelog for -pre9 says that this was done for compatibility reasons. So, what's the probability that there are clients out there that started talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 kernel seventeen years later? (Do nfs handles persist across client restarts/remounts?) --D > > --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-05 19:19 ` Darrick J. Wong @ 2017-07-05 20:27 ` Theodore Ts'o 2017-07-07 10:51 ` Jeff Layton 2017-07-05 20:49 ` J. Bruce Fields 1 sibling, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2017-07-05 20:27 UTC (permalink / raw) To: Darrick J. Wong Cc: J. Bruce Fields, William Koh, Andreas Dilger, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Wed, Jul 05, 2017 at 12:19:33PM -0700, Darrick J. Wong wrote: > > So, what's the probability that there are clients out there that started > talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 > kernel seventeen years later? (Do nfs handles persist across client > restarts/remounts?) It's whether or not nfs handles persist across server restarts which would be the more interesting question. So if you had a NAS box that was using a Linux 2.2 kernel, and you had clients access the box, and then that box gets upgraded to use a 4.13 kernel, what happens? In the ideal world, the client wouldn't notice, and its 2.2-based file handles that it obtained while the 2.2 kernel was running would continue to work after the box came back up running the new 4.13 kernel. To be honest, I'm not sure I care that much, but I don't use NFS much if at all these days myself. And in reality, what's the chance that an NAS box vendor would continue to support a box that is 17 years old and provide an upgrade for it? (OK, everyone can stop laughing now. :-) - Ted ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-05 20:27 ` Theodore Ts'o @ 2017-07-07 10:51 ` Jeff Layton 2017-07-07 15:51 ` Theodore Ts'o 0 siblings, 1 reply; 22+ messages in thread From: Jeff Layton @ 2017-07-07 10:51 UTC (permalink / raw) To: Theodore Ts'o, Darrick J. Wong Cc: J. Bruce Fields, William Koh, Andreas Dilger, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Wed, 2017-07-05 at 16:27 -0400, Theodore Ts'o wrote: > On Wed, Jul 05, 2017 at 12:19:33PM -0700, Darrick J. Wong wrote: > > > > So, what's the probability that there are clients out there that started > > talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 > > kernel seventeen years later? (Do nfs handles persist across client > > restarts/remounts?) > > It's whether or not nfs handles persist across server restarts which > would be the more interesting question. So if you had a NAS box that > was using a Linux 2.2 kernel, and you had clients access the box, and > then that box gets upgraded to use a 4.13 kernel, what happens? > > In the ideal world, the client wouldn't notice, and its 2.2-based file > handles that it obtained while the 2.2 kernel was running would > continue to work after the box came back up running the new 4.13 > kernel. > Right. That's the case today if we don't remove support for old filehandles. If we were to remove them, the clients would get back -ESTALE there if they tried to use the old 2.2-style fh's that they saw before the upgrade. The main takeaway here is that NFS filehandle lifetime is really only bounded by the boot time of the oldest clients. > To be honest, I'm not sure I care that much, but I don't use NFS much > if at all these days myself. And in reality, what's the chance that > an NAS box vendor would continue to support a box that is 17 years old > and provide an upgrade for it? (OK, everyone can stop laughing now. :-) Agreed. I think we're safe enough to remove this support. If we were really concerned about it, we could move support for it under a Kconfig option that defaults to "off" for now, and then we could plan to just drop it in a couple of years. Vendors that thought they might need it could enable it and speak up with their use case if they wanted to keep it in for future releases. Most likely though, no-one will care. -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-07 10:51 ` Jeff Layton @ 2017-07-07 15:51 ` Theodore Ts'o 2017-07-07 16:13 ` Jeff Layton 0 siblings, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2017-07-07 15:51 UTC (permalink / raw) To: Jeff Layton Cc: Darrick J. Wong, J. Bruce Fields, William Koh, Andreas Dilger, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Fri, Jul 07, 2017 at 06:51:37AM -0400, Jeff Layton wrote: > > Right. That's the case today if we don't remove support for old > filehandles. If we were to remove them, the clients would get back > -ESTALE there if they tried to use the old 2.2-style fh's that they saw > before the upgrade. > > The main takeaway here is that NFS filehandle lifetime is really only > bounded by the boot time of the oldest clients. Well, and how long an NFS server is still up. So one could construct a use case where a (hypothetical) system administrator had a RHEL 7.0 system with a 2.2.16-22 kernel, and they try to update it to a (hypothetical) RHEL 10 kernel in one fell swoop with a 4.13+ kernel that no longer supports the 2-2-style fh's. A client that had the server mounted when it was running the 2.2 kernel might only be up for a few hours, before the upgrade to RHEL 10 happened, and then the client would get ESTALE errors. Of course, I've stopped carrying about enterprise kernel support a long time ago, so I just think that scenario is funny. I recognize that folks who work at Red Hat have to worry about such things --- and I'm sorry. :-) In reality a server installed with RHEL 7.0 has probably died of old age by now --- unless someone crazy is running it in a VMware VM because they had some enterprise software package or some bar-code printing module for which they don't have source code[1], and so they are stuck on RHEL 7.0, even in 2017. Have I mentioned I'm so glad I don't have to worry these sorts of things any more? - Ted [1] That wasn't a made up example; I once visited a customer on site, back in the day, that had that exact problem, and so they were stuck on some antique version of RHEL, and they expected me to help them. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-07 15:51 ` Theodore Ts'o @ 2017-07-07 16:13 ` Jeff Layton 2017-07-07 16:47 ` J. Bruce Fields 0 siblings, 1 reply; 22+ messages in thread From: Jeff Layton @ 2017-07-07 16:13 UTC (permalink / raw) To: Theodore Ts'o Cc: Darrick J. Wong, J. Bruce Fields, William Koh, Andreas Dilger, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Fri, 2017-07-07 at 11:51 -0400, Theodore Ts'o wrote: > On Fri, Jul 07, 2017 at 06:51:37AM -0400, Jeff Layton wrote: > > > > Right. That's the case today if we don't remove support for old > > filehandles. If we were to remove them, the clients would get back > > -ESTALE there if they tried to use the old 2.2-style fh's that they saw > > before the upgrade. > > > > The main takeaway here is that NFS filehandle lifetime is really only > > bounded by the boot time of the oldest clients. > > Well, and how long an NFS server is still up. So one could construct > a use case where a (hypothetical) system administrator had a RHEL 7.0 > system with a 2.2.16-22 kernel, and they try to update it to a > (hypothetical) RHEL 10 kernel in one fell swoop with a 4.13+ kernel > that no longer supports the 2-2-style fh's. A client that had the > server mounted when it was running the 2.2 kernel might only be up for > a few hours, before the upgrade to RHEL 10 happened, and then the > client would get ESTALE errors. > > Of course, I've stopped carrying about enterprise kernel support a > long time ago, so I just think that scenario is funny. I recognize > that folks who work at Red Hat have to worry about such things --- and > I'm sorry. :-) > > In reality a server installed with RHEL 7.0 has probably died of old > age by now --- unless someone crazy is running it in a VMware VM > because they had some enterprise software package or some bar-code > printing module for which they don't have source code[1], and so they are > stuck on RHEL 7.0, even in 2017. Have I mentioned I'm so glad I don't > have to worry these sorts of things any more? > > - Ted > > [1] That wasn't a made up example; I once visited a customer on site, > back in the day, that had that exact problem, and so they were stuck > on some antique version of RHEL, and they expected me to help them. Yep, exactly. An abrupt upgrade like that is always a possibility, but it's pretty unlikely, and I don't have a ton of sympathy for anyone who does that. I guess another thing we could do as an interim step is to add a scary-looking printk that fires when someone sends us one of these really old-style filehandles. That won't help the poor bastard who updates the host directly from v2.2-era kernel to the version that eventually removes support for the old filehandles. It might give us an idea of whether there are still clients in the field that are still using them though. -- Jeff Layton <jlayton@redhat.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-07 16:13 ` Jeff Layton @ 2017-07-07 16:47 ` J. Bruce Fields 0 siblings, 0 replies; 22+ messages in thread From: J. Bruce Fields @ 2017-07-07 16:47 UTC (permalink / raw) To: Jeff Layton Cc: Theodore Ts'o, Darrick J. Wong, William Koh, Andreas Dilger, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Fri, Jul 07, 2017 at 12:13:36PM -0400, Jeff Layton wrote: > On Fri, 2017-07-07 at 11:51 -0400, Theodore Ts'o wrote: > > On Fri, Jul 07, 2017 at 06:51:37AM -0400, Jeff Layton wrote: > > > > > > Right. That's the case today if we don't remove support for old > > > filehandles. If we were to remove them, the clients would get back > > > -ESTALE there if they tried to use the old 2.2-style fh's that they saw > > > before the upgrade. > > > > > > The main takeaway here is that NFS filehandle lifetime is really only > > > bounded by the boot time of the oldest clients. > > > > Well, and how long an NFS server is still up. So one could construct > > a use case where a (hypothetical) system administrator had a RHEL 7.0 > > system with a 2.2.16-22 kernel, and they try to update it to a > > (hypothetical) RHEL 10 kernel in one fell swoop with a 4.13+ kernel > > that no longer supports the 2-2-style fh's. A client that had the > > server mounted when it was running the 2.2 kernel might only be up for > > a few hours, before the upgrade to RHEL 10 happened, and then the > > client would get ESTALE errors. > > > > Of course, I've stopped carrying about enterprise kernel support a > > long time ago, so I just think that scenario is funny. I recognize > > that folks who work at Red Hat have to worry about such things --- and > > I'm sorry. :-) > > > > In reality a server installed with RHEL 7.0 has probably died of old > > age by now --- unless someone crazy is running it in a VMware VM > > because they had some enterprise software package or some bar-code > > printing module for which they don't have source code[1], and so they are > > stuck on RHEL 7.0, even in 2017. Have I mentioned I'm so glad I don't > > have to worry these sorts of things any more? RHEL 7 is current, I think you mean the 17-year-old Red Hat Linux 7. Anyone that far back is on their own as far as any enterprise distro is concerned. There are some exceptions to the "lifetime of a mount" rule, none real issues, I think: - fscache may keep fh's around across client boots, but I suspect you just lose the benefit of the cache until it expires data keyed under old filehandles and repopulates the cache with new ones. - Does the client actually depend on stable filehandles across client reboots if it might cache write data under a persistent delegation? But seeing as we don't even implement persistent delegations, this is a non-issue. - nontraditional NFS clients could do any random thing. NFS is just a protocol, we have no idea how some weird application that talks NFS directly to the server might use filehandles. But this is purely hypothetical, I don't know of one. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-05 19:19 ` Darrick J. Wong 2017-07-05 20:27 ` Theodore Ts'o @ 2017-07-05 20:49 ` J. Bruce Fields 2017-07-06 1:08 ` NeilBrown 1 sibling, 1 reply; 22+ messages in thread From: J. Bruce Fields @ 2017-07-05 20:49 UTC (permalink / raw) To: Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs, neilb On Wed, Jul 05, 2017 at 12:19:33PM -0700, Darrick J. Wong wrote: > On Tue, Jul 04, 2017 at 09:15:34PM -0400, J. Bruce Fields wrote: > > On Mon, Jul 03, 2017 at 09:04:46PM -0700, Darrick J. Wong wrote: > > > On Thu, Jun 29, 2017 at 02:50:22PM -0400, J. Bruce Fields wrote: > > > > On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: > > > > > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: > > > > > > Was there ever a version of NFS (or more generally callers of the > > > > > > exportfs code) that couldn't deal with i_generation in the file handle, > > > > > > and therefore we invented this generation hack to work around the loss > > > > > > of the generation information? > > > > > > > > > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes > > > > > > with subtree_check (which seems to require one ino/gen pair for the file > > > > > > and a second pair for the file's parent) on NFSv2 because v2 doesn't > > > > > > provide enough space for all the file handle information, but that's the > > > > > > furthest I got with lazy-mining the git history. :) > > > > > > > > > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode > > > > > > > > > > * Currently we don't know the generation for parent directory, so > > > > > * a generation of 0 means "accept any" > > > > > > > > > > But I don't see that used. > > > > > > > > > > It was used once upon a time; I see it actually used in old 2.5 code in > > > > > nfsd_get_dentry. Hm. > > > > > > > > Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: > > > > > > > > switch (fh_type) { > > > > case FILEID_INO32_GEN_PARENT: > > > > inode = get_inode(sb, fid->i32.parent_ino, > > > > (fh_len > 3 ? fid->i32.parent_gen : 0)); > > > > break; > > > > } > > > > > > > > I'm not sure under what conditions that filehandle encoding is used. > > > > > > The best guess I can come up with is the old nfs_fhbase_old style handles, > > > which (afaict) do not carry parent i_generation? > > > > Yeah, I just couldn't tell in the time I looked whether they could still > > be handed out. > > > > If not, then the only way they'd still be used is if a client had a > > server continually mounted while the server was upgraded from a kernel > > that still handed out the old filehandle. > > > > So if they haven't been given out for long enough it's possible nobody > > would notice if we dropped support. > > > > But, I didn't get far enough to figure that out. > > Hmm, so looking back through prehistory, Linux prior to 2.3.51 (11 March > 2000) gave out the old dentry style fhandles. After that, the kernel > only gave out the new style handles that we still use today. In 2.4.6 > (4 July 2001) the behavior was modified again to chain handle types, > i.e. if the client passed in an old style handle then it would get > another old style handle back. The changelog for -pre9 says that this > was done for compatibility reasons. Yeah, you're supposed to be able to reboot your NFS server for a kernel upgrade without your client applications experiencing anything worse than a temporary hang while you wait for the server to come back up. So, changing the filehandle format and returning ESTALE to everyone would be unpopular. > So, what's the probability that there are clients out there that started > talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 > kernel seventeen years later? I think it's unlikely enough that we could drop that code; cc'ing Neil in case we overlooked anything. > (Do nfs handles persist across client restarts/remounts?) No. (Well, with maybe a couple exceptions (fscache and persistent NFSv4 delegations) but neither seem relevant here.) --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-05 20:49 ` J. Bruce Fields @ 2017-07-06 1:08 ` NeilBrown 2017-07-06 2:39 ` J. Bruce Fields 0 siblings, 1 reply; 22+ messages in thread From: NeilBrown @ 2017-07-06 1:08 UTC (permalink / raw) To: J. Bruce Fields, Darrick J. Wong Cc: William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs [-- Attachment #1: Type: text/plain, Size: 5270 bytes --] On Wed, Jul 05 2017, J. Bruce Fields wrote: > On Wed, Jul 05, 2017 at 12:19:33PM -0700, Darrick J. Wong wrote: >> On Tue, Jul 04, 2017 at 09:15:34PM -0400, J. Bruce Fields wrote: >> > On Mon, Jul 03, 2017 at 09:04:46PM -0700, Darrick J. Wong wrote: >> > > On Thu, Jun 29, 2017 at 02:50:22PM -0400, J. Bruce Fields wrote: >> > > > On Thu, Jun 29, 2017 at 02:30:53PM -0400, J. Bruce Fields wrote: >> > > > > On Thu, Jun 29, 2017 at 10:25:28AM -0700, Darrick J. Wong wrote: >> > > > > > Was there ever a version of NFS (or more generally callers of the >> > > > > > exportfs code) that couldn't deal with i_generation in the file handle, >> > > > > > and therefore we invented this generation hack to work around the loss >> > > > > > of the generation information? >> > > > > > >> > > > > > There's a comment in xfs_fs_encode_fh about not supporting 64bit inodes >> > > > > > with subtree_check (which seems to require one ino/gen pair for the file >> > > > > > and a second pair for the file's parent) on NFSv2 because v2 doesn't >> > > > > > provide enough space for all the file handle information, but that's the >> > > > > > furthest I got with lazy-mining the git history. :) >> > > > > >> > > > > There's a comment in fs/ext4/super.c:ext4_nfs_get_inode >> > > > > >> > > > > * Currently we don't know the generation for parent directory, so >> > > > > * a generation of 0 means "accept any" >> > > > > >> > > > > But I don't see that used. >> > > > > >> > > > > It was used once upon a time; I see it actually used in old 2.5 code in >> > > > > nfsd_get_dentry. Hm. >> > > > >> > > > Oh, maybe it's here in fs/libfs.c:generic_fh_to_parent: >> > > > >> > > > switch (fh_type) { >> > > > case FILEID_INO32_GEN_PARENT: >> > > > inode = get_inode(sb, fid->i32.parent_ino, >> > > > (fh_len > 3 ? fid->i32.parent_gen : 0)); >> > > > break; >> > > > } >> > > > >> > > > I'm not sure under what conditions that filehandle encoding is used. >> > > >> > > The best guess I can come up with is the old nfs_fhbase_old style handles, >> > > which (afaict) do not carry parent i_generation? >> > >> > Yeah, I just couldn't tell in the time I looked whether they could still >> > be handed out. >> > >> > If not, then the only way they'd still be used is if a client had a >> > server continually mounted while the server was upgraded from a kernel >> > that still handed out the old filehandle. >> > >> > So if they haven't been given out for long enough it's possible nobody >> > would notice if we dropped support. >> > >> > But, I didn't get far enough to figure that out. >> >> Hmm, so looking back through prehistory, Linux prior to 2.3.51 (11 March >> 2000) gave out the old dentry style fhandles. After that, the kernel >> only gave out the new style handles that we still use today. In 2.4.6 >> (4 July 2001) the behavior was modified again to chain handle types, >> i.e. if the client passed in an old style handle then it would get >> another old style handle back. The changelog for -pre9 says that this >> was done for compatibility reasons. > > Yeah, you're supposed to be able to reboot your NFS server for a kernel > upgrade without your client applications experiencing anything worse > than a temporary hang while you wait for the server to come back up. > So, changing the filehandle format and returning ESTALE to everyone > would be unpopular. > >> So, what's the probability that there are clients out there that started >> talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 >> kernel seventeen years later? > > I think it's unlikely enough that we could drop that code; cc'ing Neil > in case we overlooked anything. While I remain a fan of maintaining forward/backward compatibility as much as possible, 15 years is probably more than I can realistically hope for. As you say, a generation number of '0' is only special when old-style file handles are used, with the "subtree_check" export option. They are unlikely to have been used recently. However, I note that include/linux/exportfs.h says: /* * 32bit inode number, 32 bit generation number, * 32 bit parent directory inode number. */ FILEID_INO32_GEN_PARENT = 2, This could be seen as misleading. Some code that reports that fid_type includes the directory generation number. Some other code (cephfs, squashfs) doesn't even include the generation number for the inode (which is OK for squashfs as it is write-only). I could find no code that matches this documentation. I was never a fan of having generic fid_types. Except for 0 and 255, these numbers are generated and interpreted by individual filesystems, and there is no reason that they should agree on the interpretation (and as we see here, they don't). But for the main point of your question: I see no problem with removing nfs_fhbase_old and related code, and that includes the special handling of generation number zero. Thanks, NeilBrown > >> (Do nfs handles persist across client restarts/remounts?) > > No. > > (Well, with maybe a couple exceptions (fscache and persistent NFSv4 > delegations) but neither seem relevant here.) > > --b. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] fs: ext4: inode->i_generation not assigned 0. 2017-07-06 1:08 ` NeilBrown @ 2017-07-06 2:39 ` J. Bruce Fields 0 siblings, 0 replies; 22+ messages in thread From: J. Bruce Fields @ 2017-07-06 2:39 UTC (permalink / raw) To: NeilBrown Cc: Darrick J. Wong, William Koh, Andreas Dilger, Theodore Ts'o, linux-ext4, lkml, Kernel Team, linux-fsdevel, Trond Myklebust, xfs On Thu, Jul 06, 2017 at 11:08:27AM +1000, NeilBrown wrote: > On Wed, Jul 05 2017, J. Bruce Fields wrote: > > > On Wed, Jul 05, 2017 at 12:19:33PM -0700, Darrick J. Wong wrote: > >> So, what's the probability that there are clients out there that started > >> talking to a 2.2-based knfsd and will now want to talk to a modern 4.13 > >> kernel seventeen years later? > > > > I think it's unlikely enough that we could drop that code; Wow, that was a terrible sentence. What I meant was: I think it's unlikely that such a client exists, therefore I'm OK with dropping that code. Anyway: > cc'ing Neil > > in case we overlooked anything. > > While I remain a fan of maintaining forward/backward compatibility as > much as possible, 15 years is probably more than I can realistically > hope for. > As you say, a generation number of '0' is only special when old-style > file handles are used, with the "subtree_check" export option. They are > unlikely to have been used recently. ... > But for the main point of your question: I see no problem with removing > nfs_fhbase_old and related code, and that includes the special handling > of generation number zero. So, we agree, OK. I dunno if this is actually urgent. But it'd be nice to clean out. --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-07-07 16:47 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-28 22:06 [PATCH] fs: ext4: inode->i_generation not assigned 0 Kyungchan Koh 2017-06-29 0:48 ` Darrick J. Wong 2017-06-29 0:58 ` William Koh 2017-06-29 2:32 ` Andreas Dilger 2017-06-29 4:37 ` William Koh 2017-06-29 4:59 ` Darrick J. Wong 2017-06-29 14:28 ` William Koh 2017-06-29 14:35 ` J. Bruce Fields 2017-06-29 17:25 ` Darrick J. Wong 2017-06-29 18:30 ` J. Bruce Fields 2017-06-29 18:50 ` J. Bruce Fields 2017-07-04 4:04 ` Darrick J. Wong 2017-07-05 1:15 ` J. Bruce Fields 2017-07-05 19:19 ` Darrick J. Wong 2017-07-05 20:27 ` Theodore Ts'o 2017-07-07 10:51 ` Jeff Layton 2017-07-07 15:51 ` Theodore Ts'o 2017-07-07 16:13 ` Jeff Layton 2017-07-07 16:47 ` J. Bruce Fields 2017-07-05 20:49 ` J. Bruce Fields 2017-07-06 1:08 ` NeilBrown 2017-07-06 2:39 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.