[PATCH 0/6] Fix and cleanups to ext4 namei.c

[PATCH 0/6] Fix and cleanups to ext4 namei.c

[Kemeng Shi]

This series contains a fix and some random cleanups to namei.c and
this series survives "kvm-xfstest smoke".
More details can be found in respective patches. Thanks.

Kemeng Shi (6):
  ext4: add missing brelse for bh2 in ext4_dx_add_entry
  ext4: remove unneeded bits mask in dx_get_block()
  ext4: remove unneeded forward declaration in namei.c
  ext4: remove unneeded check in get_dx_countlimit
  ext4: remove unused input "inode" in ext4_find_dest_de
  ext4: calculate rec_len of ".." with correct name length 2

 fs/ext4/ext4.h   |  3 +--
 fs/ext4/inline.c |  2 +-
 fs/ext4/namei.c  | 63 +++++++++++++-----------------------------------
 3 files changed, 19 insertions(+), 49 deletions(-)

-- 
2.30.0

[PATCH 1/6] ext4: add missing brelse for bh2 in ext4_dx_add_entry

[Kemeng Shi]

Add missing brelse for bh2 in ext4_dx_add_entry.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/namei.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 1012781ae9b4..adec145b6f7d 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2580,8 +2580,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 		BUFFER_TRACE(frame->bh, "get_write_access");
 		err = ext4_journal_get_write_access(handle, sb, frame->bh,
 						    EXT4_JTR_NONE);
-		if (err)
+		if (err) {
+			brelse(bh2);
 			goto journal_error;
+		}
 		if (!add_level) {
 			unsigned icount1 = icount/2, icount2 = icount - icount1;
 			unsigned hash2 = dx_get_hash(entries + icount1);
@@ -2592,8 +2594,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 			err = ext4_journal_get_write_access(handle, sb,
 							    (frame - 1)->bh,
 							    EXT4_JTR_NONE);
-			if (err)
+			if (err) {
+				brelse(bh2);
 				goto journal_error;
+			}
 
 			memcpy((char *) entries2, (char *) (entries + icount1),
 			       icount2 * sizeof(struct dx_entry));
@@ -2612,8 +2616,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 			dxtrace(dx_show_index("node",
 			       ((struct dx_node *) bh2->b_data)->entries));
 			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
-			if (err)
+			if (err) {
+				brelse(bh2);
 				goto journal_error;
+			}
 			brelse (bh2);
 			err = ext4_handle_dirty_dx_node(handle, dir,
 						   (frame - 1)->bh);
@@ -2638,8 +2644,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
 				       "Creating %d level index...\n",
 				       dxroot->info.indirect_levels));
 			err = ext4_handle_dirty_dx_node(handle, dir, frame->bh);
-			if (err)
+			if (err) {
+				brelse(bh2);
 				goto journal_error;
+			}
 			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
 			brelse(bh2);
 			restart = 1;
-- 
2.30.0

[PATCH 2/6] ext4: remove unneeded bits mask in dx_get_block()

[Kemeng Shi]

As high four bits of block in dx_entry is not used by any feature for now,
we can remove unneeded bits mask in dx_get_block() and add it back when
it's really needed.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/namei.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index adec145b6f7d..8ff840ef4730 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -561,14 +561,9 @@ ext4_next_entry(struct ext4_dir_entry_2 *p, unsigned long blocksize)
 		ext4_rec_len_from_disk(p->rec_len, blocksize));
 }
 
-/*
- * Future: use high four bits of block for coalesce-on-delete flags
- * Mask them off for now.
- */
-
 static inline ext4_lblk_t dx_get_block(struct dx_entry *entry)
 {
-	return le32_to_cpu(entry->block) & 0x0fffffff;
+	return le32_to_cpu(entry->block);
 }
 
 static inline void dx_set_block(struct dx_entry *entry, ext4_lblk_t value)
-- 
2.30.0

[PATCH 3/6] ext4: remove unneeded forward declaration in namei.c

[Kemeng Shi]

