ext2fs: Add 64bit feature support

[fsu]

Test Plan

The script

64bit_test.sh1 KBDownload

was used to test the feature.
Additional

tests_64bit.tar.gz2 KBDownload

test scripts.

[pfg]

You can use the existing ext4gd fields but for the others we need to have extra care.

I think/suspect the 64BIT flag needs a 64 bit system and basically breaks the on-disk compatibility: I recall it was causing issues with some bootloaders.

To be able to test the patch for real you need a huge (>16T) filesystem. IMHO enabling the feature for smaller filesystems i nonsense, but it is something that is not under our control.

sys/fs/ext2fs/ext2_alloc.c

455

How about making these inlines in a header, perhaps ext2fs.h?

sys/fs/ext2fs/ext2fs.h

380

This is the part I am afraid of: these fields don't exist on ext2/3.
Writing them, or even reading them, may be dangerous.

I think we need a new struct: ext4_gd.

[fsu]

In D11530#238803, @pfg wrote:

You can use the existing ext4gd fields but for the others we need to have extra care.

I think/suspect the 64BIT flag needs a 64 bit system and basically breaks the on-disk compatibility: I recall it was causing issues with some bootloaders.

The feature, just called 64bit, but, as I told, it is does not related to base types sizes, it is only about increasing size of struct ext2_gd, to add additional fields. Could you please describe about bootloadres? Do you mean, that FreeBSD ext2fs uses to mount boot volume somewhere?

To be able to test the patch for real you need a huge (>16T) filesystem. IMHO enabling the feature for smaller filesystems i nonsense, but it is something that is not under our control.

Here, you are completely right, only one way to test blocks allocation outside of UINT_MAX range is to use large volume, filled with real data, I am thinking about renting FreeBSD server somewhere, because I do not have access to large drives with TB sizes.

[fsu]

sys/fs/ext2fs/ext2fs.h

380

I will try to explain, what is going on.
The ondisk size of the struct is dependent only from 64bit feature flag. In case of ext2/ext3, we will read only first 32 bytes of structure,
So, the main "magic" related to it have place around e2fs_cgload() macro.

[fsu]

sys/fs/ext2fs/ext2_alloc.c

455

I tried to keep ext2fs.h more clean, but ok, will move it.

[pfg]

In D11530#238821, @thisisadrgreenthumb_gmail.com wrote:

In D11530#238803, @pfg wrote:

You can use the existing ext4gd fields but for the others we need to have extra care.

I think/suspect the 64BIT flag needs a 64 bit system and basically breaks the on-disk compatibility: I recall it was causing issues with some bootloaders.

The feature, just called 64bit, but, as I told, it is does not related to base types sizes, it is only about increasing size of struct ext2_gd, to add additional fields. Could you please describe about bootloadres? Do you mean, that FreeBSD ext2fs uses to mount boot volume somewhere?

Not a problem on FreeBSD since we don't regularly boot from ext2. The problem for external bootloaders are hinted here:

https://mail.gnome.org/archives/commits-list/2016-June/msg01079.html

[pfg]

This comment was removed by pfg.

[pfg]

sys/fs/ext2fs/ext2_alloc.c

455

Hmm.. perhaps the extents header, then?

sys/fs/ext2fs/ext2fs.h

380

OK, I missed that part.

[pfg]

In D11530#238821, @thisisadrgreenthumb_gmail.com wrote:

In D11530#238803, @pfg wrote:

You can use the existing ext4gd fields but for the others we need to have extra care.

I think/suspect the 64BIT flag needs a 64 bit system and basically breaks the on-disk compatibility: I recall it was causing issues with some bootloaders.

The feature, just called 64bit, but, as I told, it is does not related to base types sizes, it is only about increasing size of struct ext2_gd, to add additional fields. Could you please describe about bootloadres? Do you mean, that FreeBSD ext2fs uses to mount boot volume somewhere?

To be able to test the patch for real you need a huge (>16T) filesystem. IMHO enabling the feature for smaller filesystems i nonsense, but it is something that is not under our control.

Here, you are completely right, only one way to test blocks allocation outside of UINT_MAX range is to use large volume, filled with real data, I am thinking about renting FreeBSD server somewhere, because I do not have access to large drives with TB sizes.

I don't have such big disks either: perhaps with a cloud provider (Amazon or Azure), or I think ZFS a zvol with compression could be made to emulate those.

To be honest, the interesting test is to try such big disks in a 32bit OS but those are not very common in cloud environments.

[pfg]

sys/fs/ext2fs/ext2fs.h

380

Perhaps a comment in e2fs_cgload() is pertinent.

[fsu]

Ha...! The zfs with compressed zvol is good idea.

[pfg]

In D11530#238833, @thisisadrgreenthumb_gmail.com wrote:

Ha...! The zfs with compressed zvol is good idea.

Oops, yes ... I meant zvol. This kind of testing was the main reason why they were created.

[pfg]

ping @kevlo: any comments?

[fsu]

Sorry for long delay from my side, but, seems like, this review revision should be frozen. It contain some "fundamental" problems, which can not be resolved directly. I found it, when started to play with large zvols. So, I need some time, and I can not estimate it for now.

[pfg]

In D11530#245279, @thisisadrgreenthumb_gmail.com wrote:

Sorry for long delay from my side, but, seems like, this review revision should be frozen. It contain some "fundamental" problems, which can not be resolved directly. I found it, when started to play with large zvols. So, I need some time, and I can not estimate it for now.

I am very happy that you are using zvols to test this. Take your time.

[fsu]

Updated version of 64bit feature.
It was required to implement extents feature before add it.

