diff mbox series

mke2fs: allow 64bit feature without extents

Message ID 1553246667-53793-1-git-send-email-adilger@dilger.ca
State New
Headers show
Series mke2fs: allow 64bit feature without extents | expand

Commit Message

Andreas Dilger March 22, 2019, 9:24 a.m. UTC 
From: Andreas Dilger <adilger@whamcloud.com>

The 64bit feature should be allowed without extents to for 32-bit
metadata_csum checksums to be stored in the group descriptor.
Change the extents check to check for more than 2^32 blocks instead
of the 64bit feature flag.  This also avoids warnings later if the
metadata_csum feature is enabled on a filesystem without 64bit.

Consolidate repeated 64bit feature checks into one check.

The fs_blocks_count value is always set when parse_fs_type() is
called, so no need to check if it is zero.

Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Reviewed-by: Li Dongyang <dongyangli@ddn.com>
Whamcloud-bug-id: https://jira.whamcloud.com/browse/LU-11609
Change-Id: Ie73ff0ed50cfed5d6a9596260c6b6dc32d3ebbe5
Reviewed-on: https://review.whamcloud.com/33897
---
 misc/mke2fs.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Theodore Ts'o April 6, 2019, 9:52 p.m. UTC | #1 
On Fri, Mar 22, 2019 at 03:24:27AM -0600, Andreas Dilger wrote:
> From: Andreas Dilger <adilger@whamcloud.com>
> 
> The 64bit feature should be allowed without extents to for 32-bit
> metadata_csum checksums to be stored in the group descriptor.
> Change the extents check to check for more than 2^32 blocks instead
> of the 64bit feature flag.  This also avoids warnings later if the
> metadata_csum feature is enabled on a filesystem without 64bit.