Remove unneeded forward declaration in namei.c

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/namei.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 8ff840ef4730..33670cebdedc 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -291,36 +291,6 @@ struct dx_tail {
 	__le32 dt_checksum;	/* crc32c(uuid+inum+dirblock) */
 };
 
-static inline ext4_lblk_t dx_get_block(struct dx_entry *entry);
-static void dx_set_block(struct dx_entry *entry, ext4_lblk_t value);
-static inline unsigned dx_get_hash(struct dx_entry *entry);
-static void dx_set_hash(struct dx_entry *entry, unsigned value);
-static unsigned dx_get_count(struct dx_entry *entries);
-static unsigned dx_get_limit(struct dx_entry *entries);
-static void dx_set_count(struct dx_entry *entries, unsigned value);
-static void dx_set_limit(struct dx_entry *entries, unsigned value);
-static unsigned dx_root_limit(struct inode *dir, unsigned infosize);
-static unsigned dx_node_limit(struct inode *dir);
-static struct dx_frame *dx_probe(struct ext4_filename *fname,
-				 struct inode *dir,
-				 struct dx_hash_info *hinfo,
-				 struct dx_frame *frame);
-static void dx_release(struct dx_frame *frames);
-static int dx_make_map(struct inode *dir, struct buffer_head *bh,
-		       struct dx_hash_info *hinfo,
-		       struct dx_map_entry *map_tail);
-static void dx_sort_map(struct dx_map_entry *map, unsigned count);
-static struct ext4_dir_entry_2 *dx_move_dirents(struct inode *dir, char *from,
-					char *to, struct dx_map_entry *offsets,
-					int count, unsigned int blocksize);
-static struct ext4_dir_entry_2 *dx_pack_dirents(struct inode *dir, char *base,
-						unsigned int blocksize);
-static void dx_insert_block(struct dx_frame *frame,
-					u32 hash, ext4_lblk_t block);
-static int ext4_htree_next_block(struct inode *dir, __u32 hash,
-				 struct dx_frame *frame,
-				 struct dx_frame *frames,
-				 __u32 *start_hash);
 static struct buffer_head * ext4_dx_find_entry(struct inode *dir,
 		struct ext4_filename *fname,
 		struct ext4_dir_entry_2 **res_dir);
-- 
2.30.0

[PATCH 4/6] ext4: remove unneeded check in get_dx_countlimit

[Kemeng Shi]

The "offset" is always non-NULL, remove unneeded NULL check of "offset".

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/namei.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 33670cebdedc..07a1bb570deb 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -434,8 +434,7 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
 	} else
 		return NULL;
 
-	if (offset)
-		*offset = count_offset;
+	*offset = count_offset;
 	return (struct dx_countlimit *)(((void *)dirent) + count_offset);
 }
 
-- 
2.30.0

[PATCH 5/6] ext4: remove unused input "inode" in ext4_find_dest_de

[Kemeng Shi]

Remove unused input "inode" in ext4_find_dest_de.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ext4.h   | 3 +--
 fs/ext4/inline.c | 2 +-
 fs/ext4/namei.c  | 5 ++---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bbffb76d9a90..f7cec0de2790 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2821,8 +2821,7 @@ extern int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
 				struct ext4_dir_entry_2 *dirent,
 				struct fscrypt_str *ent_name);
 extern void ext4_htree_free_dir_info(struct dir_private_info *p);
