ext2fs: Add 64bit feature support
ClosedPublic
Actions

Authored by fsu on Jul 8 2017, 5:02 PM.

Details

Reviewers

pfg
kevlo

Commits

rS327584: Add 64bit feature support.

Test Plan

The script

64bit_test.sh1 KBDownload

was used to test the feature.
Additional

tests_64bit.tar.gz2 KBDownload

test scripts.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

fsu created this revision.Jul 8 2017, 5:02 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 8 2017, 5:02 PM

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 ↗	(On Diff #30579)	How about making these inlines in a header, perhaps ext2fs.h?
sys/fs/ext2fs/ext2fs.h
380 ↗	(On Diff #30579)	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.

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 added inline comments.Jul 9 2017, 5:33 PM

sys/fs/ext2fs/ext2fs.h
380 ↗	(On Diff #30579)	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 added inline comments.Jul 9 2017, 5:35 PM

sys/fs/ext2fs/ext2_alloc.c
455 ↗	(On Diff #30579)	I tried to keep ext2fs.h more clean, but ok, will move it.

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 added a comment.Jul 9 2017, 5:45 PM

This comment was removed by pfg.

pfg added inline comments.Jul 9 2017, 5:46 PM

sys/fs/ext2fs/ext2_alloc.c
455 ↗	(On Diff #30579)	Hmm.. perhaps the extents header, then?
sys/fs/ext2fs/ext2fs.h
380 ↗	(On Diff #30579)	OK, I missed that part.

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 added inline comments.Jul 9 2017, 6:11 PM

sys/fs/ext2fs/ext2fs.h
380 ↗	(On Diff #30579)	Perhaps a comment in e2fs_cgload() is pertinent.

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

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.

ping @kevlo: any comments?

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.

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.

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

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

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.

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?

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.

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:

A straight comment.
the code already has some

#ifdef EXT2FS_DEBUG
#endif

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 added inline comments.Dec 25 2017, 2:20 AM

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.

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 added inline comments.Jan 2 2018, 3:57 PM

sys/fs/ext2fs/ext2_alloc.c
566 ↗	(On Diff #37402)	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 added inline comments.Jan 3 2018, 7:44 AM

sys/fs/ext2fs/ext2_alloc.c
566 ↗	(On Diff #37402)	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.