[PATCH] ext4: fix dir_nlink behaviour

[PATCH] ext4: fix dir_nlink behaviour

[Andreas Dilger]

The dir_nlink feature has been enabled by default for new ext4
filesystems since e2fsprogs-1.41 in 2008, and was automatically
enabled by the kernel for older ext4 filesystems since the
dir_nlink feature was added with ext4 in kernel 2.6.28+ when
the subdirectory count exceeded EXT4_LINK_MAX.

We shouldn't really be enabling filesystem features automatically,
as this prevents the administrator from disabling the feature at
format time, or via tune2fs.  This should not affect many users by
this point, but allows limiting subdirectory counts to those that
can strictly fit into i_links_count rather than using "1" to
indicate that the number of links on the directory is not tracked.
This avoids a bug in glibc fts_read() that incorrectly optimizes
the directory traversal for such directories.

This also addresses a minor bug in ext4_inc_count() where i_nlinks
was wrapped at (EXT4_LINK_MAX - 1) links rather than allowing the
full EXT4_LINK_MAX links on the parent directory (including "."
and "..") before storing i_links_count = 1.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196405
Signed-off-by: Andreas Dilger <adilger@dilger.ca>
---
 fs/ext4/ext4.h  |  3 ++-
 fs/ext4/namei.c | 21 ++++++++++++---------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8e80461..e163b87 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2016,7 +2016,8 @@ static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
 
 #define is_dx(dir) (ext4_has_feature_dir_index((dir)->i_sb) && \
 		    ext4_test_inode_flag((dir), EXT4_INODE_INDEX))