-extern int ext4_find_dest_de(struct inode *dir, struct inode *inode,
-			     struct buffer_head *bh,
+extern int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
 			     void *buf, int buf_size,
 			     struct ext4_filename *fname,
 			     struct ext4_dir_entry_2 **dest_de);
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 3536ca7e4fcc..29081a04aef5 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1012,7 +1012,7 @@ static int ext4_add_dirent_to_inline(handle_t *handle,
 	int		err;
 	struct ext4_dir_entry_2 *de;
 
-	err = ext4_find_dest_de(dir, inode, iloc->bh, inline_start,
+	err = ext4_find_dest_de(dir, iloc->bh, inline_start,
 				inline_size, fname, &de);
 	if (err)
 		return err;
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 07a1bb570deb..aee1858e6482 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2024,8 +2024,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	return ERR_PTR(err);
 }
 
-int ext4_find_dest_de(struct inode *dir, struct inode *inode,
-		      struct buffer_head *bh,
+int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
 		      void *buf, int buf_size,
 		      struct ext4_filename *fname,
 		      struct ext4_dir_entry_2 **dest_de)
@@ -2111,7 +2110,7 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
 		csum_size = sizeof(struct ext4_dir_entry_tail);
 
 	if (!de) {
-		err = ext4_find_dest_de(dir, inode, bh, bh->b_data,
+		err = ext4_find_dest_de(dir, bh, bh->b_data,
 					blocksize - csum_size, fname, &de);
 		if (err)
 			return err;
-- 
2.30.0

[PATCH 6/6] ext4: calculate rec_len of ".." with correct name length 2

[Kemeng Shi]

The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead
of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same
number either with name_len 1 or name_len 2, it's better use the right
name_len to make code more intuitive.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index aee1858e6482..24cdeb2aa0d5 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2925,7 +2925,7 @@ struct ext4_dir_entry_2 *ext4_init_dot_dotdot(struct inode *inode,
 	de->name_len = 2;
 	if (!dotdot_real_len)
 		de->rec_len = ext4_rec_len_to_disk(blocksize -
-					(csum_size + ext4_dir_rec_len(1, NULL)),
+					(csum_size + ext4_dir_rec_len(2, NULL)),
 					blocksize);
 	else
 		de->rec_len = ext4_rec_len_to_disk(
-- 
2.30.0

Re: [PATCH 2/6] ext4: remove unneeded bits mask in dx_get_block()

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On Dec 19, 2024, at 4:00 AM, Kemeng Shi <shikemeng@huaweicloud.com> wrote:
> 
> As high four bits of block in dx_entry is not used by any feature for now, we can remove unneeded bits mask in dx_get_block() and add it back
> when it's really needed.

Actually, the opposite is true.  This mask protects the *CURRENT* code
from any future use for these bits, so removing it now means that they
could never be used in the future, since the block number would be
taken as all 32 bits instead of only the bottom 28 bits.  I don't think
we are in any danger of having a 16TB single directory any time soon.

However, the top bits were intended to store a "fullness" for the index
blocks, to optimize online directory shrinking without having to scan
each of the blocks for how many entries are currently in the block.
This would allow the dirent removal to easily see "this block and the
previous/next block are only 1/3 full and could be merged".

See the following thread for a prototype patch and discussion on this:
https://patchwork.ozlabs.org/project/linux-ext4/patch/20190821182740.97127-1-harshadshirwadkar@gmail.com/

I think removing this mask has a negative effect on future usefulness,
and virtually no benefit to the code today, so I would object to landing
it.

Cheers, Andreas

> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
> fs/ext4/namei.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index adec145b6f7d..8ff840ef4730 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -561,14 +561,9 @@ ext4_next_entry(struct ext4_dir_entry_2 *p, unsigned long blocksize)
> 		ext4_rec_len_from_disk(p->rec_len, blocksize));
> }
> 
> -/*
> - * Future: use high four bits of block for coalesce-on-delete flags
> - * Mask them off for now.
> - */
> -
> static inline ext4_lblk_t dx_get_block(struct dx_entry *entry)
> {
> -	return le32_to_cpu(entry->block) & 0x0fffffff;
> +	return le32_to_cpu(entry->block);
> }
> 
> static inline void dx_set_block(struct dx_entry *entry, ext4_lblk_t value)
> --
> 2.30.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 4/6] ext4: remove unneeded check in get_dx_countlimit

[Ojaswin Mujoo]

On Thu, Dec 19, 2024 at 07:00:25PM +0800, Kemeng Shi wrote:
> The "offset" is always non-NULL, remove unneeded NULL check of "offset".
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Hi Kemeng,

I know the current callers don't pass NULL but I think we should still
keep the check around just in case, to avoid NULL dereferences in
future. I don't think there's any harm in keeping it

Regards,
ojaswin

> ---
>  fs/ext4/namei.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 33670cebdedc..07a1bb570deb 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -434,8 +434,7 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
>  	} else
>  		return NULL;
>  
> -	if (offset)
> -		*offset = count_offset;
> +	*offset = count_offset;
>  	return (struct dx_countlimit *)(((void *)dirent) + count_offset);
>  }
>  
> -- 
> 2.30.0
> 