So what worries me about this change is if extents aren't enabled, and
we do an online (or off-line) resize such that we now have > 2^32
blocks, things are going to get problematic.  Even if the resize
operation sets the extent feature (which I don't think it will), the
problem is if you have an existing file which is using indirect
blocks, and you try to extend the file and there is simply no blocks
under the 2^32 cutoff, what then?  Some files might get ENOSPC errors
while others will work just fine.

With current versions of e2fsprogs we enable the 64-bit (and
metadata_csum) feature by default, which should avoid all of these
problems.  Sure, that's not going to help older file systems --- but
this patch to mke2fs isn't going to help them, either.

It simplifies our test matrix to simply require the extents feature if
the 64-bit feature is desired.  I'm not sure I see the value in
relaxing this requirement.  Is there a reason why you specifically
want 64bit && !extents && metadata_csum?

					- Ted
Andreas Dilger April 8, 2019, 11:15 a.m. UTC | #2 
On Apr 6, 2019, at 3:52 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> On Fri, Mar 22, 2019 at 03:24:27AM -0600, Andreas Dilger wrote:
>> From: Andreas Dilger <adilger@whamcloud.com>
>> 
>> The 64bit feature should be allowed without extents to for 32-bit
>> metadata_csum checksums to be stored in the group descriptor.
>> Change the extents check to check for more than 2^32 blocks instead
>> of the 64bit feature flag.  This also avoids warnings later if the
>> metadata_csum feature is enabled on a filesystem without 64bit.
> 
> So what worries me about this change is if extents aren't enabled, and
> we do an online (or off-line) resize such that we now have > 2^32
> blocks, things are going to get problematic.  Even if the resize
> operation sets the extent feature (which I don't think it will), the
> problem is if you have an existing file which is using indirect
> blocks, and you try to extend the file and there is simply no blocks
> under the 2^32 cutoff, what then?  Some files might get ENOSPC errors
> while others will work just fine.
> 
> With current versions of e2fsprogs we enable the 64-bit (and
> metadata_csum) feature by default, which should avoid all of these
> problems.  Sure, that's not going to help older file systems --- but
> this patch to mke2fs isn't going to help them, either.
> 
> It simplifies our test matrix to simply require the extents feature if
> the 64-bit feature is desired.  I'm not sure I see the value in
> relaxing this requirement.  Is there a reason why you specifically
> want 64bit && !extents && metadata_csum?

Mostly we want to enable metadata_csum on Lustre metadata targets, which
are typically always under 16TB in size, and since the MDT doesn't store
much file data, it is more efficient to use block-mapped directories
rather than extent-mapped, since directory blocks are typically allocated
randomly, so extents are not really useful or needed.

Currently the "64bit" feature is needed to get large group_descriptor
entries to store the 32-bit metadata_csum fields to enable metadata_csum,
which is the reason for this patch.  If it were possible to enable 64-byte
struct ext4_group_desc without the 64bit feature so that metadata_csum
could store 32-bit checksums, that would be another solution instead of
requiring 64bit to be enabled without extents.

Cheers, Andreas
diff mbox series

Patch

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index f05003f..267e919 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1985,8 +1985,7 @@  profile_error:
 	 * be appropriately configured.
 	 */
 	fs_types = parse_fs_type(fs_type, usage_types, &fs_param,
-				 fs_blocks_count ? fs_blocks_count : dev_size,
-				 argv[0]);
+				 fs_blocks_count, argv[0]);
 	if (!fs_types) {
 		fprintf(stderr, "%s", _("Failed to parse fs types list\n"));
 		exit(1);
@@ -2118,29 +2117,30 @@  profile_error:
 	 * We now need to do a sanity check of fs_blocks_count for
 	 * 32-bit vs 64-bit block number support.
 	 */
-	if ((fs_blocks_count > MAX_32_NUM) &&
-	    ext2fs_has_feature_64bit(&fs_param))
-		ext2fs_clear_feature_resize_inode(&fs_param);
-	if ((fs_blocks_count > MAX_32_NUM) &&
-	    !ext2fs_has_feature_64bit(&fs_param) &&
-	    get_bool_from_profile(fs_types, "auto_64-bit_support", 0)) {
-		ext2fs_set_feature_64bit(&fs_param);
-		ext2fs_clear_feature_resize_inode(&fs_param);
-	}
-	if ((fs_blocks_count > MAX_32_NUM) &&
-	    !ext2fs_has_feature_64bit(&fs_param)) {
-		fprintf(stderr, _("%s: Size of device (0x%llx blocks) %s "
+	if (fs_blocks_count > MAX_32_NUM) {
+		if (!ext2fs_has_feature_64bit(&fs_param) &&
+		    get_bool_from_profile(fs_types, "auto_64-bit_support", 0))
+			ext2fs_set_feature_64bit(&fs_param);
+
+		if (ext2fs_has_feature_64bit(&fs_param)) {
+			ext2fs_clear_feature_resize_inode(&fs_param);
+		} else {
+			fprintf(stderr,
+				_("%s: Size of device (0x%llx blocks) %s "
 				  "too big to be expressed\n\t"
 				  "in 32 bits using a blocksize of %d.\n"),
-			program_name, fs_blocks_count, device_name,
-			EXT2_BLOCK_SIZE(&fs_param));
-		exit(1);
+				program_name, fs_blocks_count, device_name,
+				EXT2_BLOCK_SIZE(&fs_param));
+			exit(1);
+		}
 	}
+
 	/*
 	 * Guard against group descriptor count overflowing... Mostly to avoid
 	 * strange results for absurdly large devices.
 	 */
-	if (fs_blocks_count > ((1ULL << (fs_param.s_log_block_size + 3 + 32)) - 1)) {
+	if (fs_blocks_count >
+	    ((1ULL << (fs_param.s_log_block_size + 3 + 32)) - 1)) {
 		fprintf(stderr, _("%s: Size of device (0x%llx blocks) %s "
 				  "too big to create\n\t"
 				  "a filesystem using a blocksize of %d.\n"),
@@ -2213,13 +2213,13 @@  profile_error:
 		fs_param.s_feature_compat = 0;
 		fs_param.s_feature_ro_compat &=
 					EXT4_FEATURE_RO_COMPAT_METADATA_CSUM;
- 	}
+	}
 
 	/* Check the user's mkfs options for 64bit */
-	if (ext2fs_has_feature_64bit(&fs_param) &&
+	if (fs_blocks_count > MAX_32_NUM &&
 	    !ext2fs_has_feature_extents(&fs_param)) {
-		printf("%s", _("Extents MUST be enabled for a 64-bit "
-			       "filesystem.  Pass -O extents to rectify.\n"));
+		printf("%s", _("Extents MUST be enabled for filesystems with "
+			       "over 2^32 blocks. Use '-O extents' to fix.\n"));
 		exit(1);
 	}