-#define EXT4_DIR_LINK_MAX(dir) (!is_dx(dir) && (dir)->i_nlink >= EXT4_LINK_MAX)
+#define EXT4_DIR_LINK_MAX(dir) unlikely((dir)->i_nlink >= EXT4_LINK_MAX && \
+		    !(ext4_has_feature_dir_nlink((dir)->i_sb) && is_dx(dir)))
 #define EXT4_DIR_LINK_EMPTY(dir) ((dir)->i_nlink == 2 || (dir)->i_nlink == 1)
 
 /* Legal values for the dx_root hash_version field: */
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index b81f7d4..566c149 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2351,19 +2351,22 @@ static int ext4_delete_entry(handle_t *handle,
 }
 
 /*
- * DIR_NLINK feature is set if 1) nlinks > EXT4_LINK_MAX or 2) nlinks == 2,
- * since this indicates that nlinks count was previously 1.
+ * Set directory link count to 1 if nlinks > EXT4_LINK_MAX, or if nlinks == 2
+ * since this indicates that nlinks count was previously 1 to avoid overflowing
+ * the 16-bit i_links_count field on disk.  Directories with i_nlink == 1 mean
+ * that subdirectory link counts are not being maintained accurately.
+ *
+ * The caller has already checked for i_nlink overflow in case the DIR_LINK
+ * feature is not enabled and returned -EMLINK.  The is_dx() check is a proxy
+ * for checking S_ISDIR(inode) (since the INODE_INDEX feature will not be set
+ * on regular files) and to avoid creating huge/slow non-HTREE directories.
  */
 static void ext4_inc_count(handle_t *handle, struct inode *inode)
 {
 	inc_nlink(inode);
-	if (is_dx(inode) && inode->i_nlink > 1) {
-		/* limit is 16-bit i_links_count */
-		if (inode->i_nlink >= EXT4_LINK_MAX || inode->i_nlink == 2) {
-			set_nlink(inode, 1);
-			ext4_set_feature_dir_nlink(inode->i_sb);
-		}
-	}
+	if (is_dx(inode) &&
+	    (inode->i_nlink > EXT4_LINK_MAX || inode->i_nlink == 2))
+		set_nlink(inode, 1);
 }
 
 /*
-- 
1.8.0

Re: [PATCH] ext4: fix dir_nlink behaviour

[Damien Guibouret]

Andreas Dilger wrote:
> The dir_nlink feature has been enabled by default for new ext4
> filesystems since e2fsprogs-1.41 in 2008, and was automatically
> enabled by the kernel for older ext4 filesystems since the
> dir_nlink feature was added with ext4 in kernel 2.6.28+ when
> the subdirectory count exceeded EXT4_LINK_MAX.
> 
> We shouldn't really be enabling filesystem features automatically,
> as this prevents the administrator from disabling the feature at
> format time, or via tune2fs.  This should not affect many users by
> this point, but allows limiting subdirectory counts to those that
> can strictly fit into i_links_count rather than using "1" to
> indicate that the number of links on the directory is not tracked.
> This avoids a bug in glibc fts_read() that incorrectly optimizes
> the directory traversal for such directories.
> 
> This also addresses a minor bug in ext4_inc_count() where i_nlinks
> was wrapped at (EXT4_LINK_MAX - 1) links rather than allowing the
> full EXT4_LINK_MAX links on the parent directory (including "."
> and "..") before storing i_links_count = 1.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196405
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>
> ---
>  fs/ext4/ext4.h  |  3 ++-
>  fs/ext4/namei.c | 21 ++++++++++++---------
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8e80461..e163b87 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2016,7 +2016,8 @@ static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
>  
>  #define is_dx(dir) (ext4_has_feature_dir_index((dir)->i_sb) && \
>  		    ext4_test_inode_flag((dir), EXT4_INODE_INDEX))
> -#define EXT4_DIR_LINK_MAX(dir) (!is_dx(dir) && (dir)->i_nlink >= EXT4_LINK_MAX)
> +#define EXT4_DIR_LINK_MAX(dir) unlikely((dir)->i_nlink >= EXT4_LINK_MAX && \
> +		    !(ext4_has_feature_dir_nlink((dir)->i_sb) && is_dx(dir)))
>  #define EXT4_DIR_LINK_EMPTY(dir) ((dir)->i_nlink == 2 || (dir)->i_nlink == 1)
>  
>  /* Legal values for the dx_root hash_version field: */
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index b81f7d4..566c149 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2351,19 +2351,22 @@ static int ext4_delete_entry(handle_t *handle,
>  }
>  
>  /*
> - * DIR_NLINK feature is set if 1) nlinks > EXT4_LINK_MAX or 2) nlinks == 2,
> - * since this indicates that nlinks count was previously 1.
> + * Set directory link count to 1 if nlinks > EXT4_LINK_MAX, or if nlinks == 2
> + * since this indicates that nlinks count was previously 1 to avoid overflowing
> + * the 16-bit i_links_count field on disk.  Directories with i_nlink == 1 mean
> + * that subdirectory link counts are not being maintained accurately.
> + *
> + * The caller has already checked for i_nlink overflow in case the DIR_LINK
> + * feature is not enabled and returned -EMLINK.  The is_dx() check is a proxy
> + * for checking S_ISDIR(inode) (since the INODE_INDEX feature will not be set
> + * on regular files) and to avoid creating huge/slow non-HTREE directories.
>   */
>  static void ext4_inc_count(handle_t *handle, struct inode *inode)
>  {
>  	inc_nlink(inode);
> -	if (is_dx(inode) && inode->i_nlink > 1) {
> -		/* limit is 16-bit i_links_count */
> -		if (inode->i_nlink >= EXT4_LINK_MAX || inode->i_nlink == 2) {
> -			set_nlink(inode, 1);
> -			ext4_set_feature_dir_nlink(inode->i_sb);
> -		}
> -	}
> +	if (is_dx(inode) &&
> +	    (inode->i_nlink > EXT4_LINK_MAX || inode->i_nlink == 2))
> +		set_nlink(inode, 1);
>  }
>  
>  /*

Hello,

Is the change of the check from (inode->i_nlink >= EXT4_LINK_MAX) to 
(inode->i_nlink > EXT4_LINK_MAX) really needed?
That just allows reaching the EXT4_LINK_MAX in case the nlink feature is enabled 
and what does it change? At next call, it will be set to 1 so that just delays 
the wrap for 1 directory.
It shall be noticed that in case nlink feature is absent, the EXT4_LINK_MAX 
value will be reached (so the limit is correct), and if it is present, reaching 
this value or one less is quite irrevelant as you can create more links than that.

ext4_link does the same check and it is modified from >= to > also, in case 
nlink feature is absent that will allow reaching EXT4_LINK_MAX+1.

If you look at vfs layer, vfs_mkdir does the check against max_links with >= and 
so if you reach the value, you will never be able to create a new directory even 
if nlink feature is present. Actually it is not a problem because ext4 does not 
set the s_max_links value in vfs (do not know why as ext2 set it perhaps because 
there is no limit if nlink is set?), but it could be some latent bug if someday 
somebody thinks it could be a good idea to set it.

Regards,

Damien

Re: [PATCH] ext4: fix dir_nlink behaviour

[Theodore Ts'o]

On Sat, Jul 22, 2017 at 03:38:41PM +0200, Damien Guibouret wrote:
> 
> If you look at vfs layer, vfs_mkdir does the check against max_links with >=
> and so if you reach the value, you will never be able to create a new
> directory even if nlink feature is present. Actually it is not a problem
> because ext4 does not set the s_max_links value in vfs (do not know why as
> ext2 set it perhaps because there is no limit if nlink is set?), but it
> could be some latent bug if someday somebody thinks it could be a good idea
> to set it.

We can't set s_max_links precisely because of the dir_nlinks feature.
So the fact that it is not set today is deliberate, and if someone
thought it was good idea, I and the other ext4 developers would NAK
it.

						- Ted

Re: [PATCH] ext4: fix dir_nlink behaviour

[Theodore Ts'o]

On Fri, Jul 21, 2017 at 04:49:27PM -0600, Andreas Dilger wrote:
> The dir_nlink feature has been enabled by default for new ext4
> filesystems since e2fsprogs-1.41 in 2008, and was automatically
> enabled by the kernel for older ext4 filesystems since the
> dir_nlink feature was added with ext4 in kernel 2.6.28+ when
> the subdirectory count exceeded EXT4_LINK_MAX.
> 
> We shouldn't really be enabling filesystem features automatically,
> as this prevents the administrator from disabling the feature at
> format time, or via tune2fs.  This should not affect many users by
> this point, but allows limiting subdirectory counts to those that
> can strictly fit into i_links_count rather than using "1" to
> indicate that the number of links on the directory is not tracked.
> This avoids a bug in glibc fts_read() that incorrectly optimizes
> the directory traversal for such directories.
> 
> This also addresses a minor bug in ext4_inc_count() where i_nlinks
> was wrapped at (EXT4_LINK_MAX - 1) links rather than allowing the
> full EXT4_LINK_MAX links on the parent directory (including "."
> and "..") before storing i_links_count = 1.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196405
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>

Thanks, applied.

					- Ted