Re: [PATCH 3/6] ext4: remove unneeded forward declaration in namei.c

[Ojaswin Mujoo]

On Thu, Dec 19, 2024 at 07:00:24PM +0800, Kemeng Shi wrote:
> Remove unneeded forward declaration in namei.c
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Regards,
ojaswin

> ---
>  fs/ext4/namei.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8ff840ef4730..33670cebdedc 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -291,36 +291,6 @@ struct dx_tail {
>  	__le32 dt_checksum;	/* crc32c(uuid+inum+dirblock) */
>  };
>  
> -static inline ext4_lblk_t dx_get_block(struct dx_entry *entry);
> -static void dx_set_block(struct dx_entry *entry, ext4_lblk_t value);
> -static inline unsigned dx_get_hash(struct dx_entry *entry);
> -static void dx_set_hash(struct dx_entry *entry, unsigned value);
> -static unsigned dx_get_count(struct dx_entry *entries);
> -static unsigned dx_get_limit(struct dx_entry *entries);
> -static void dx_set_count(struct dx_entry *entries, unsigned value);
> -static void dx_set_limit(struct dx_entry *entries, unsigned value);
> -static unsigned dx_root_limit(struct inode *dir, unsigned infosize);
> -static unsigned dx_node_limit(struct inode *dir);
> -static struct dx_frame *dx_probe(struct ext4_filename *fname,
> -				 struct inode *dir,
> -				 struct dx_hash_info *hinfo,
> -				 struct dx_frame *frame);
> -static void dx_release(struct dx_frame *frames);
> -static int dx_make_map(struct inode *dir, struct buffer_head *bh,
> -		       struct dx_hash_info *hinfo,
> -		       struct dx_map_entry *map_tail);
> -static void dx_sort_map(struct dx_map_entry *map, unsigned count);
> -static struct ext4_dir_entry_2 *dx_move_dirents(struct inode *dir, char *from,
> -					char *to, struct dx_map_entry *offsets,
> -					int count, unsigned int blocksize);
> -static struct ext4_dir_entry_2 *dx_pack_dirents(struct inode *dir, char *base,
> -						unsigned int blocksize);
> -static void dx_insert_block(struct dx_frame *frame,
> -					u32 hash, ext4_lblk_t block);
> -static int ext4_htree_next_block(struct inode *dir, __u32 hash,
> -				 struct dx_frame *frame,
> -				 struct dx_frame *frames,
> -				 __u32 *start_hash);
>  static struct buffer_head * ext4_dx_find_entry(struct inode *dir,
>  		struct ext4_filename *fname,
>  		struct ext4_dir_entry_2 **res_dir);
> -- 
> 2.30.0
> 

Re: [PATCH 1/6] ext4: add missing brelse for bh2 in ext4_dx_add_entry

[Ojaswin Mujoo]

On Thu, Dec 19, 2024 at 07:00:22PM +0800, Kemeng Shi wrote:
> Add missing brelse for bh2 in ext4_dx_add_entry.

Looks good, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Regards,
ojaswin

> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/namei.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 1012781ae9b4..adec145b6f7d 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2580,8 +2580,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  		BUFFER_TRACE(frame->bh, "get_write_access");
>  		err = ext4_journal_get_write_access(handle, sb, frame->bh,
>  						    EXT4_JTR_NONE);
> -		if (err)
> +		if (err) {
> +			brelse(bh2);
>  			goto journal_error;
> +		}
>  		if (!add_level) {
>  			unsigned icount1 = icount/2, icount2 = icount - icount1;
>  			unsigned hash2 = dx_get_hash(entries + icount1);
> @@ -2592,8 +2594,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			err = ext4_journal_get_write_access(handle, sb,
>  							    (frame - 1)->bh,
>  							    EXT4_JTR_NONE);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  
>  			memcpy((char *) entries2, (char *) (entries + icount1),
>  			       icount2 * sizeof(struct dx_entry));
> @@ -2612,8 +2616,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			dxtrace(dx_show_index("node",
>  			       ((struct dx_node *) bh2->b_data)->entries));
>  			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  			brelse (bh2);
>  			err = ext4_handle_dirty_dx_node(handle, dir,
>  						   (frame - 1)->bh);
> @@ -2638,8 +2644,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  				       "Creating %d level index...\n",
>  				       dxroot->info.indirect_levels));
>  			err = ext4_handle_dirty_dx_node(handle, dir, frame->bh);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
>  			brelse(bh2);
>  			restart = 1;
> -- 
> 2.30.0
> 

