[PATCH] ext4: Modify the rec_len helpers to accommodate future cases

[PATCH] ext4: Modify the rec_len helpers to accommodate future cases

[zhangshida]

From: Shida Zhang <zhangshida@kylinos.cn>

Following Andreas' suggestion, it is time to adapt these helpers
to handle larger records during runtime, especially in preparation
for the eventual support of ext4 with a block size greater than
PAGE_SIZE.

Suggested-by: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
Link: https://lore.kernel.org/lkml/A9ECDF14-95A1-4B1E-A815-4B6ABF4916C6@dilger.ca/
---
 fs/ext4/ext4.h | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0a2d55faa095..87cdf4d56da1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2386,39 +2386,41 @@ static inline unsigned int ext4_dir_rec_len(__u8 name_len,
 }
 
 /*
- * If we ever get support for fs block sizes > page_size, we'll need
- * to remove the #if statements in the next two functions...
+ * The next two helpers facilitate the conversion between the encoded
+ * and plain forms of rec_len. Try to access rec_len through these helpers
+ * rather than accessing it directly.
  */
-static inline unsigned int
-ext4_rec_len_from_disk(__le16 dlen, unsigned blocksize)
+static inline
+unsigned int ext4_rec_len_from_disk(__le16 dlen, unsigned blocksize)
 {
 	unsigned len = le16_to_cpu(dlen);
 
-#if (PAGE_SIZE >= 65536)
+	if (blocksize < 65536)
+		return len;
+
 	if (len == EXT4_MAX_REC_LEN || len == 0)
 		return blocksize;
+
 	return (len & 65532) | ((len & 3) << 16);
-#else
-	return len;
-#endif
 }
 
 static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
 {
-	BUG_ON((len > blocksize) || (blocksize > (1 << 18)) || (len & 3));
-#if (PAGE_SIZE >= 65536)
-	if (len < 65536)
+	BUG_ON(len > blocksize);
+	BUG_ON(blocksize > (1 << 18));
+	BUG_ON(len & 3);
+
+	if (len < 65536) /* always true for blocksize < 65536 */
 		return cpu_to_le16(len);
+
 	if (len == blocksize) {
 		if (blocksize == 65536)
 			return cpu_to_le16(EXT4_MAX_REC_LEN);
-		else
-			return cpu_to_le16(0);
+
+		return cpu_to_le16(0);
 	}
+
 	return cpu_to_le16((len & 65532) | ((len >> 16) & 3));
-#else
-	return cpu_to_le16(len);
-#endif
 }
 
 /*
-- 
2.27.0

Re: [PATCH] ext4: Modify the rec_len helpers to accommodate future cases

[Theodore Ts'o]

On Mon, Aug 07, 2023 at 09:26:54AM +0800, zhangshida wrote:
> From: Shida Zhang <zhangshida@kylinos.cn>
> 
> Following Andreas' suggestion, it is time to adapt these helpers
> to handle larger records during runtime, especially in preparation
> for the eventual support of ext4 with a block size greater than
> PAGE_SIZE.

Is there a reason for landing this now?  We don't have support for
block_size > PAGE_SIZE yet, and this patch doesn't come for free, at
least not systems with page_size < 64k.  These inline functions are
*very* hot and get used in a large number of places.  Have you looked
to see what it might do to text size of the ext4 code?  And whether
the expansion to the icache might actually impact performance on CPU
bound workloads with very large directories?

I will note that there are some opportunities to optimize how often we
use ext4_rec_len_from_disk.  For example, it gets called from
ext4_check_dir_entry(), and often the callers of that function will
need the directory record length.  So having ext4_check_dir_entry()
optionally fill in the rec_len via a passed-in pointer might be
worthwhile.

Cheers,

						- Ted

Re: [PATCH] ext4: Modify the rec_len helpers to accommodate future cases

[Stephen Zhang]

Theodore Ts'o <tytso@mit.edu> 于2023年8月18日周五 01:31写道：
>
> On Mon, Aug 07, 2023 at 09:26:54AM +0800, zhangshida wrote:
> > From: Shida Zhang <zhangshida@kylinos.cn>
> >
> > Following Andreas' suggestion, it is time to adapt these helpers
> > to handle larger records during runtime, especially in preparation
> > for the eventual support of ext4 with a block size greater than
> > PAGE_SIZE.
>
> Is there a reason for landing this now?  We don't have support for
> block_size > PAGE_SIZE yet, and this patch doesn't come for free, at
> least not systems with page_size < 64k.  These inline functions are
> *very* hot and get used in a large number of places.  Have you looked
> to see what it might do to text size of the ext4 code?  And whether
> the expansion to the icache might actually impact performance on CPU
> bound workloads with very large directories?
>
> I will note that there are some opportunities to optimize how often we
> use ext4_rec_len_from_disk.  For example, it gets called from
> ext4_check_dir_entry(), and often the callers of that function will
> need the directory record length.  So having ext4_check_dir_entry()
> optionally fill in the rec_len via a passed-in pointer might be
> worthwhile.

Yep, the best way to do it is to leave it unmerged until it is necessary.
At the same time, I will try to eliminate these regression concerns based
on these suggestions.

Cheers,
Shida

>
> Cheers,
>
>                                                 - Ted