[pfg]

What is the status .. is this ready or just an update for the record?

[fsu]

I can confirm that it is ready to commit to head with long MFC for additional testing.
I made normal tests using fsx/fstorture + tests for modes when blocks are allocated out of UINT_MAX range.
More detailed information could be found in second test scripts attachment.
The reason why it was done in two phases (I mean a bunch of time between first and second review revisions),
because this feature heavily related with extents.
There were two ways to integrate extents and 64bit:

• RO extents, then RO 64bit, then RW extents, then RW 64bit.
• RO extents, then RW extetns, then RW 64bit.

So, the second way was chosen, because I decided, that it is more difficult to test RO feature implementation, than RW.
The next required feature is metadata_csum. When it will be integrated, it will be possible to switch to meta_bg/flex_bg features set.

[pfg]

In D11530#284091, @fsu wrote:

I can confirm that it is ready to commit to head with long MFC for additional testing.
I made normal tests using fsx/fstorture + tests for modes when blocks are allocated out of UINT_MAX range.
More detailed information could be found in second test scripts attachment.
The reason why it was done in two phases (I mean a bunch of time between first and second review revisions),
because this feature heavily related with extents.
There were two ways to integrate extents and 64bit:

• RO extents, then RO 64bit, then RW extents, then RW 64bit.
• RO extents, then RW extetns, then RW 64bit.

So, the second way was chosen, because I decided, that it is more difficult to test RO feature implementation, than RW.
The next required feature is metadata_csum. When it will be integrated, it will be possible to switch to meta_bg/flex_bg features set.

What I find confusing is that there is "force_64bit_alloc", labelled for testing/debugging only and a couple of TODO things that give the impresion that the implementation is still not ready.

fs/ext2fs/ext2_alloc.c

415 ↗

(On Diff #36802)

Unnecessary change.

574 ↗

(On Diff #36802)

There is no chance for these to overflow.

FWIW, I unsigned many of these for consistency with UFS where there can be overflows but on ext2fs that can't happen.

fs/ext2fs/ext2_vfsops.c

325 ↗

(On Diff #36802)

64k or 64bit?

[fsu]

What I find confusing is that there is "force_64bit_alloc", labelled for testing/debugging only and a couple of TODO things that give the impresion that the implementation is still not ready.

Yep, seems like I should ask the suggestion here...
Let me explain. This is hack to allow to allocate blocks outside of UINT_MAX range, which was used for testing purposes..
I mean, it is too difficult to test 16TB volume in case of ENOSPC without it.
So, I want to store it in svn history, to explain, how it was tested.
My plan was to commit main patch and then remove this hack. Or it is too dirty behavior to store stuff like this in the history?

fs/ext2fs/ext2_alloc.c

415 ↗

(On Diff #36802)

Will be reverted.

fs/ext2fs/ext2_vfsops.c

325 ↗

(On Diff #36802)

Seems like, I will separate it from this change, it is 64k.
This check is used to not allow mount in case we have 64k block size and extents features combination in rw mode.

[pfg]

In D11530#284219, @fsu wrote:

What I find confusing is that there is "force_64bit_alloc", labelled for testing/debugging only and a couple of TODO things that give the impresion that the implementation is still not ready.

Yep, seems like I should ask the suggestion here...
Let me explain. This is hack to allow to allocate blocks outside of UINT_MAX range, which was used for testing purposes..
I mean, it is too difficult to test 16TB volume in case of ENOSPC without it.
So, I want to store it in svn history, to explain, how it was tested.
My plan was to commit main patch and then remove this hack. Or it is too dirty behavior to store stuff like this in the history?

svn history is not the right place to put this hack ...
If you are not likely to use these ever again you can obviate them.
If you want to keep some of that around, more elegant options are:

1. A straight comment.
2. the code already has some

#ifdef EXT2FS_DEBUG
#endif

3. both of the above.

fs/ext2fs/ext2_vfsops.c

325 ↗

(On Diff #36802)

Aha! Yes, this is a different issue so it should be in a different commit.

[kevlo]

fs/ext2fs/ext2fs.h

331 ↗

(On Diff #36802)

Don't remove EXT2F_INCOMPAT_FLEX_BG. It still couldn't mount a ext4 file
system that is created from scratch because the flex_bg feature is enabled by
default. Works for me on amd64 apart from a EXT2F_INCOMPAT_FLEX_BG thing.

[fsu]

Diff changes:

• Restore FLEX_BG check.
• Remove 64k block size/extents mount check.
• Remove 64bit feature testing hack.
• Revert newline in the ext2_alloc.c

[pfg]

sys/fs/ext2fs/ext2_alloc.c

566

Bumping these serves no purpose:
On line 485, avgbfree cannot overflow: e2fs_fbcount is uint32_t.
On line 520,minbfree is smaller that avgbfree.

Bumping these is unecessary, unless you want to set them to uint32_t.

[fsu]

sys/fs/ext2fs/ext2_alloc.c

566

Little bit incorrect.
avgbfree same as minbfree can overflow because e2fs_fbcount is 64bit.
This value come from struct m_ext2fs, which is in-memory representation of superblock.
From other point, the struct ext2fs (on-disk representation) have variable with same name, but uint32.
Seems like, the confusion is here.

[kevlo]

Thanks for getting this done. It works for me on amd64.

[pfg]

Approved: however since this depend on the 64 bit inode changes, my guess is that it cannot be merged to stable as-is.

sys/fs/ext2fs/ext2_alloc.c

566

Oops.. dejavu moment here.
The opengrok instant that I use never caught up with this change.