Re: [PATCH 1/6] ext4: add missing brelse for bh2 in ext4_dx_add_entry

[Markus Elfring]

> Add missing brelse for bh2 in ext4_dx_add_entry.

* I propose to append parentheses to function names.

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145


…
> +++ b/fs/ext4/namei.c
> @@ -2580,8 +2580,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  		BUFFER_TRACE(frame->bh, "get_write_access");
>  		err = ext4_journal_get_write_access(handle, sb, frame->bh,
>  						    EXT4_JTR_NONE);
> -		if (err)
> +		if (err) {
> +			brelse(bh2);
>  			goto journal_error;
> +		}
>  		if (!add_level) {
…

I suggest to add another jump target instead so that a bit of exception handling
can be better reused at the end of this function implementation.

Regards,
Markus

Re: [PATCH 6/6] ext4: calculate rec_len of ".." with correct name length 2

[Markus Elfring]

> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead
> of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same
> number either with name_len 1 or name_len 2, it's better use the right
> name_len to make code more intuitive.

Do you try to point a correctness issue out here?

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145

Regards,
Markus

Re: [PATCH 6/6] ext4: calculate rec_len of ".." with correct name length 2

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On Dec 20, 2024, at 6:52 AM, Markus Elfring <Markus.Elfring@web.de> wrote:
> 
>> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead
>> of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same
>> number either with name_len 1 or name_len 2, it's better use the right
>> name_len to make code more intuitive.
> 
> Do you try to point a correctness issue out here?
> 
> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145

The patch is a no-op in terms of functionality.  Dirent lengths are rounded up to a multiple of 4 bytes, so "1" and "2" give the same value.

I was looking back at the changes to this code, and it has existed
since at least when ext4 was cloned from ext3.  In older versions
of the code this calculation was done *before* the

I also realized that the original code with "1" is actually correct.
This code is calculating the dirent length of the ".." entry, but
this is actually "the rest of the block minus the the '.' dirent size",
so passing "1" is correct in this case.

So this patch should not be applied.


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/6] ext4: add missing brelse for bh2 in ext4_dx_add_entry

[Zhang Yi]

On 2024/12/19 19:00, Kemeng Shi wrote:
> Add missing brelse for bh2 in ext4_dx_add_entry.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

It's a good catch, looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/namei.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 1012781ae9b4..adec145b6f7d 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2580,8 +2580,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  		BUFFER_TRACE(frame->bh, "get_write_access");
>  		err = ext4_journal_get_write_access(handle, sb, frame->bh,
>  						    EXT4_JTR_NONE);
> -		if (err)
> +		if (err) {
> +			brelse(bh2);
>  			goto journal_error;
> +		}
>  		if (!add_level) {
>  			unsigned icount1 = icount/2, icount2 = icount - icount1;
>  			unsigned hash2 = dx_get_hash(entries + icount1);
> @@ -2592,8 +2594,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			err = ext4_journal_get_write_access(handle, sb,
>  							    (frame - 1)->bh,
>  							    EXT4_JTR_NONE);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  
>  			memcpy((char *) entries2, (char *) (entries + icount1),
>  			       icount2 * sizeof(struct dx_entry));
> @@ -2612,8 +2616,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  			dxtrace(dx_show_index("node",
>  			       ((struct dx_node *) bh2->b_data)->entries));
>  			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  			brelse (bh2);
>  			err = ext4_handle_dirty_dx_node(handle, dir,
>  						   (frame - 1)->bh);
> @@ -2638,8 +2644,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>  				       "Creating %d level index...\n",
>  				       dxroot->info.indirect_levels));
>  			err = ext4_handle_dirty_dx_node(handle, dir, frame->bh);
> -			if (err)
> +			if (err) {
> +				brelse(bh2);
>  				goto journal_error;
> +			}
>  			err = ext4_handle_dirty_dx_node(handle, dir, bh2);
>  			brelse(bh2);
>  			restart = 1;

