Re: [PATCH] ext4: Improve feature checking

On 6/30/24 17:25, Richard Weinberger wrote:

Evaluate the filesystem incompat and ro_compat bit fields to judge whether the filesystem can be read or written. For the read side only a scary warning is shown so far. I'd love to about mounting too, but I fear this will break some setups

I think you are missing a verb in the first half of your sentence

where the driver works by chance.

Signed-off-by: Richard Weinberger richard@nod.at

After facing ext4 write corruptions and read errors I checked the ext4 implementation briefly. Hopefully this patch helps other users to detect earlier that they're using ext4 features which are not supported by u-boot.

To make feature checking possible I had to figure what this implementation actually supports. I hope I got them right, feedback is welcome!

Thanks, //richard

---

fs/ext4/ext4_common.c | 8 +++++++ fs/ext4/ext4_write.c | 12 ++++++++-- include/ext4fs.h | 55 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 2ff0dca249..7148d35ee0 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2386,6 +2386,14 @@ int ext4fs_mount(void) fs->inodesz = 128; fs->gdsize = 32; } else {

• int missing = __le32_to_cpu(data->sblock.feature_incompat) & \
  

• 	      ~(EXT4_FEATURE_INCOMPAT_SUPP | \
  

• 		EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO);
  

• if (missing)
  

• 	log_err("fs uses incompatible features: %08x, failures *are* expected!\n",
  

• 		missing);
  

I think it is a bit unclear to the user what their action should be. Maybe something like

printf("EXT4 incompatible features: %08x, ignoring...\n")

and then maybe add a comment like what you have in the commit message.

debug("EXT4 features COMPAT: %08x INCOMPAT: %08x RO_COMPAT: %08x\n",
      __le32_to_cpu(data->sblock.feature_compatibility),
      __le32_to_cpu(data->sblock.feature_incompat),

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index d057f6b5a7..4aae3c5f7f 100644 --- a/fs/ext4/ext4_write.c +++ b/fs/ext4/ext4_write.c @@ -869,6 +869,7 @@ int ext4fs_write(const char *fname, const char *buffer, ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256); bool store_link_in_inode = false; memset(filename, 0x00, 256);

• int missing_feat;

  if (type != FILETYPE_REG && type != FILETYPE_SYMLINK) return -1;

@@ -882,8 +883,15 @@ int ext4fs_write(const char *fname, const char *buffer, return -1; }

• if (le32_to_cpu(fs->sb->feature_ro_compat) & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {

• printf("Unsupported feature metadata_csum found, not writing.\n");
  

• missing_feat = le32_to_cpu(fs->sb->feature_incompat) & ~EXT4_FEATURE_INCOMPAT_SUPP;
• if (missing_feat) {

• log_err("Unsupported features found %08x, not writing.\n", missing_feat);
  

• return -1;
  

• }
• missing_feat = le32_to_cpu(fs->sb->feature_ro_compat) & ~EXT4_FEATURE_RO_COMPAT_SUPP;
• if (missing_feat) {

• log_err("Unsupported RO compat features found %08x, not writing.\n", missing_feat);
  

  return -1; }

diff --git a/include/ext4fs.h b/include/ext4fs.h index d96edfd057..01b66f469f 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -34,12 +34,65 @@ struct disk_partition; #define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/ #define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */ #define EXT4_EXT_MAGIC 0xf30a -#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010

+#define EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER 0x0001 +#define EXT4_FEATURE_RO_COMPAT_LARGE_FILE 0x0002 +#define EXT4_FEATURE_RO_COMPAT_BTREE_DIR 0x0004 +#define EXT4_FEATURE_RO_COMPAT_HUGE_FILE 0x0008 +#define EXT4_FEATURE_RO_COMPAT_GDT_CSUM 0x0010 +#define EXT4_FEATURE_RO_COMPAT_DIR_NLINK 0x0020 +#define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE 0x0040 +#define EXT4_FEATURE_RO_COMPAT_QUOTA 0x0100 +#define EXT4_FEATURE_RO_COMPAT_BIGALLOC 0x0200 #define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM 0x0400

+#define EXT4_FEATURE_INCOMPAT_FILETYPE 0x0002 +#define EXTE_FEATURE_INCOMPAT_RECOVER 0x0004 #define EXT4_FEATURE_INCOMPAT_EXTENTS 0x0040 #define EXT4_FEATURE_INCOMPAT_64BIT 0x0080 +#define EXT4_FEATURE_INCOMPAT_MMP 0x0100 +#define EXT4_FEATURE_INCOMPAT_FLEX_BG 0x0200 +#define EXT4_FEATURE_INCOMPAT_CSUM_SEED 0x2000 +#define EXT4_FEATURE_INCOMPAT_ENCRYPT 0x10000

• #define EXT4_INDIRECT_BLOCKS 12

+/*

• • Incompat features supported by this implementation.
• */

+#define EXT4_FEATURE_INCOMPAT_SUPP ( EXT4_FEATURE_INCOMPAT_FILETYPE \

• 		   | EXTE_FEATURE_INCOMPAT_RECOVER \
  

EXT4

• 		   | EXT4_FEATURE_INCOMPAT_EXTENTS \
  

• 		   | EXT4_FEATURE_INCOMPAT_64BIT \
  

• 		   | EXT4_FEATURE_INCOMPAT_FLEX_BG \
  

• 		   )
  

Operators should go at the end of the line. So like

#define EXT4_FEATURE_INCOMPAT_SUPP (EXT4_FEATURE_INCOMPAT_FILETYPE | \ EXT4_FEATURE_INCOMPAT_RECOVER | \ EXT4_FEATURE_INCOMPAT_EXTENTS | \ EXT4_FEATURE_INCOMPAT_64BIT | \ EXT4_FEATURE_INCOMPAT_FLEX_BG)

+/*

• • Incompat features supported by this implementation only in a lazy
• • way, good enough for reading files.
• • • Multi mount protection (mmp) is not supported, but for read-only
• • we get away with it.
• • • Same fore metadata_csum_seed and metadata_csum.

nit: for

• • • The implementation has also no clue about fscrypt, but it can read
• • unencrypted files. Reading encrypted files will read garbage.
• */

+#define EXT4_FEATURE_INCOMPAT_SUPP_LAZY_RO ( EXT4_FEATURE_INCOMPAT_MMP \

• 			   | EXT4_FEATURE_INCOMPAT_CSUM_SEED \
  

• 			   | EXT4_FEATURE_INCOMPAT_ENCRYPT \
  

• 			   )
  

ditto

+/*

• • Read-only compat features we support.
• • If unknown ro compat features are detected, writing to the fs is denied.
• */

+#define EXT4_FEATURE_RO_COMPAT_SUPP ( EXT4_FEATURE_RO_COMPAT_SPARSE_SUPER \

• 		    | EXT4_FEATURE_RO_COMPAT_LARGE_FILE \
  

• 		    | EXT4_FEATURE_RO_COMPAT_HUGE_FILE \
  

• 		    | EXT4_FEATURE_RO_COMPAT_DIR_NLINK \
  

• 		    | EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE \
  

• 		    )
  

ditto

#define EXT4_BG_INODE_UNINIT 0x0001 #define EXT4_BG_BLOCK_UNINIT 0x0002 #define EXT4_BG_INODE_ZEROED 0x0004

Anyway, the substance of this patch is good. Just needs a little cosmetic cleanup.

--Sean