Page MenuHomeFreeBSD

ext2fs: Add 64bit feature support
ClosedPublic

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

Details

Test Plan

The script

was used to test the feature.
Additional test scripts.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fsu created this revision.Jul 8 2017, 5:02 PM
pfg edited edge metadata.Jul 9 2017, 4:01 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.

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

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

pfg added a comment.EditedJul 9 2017, 6:06 PM
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.

fsu added a comment.Jul 9 2017, 6:17 PM

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

pfg added a comment.Jul 9 2017, 9:41 PM
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 removed a reviewer: rwatson.Aug 2 2017, 5:06 PM

ping @kevlo: any comments?

fsu added a comment.Aug 2 2017, 8:57 PM

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 added a comment.Aug 3 2017, 3:40 PM
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 this revision to Diff 36802.Dec 20 2017, 9:37 AM
fsu edited the test plan for this revision. (Show Details)

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

pfg removed a reviewer: trasz.Dec 21 2017, 7:50 PM

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

fsu added a comment.Dec 22 2017, 7:37 AM

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 added a comment.Dec 22 2017, 2:43 PM
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 added a comment.Dec 22 2017, 5:33 PM

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 added a comment.Dec 22 2017, 5:52 PM
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
  1. 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.

fsu updated this revision to Diff 37402.Jan 2 2018, 9:02 AM

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.

kevlo accepted this revision.Jan 3 2018, 8:56 AM

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

This revision is now accepted and ready to land.Jan 3 2018, 8:56 AM
pfg accepted this revision.Jan 3 2018, 3:02 PM

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 ↗(On Diff #37402)

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

This revision was automatically updated to reflect the committed changes.