Re: [PATCH 3/6] ext4: remove unneeded forward declaration in namei.c

[Zhang Yi]

On 2024/12/19 19:00, Kemeng Shi wrote:
> Remove unneeded forward declaration in namei.c
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/namei.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8ff840ef4730..33670cebdedc 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -291,36 +291,6 @@ struct dx_tail {
>  	__le32 dt_checksum;	/* crc32c(uuid+inum+dirblock) */
>  };
>  
> -static inline ext4_lblk_t dx_get_block(struct dx_entry *entry);
> -static void dx_set_block(struct dx_entry *entry, ext4_lblk_t value);
> -static inline unsigned dx_get_hash(struct dx_entry *entry);
> -static void dx_set_hash(struct dx_entry *entry, unsigned value);
> -static unsigned dx_get_count(struct dx_entry *entries);
> -static unsigned dx_get_limit(struct dx_entry *entries);
> -static void dx_set_count(struct dx_entry *entries, unsigned value);
> -static void dx_set_limit(struct dx_entry *entries, unsigned value);
> -static unsigned dx_root_limit(struct inode *dir, unsigned infosize);
> -static unsigned dx_node_limit(struct inode *dir);
> -static struct dx_frame *dx_probe(struct ext4_filename *fname,
> -				 struct inode *dir,
> -				 struct dx_hash_info *hinfo,
> -				 struct dx_frame *frame);
> -static void dx_release(struct dx_frame *frames);
> -static int dx_make_map(struct inode *dir, struct buffer_head *bh,
> -		       struct dx_hash_info *hinfo,
> -		       struct dx_map_entry *map_tail);
> -static void dx_sort_map(struct dx_map_entry *map, unsigned count);
> -static struct ext4_dir_entry_2 *dx_move_dirents(struct inode *dir, char *from,
> -					char *to, struct dx_map_entry *offsets,
> -					int count, unsigned int blocksize);
> -static struct ext4_dir_entry_2 *dx_pack_dirents(struct inode *dir, char *base,
> -						unsigned int blocksize);
> -static void dx_insert_block(struct dx_frame *frame,
> -					u32 hash, ext4_lblk_t block);
> -static int ext4_htree_next_block(struct inode *dir, __u32 hash,
> -				 struct dx_frame *frame,
> -				 struct dx_frame *frames,
> -				 __u32 *start_hash);
>  static struct buffer_head * ext4_dx_find_entry(struct inode *dir,
>  		struct ext4_filename *fname,
>  		struct ext4_dir_entry_2 **res_dir);

Re: [PATCH 4/6] ext4: remove unneeded check in get_dx_countlimit

[Zhang Yi]

On 2024/12/19 19:00, Kemeng Shi wrote:
> The "offset" is always non-NULL, remove unneeded NULL check of "offset".
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I agree with Ojaswin, the check strengthens the function, and I'd
suggest that we'd better to keep it for now.

Thanks,
Yi.

> ---
>  fs/ext4/namei.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 33670cebdedc..07a1bb570deb 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -434,8 +434,7 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
>  	} else
>  		return NULL;
>  
> -	if (offset)
> -		*offset = count_offset;
> +	*offset = count_offset;
>  	return (struct dx_countlimit *)(((void *)dirent) + count_offset);
>  }
>  

Re: [PATCH 5/6] ext4: remove unused input "inode" in ext4_find_dest_de

[Zhang Yi]

On 2024/12/19 19:00, Kemeng Shi wrote:
> Remove unused input "inode" in ext4_find_dest_de.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/ext4.h   | 3 +--
>  fs/ext4/inline.c | 2 +-
>  fs/ext4/namei.c  | 5 ++---
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index bbffb76d9a90..f7cec0de2790 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2821,8 +2821,7 @@ extern int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
>  				struct ext4_dir_entry_2 *dirent,
>  				struct fscrypt_str *ent_name);
>  extern void ext4_htree_free_dir_info(struct dir_private_info *p);
> -extern int ext4_find_dest_de(struct inode *dir, struct inode *inode,
> -			     struct buffer_head *bh,
> +extern int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  			     void *buf, int buf_size,
>  			     struct ext4_filename *fname,
>  			     struct ext4_dir_entry_2 **dest_de);
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 3536ca7e4fcc..29081a04aef5 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1012,7 +1012,7 @@ static int ext4_add_dirent_to_inline(handle_t *handle,
>  	int		err;
>  	struct ext4_dir_entry_2 *de;
>  
> -	err = ext4_find_dest_de(dir, inode, iloc->bh, inline_start,
> +	err = ext4_find_dest_de(dir, iloc->bh, inline_start,
>  				inline_size, fname, &de);
>  	if (err)
>  		return err;
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 07a1bb570deb..aee1858e6482 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2024,8 +2024,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
>  	return ERR_PTR(err);
>  }
>  
> -int ext4_find_dest_de(struct inode *dir, struct inode *inode,
> -		      struct buffer_head *bh,
> +int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  		      void *buf, int buf_size,
>  		      struct ext4_filename *fname,
>  		      struct ext4_dir_entry_2 **dest_de)
> @@ -2111,7 +2110,7 @@ static int add_dirent_to_buf(handle_t *handle, struct ext4_filename *fname,
>  		csum_size = sizeof(struct ext4_dir_entry_tail);
>  
>  	if (!de) {
> -		err = ext4_find_dest_de(dir, inode, bh, bh->b_data,
> +		err = ext4_find_dest_de(dir, bh, bh->b_data,
>  					blocksize - csum_size, fname, &de);
>  		if (err)
>  			return err;

Re: [PATCH 2/6] ext4: remove unneeded bits mask in dx_get_block()

[Kemeng Shi]

on 12/20/2024 9:10 AM, Andreas Dilger wrote:
> On Dec 19, 2024, at 4:00 AM, Kemeng Shi <shikemeng@huaweicloud.com> wrote:
>>
>> As high four bits of block in dx_entry is not used by any feature for now, we can remove unneeded bits mask in dx_get_block() and add it back
>> when it's really needed.
> 
> Actually, the opposite is true.  This mask protects the *CURRENT* code
> from any future use for these bits, so removing it now means that they
> could never be used in the future, since the block number would be
> taken as all 32 bits instead of only the bottom 28 bits.  I don't think
> we are in any danger of having a 16TB single directory any time soon.
> 
> However, the top bits were intended to store a "fullness" for the index
> blocks, to optimize online directory shrinking without having to scan
> each of the blocks for how many entries are currently in the block.
> This would allow the dirent removal to easily see "this block and the
> previous/next block are only 1/3 full and could be merged".
> 
> See the following thread for a prototype patch and discussion on this:
> https://patchwork.ozlabs.org/project/linux-ext4/patch/20190821182740.97127-1-harshadshirwadkar@gmail.com/
> 
> I think removing this mask has a negative effect on future usefulness,
> and virtually no benefit to the code today, so I would object to landing
> it.
Sure, it makes sense to reserve bit for future use if it will likely be
used. But I wonder would it be better to catch using high four bits in
in ext4_append() in which case we could forbit using high four bits in
time rather than lost dir when reserved bits are really used in future.
This also reduce cpu cost as dx_get_block() is used likely more frequent
than ext4_append().
Just a thought.

Thanks,
Kemeng

Re: [PATCH 4/6] ext4: remove unneeded check in get_dx_countlimit

[Kemeng Shi]

on 12/20/2024 8:51 PM, Ojaswin Mujoo wrote:
> On Thu, Dec 19, 2024 at 07:00:25PM +0800, Kemeng Shi wrote:
>> The "offset" is always non-NULL, remove unneeded NULL check of "offset".
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> Hi Kemeng,
> 
> I know the current callers don't pass NULL but I think we should still
> keep the check around just in case, to avoid NULL dereferences in
> future. I don't think there's any harm in keeping it
> 
Sure, no insistant on this and will drop this in next version.

Thanks,
Kemeng

> Regards,
> ojaswin
> 
>> ---
>>  fs/ext4/namei.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index 33670cebdedc..07a1bb570deb 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -434,8 +434,7 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
>>  	} else
>>  		return NULL;
>>  
>> -	if (offset)
>> -		*offset = count_offset;
>> +	*offset = count_offset;
>>  	return (struct dx_countlimit *)(((void *)dirent) + count_offset);
>>  }
>>  
>> -- 
>> 2.30.0
>>
> 

Re: [PATCH 1/6] ext4: add missing brelse for bh2 in ext4_dx_add_entry

[Kemeng Shi]

on 12/20/2024 9:15 PM, Markus Elfring wrote:
>> Add missing brelse for bh2 in ext4_dx_add_entry.
> 
> * I propose to append parentheses to function names.
> 
> * How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145
Sure, will handle these in next version.

Thanks,
Kemeng
> 
> 
> …
>> +++ b/fs/ext4/namei.c
>> @@ -2580,8 +2580,10 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname,
>>  		BUFFER_TRACE(frame->bh, "get_write_access");
>>  		err = ext4_journal_get_write_access(handle, sb, frame->bh,
>>  						    EXT4_JTR_NONE);
>> -		if (err)
>> +		if (err) {
>> +			brelse(bh2);
>>  			goto journal_error;
>> +		}
>>  		if (!add_level) {
> …
> 
> I suggest to add another jump target instead so that a bit of exception handling
> can be better reused at the end of this function implementation.
> 
> Regards,
> Markus
> 

Re: [PATCH 6/6] ext4: calculate rec_len of ".." with correct name length 2

[Kemeng Shi]

on 12/21/2024 4:38 AM, Andreas Dilger wrote:
> On Dec 20, 2024, at 6:52 AM, Markus Elfring <Markus.Elfring@web.de> wrote:
>>
>>> The rec_len of directory ".." should be ext4_dir_rec_len(2, NULL) instead
>>> of ext4_dir_rec_len(1, NULL). Although ext4_dir_rec_len return the same
>>> number either with name_len 1 or name_len 2, it's better use the right
>>> name_len to make code more intuitive.
>>
>> Do you try to point a correctness issue out here?
>>
>> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13-rc3#n145
> 
> The patch is a no-op in terms of functionality.  Dirent lengths are rounded up to a multiple of 4 bytes, so "1" and "2" give the same value.
> 
> I was looking back at the changes to this code, and it has existed
> since at least when ext4 was cloned from ext3.  In older versions
> of the code this calculation was done *before* the
> 
> I also realized that the original code with "1" is actually correct.
> This code is calculating the dirent length of the ".." entry, but
> this is actually "the rest of the block minus the the '.' dirent size",
> so passing "1" is correct in this case.
> 
> So this patch should not be applied.
Ahh, right... Thanks for point this out. Will drop this in next version.

Thanks,
Kemeng
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

Re: [PATCH 4/6] ext4: remove unneeded check in get_dx_countlimit

[Kemeng Shi]

on 12/21/2024 3:44 PM, Zhang Yi wrote:
> On 2024/12/19 19:00, Kemeng Shi wrote:
>> The "offset" is always non-NULL, remove unneeded NULL check of "offset".
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> I agree with Ojaswin, the check strengthens the function, and I'd
> suggest that we'd better to keep it for now.
Sure, will drop this in next version.

Thanks,
Kemeng
> 
> Thanks,
> Yi.
> 
>> ---
>>  fs/ext4/namei.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index 33670cebdedc..07a1bb570deb 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -434,8 +434,7 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
>>  	} else
>>  		return NULL;
>>  
>> -	if (offset)
>> -		*offset = count_offset;
>> +	*offset = count_offset;
>>  	return (struct dx_countlimit *)(((void *)dirent) + count_offset);
>>  }
>>  
>