Page MenuHomeFreeBSD

ext2fs: Add unsupported ext2fs features dmesg report
ClosedPublic

Authored by fsu on Jun 15 2017, 8:21 AM.

Details

Summary

It could be useful to user to know which ext2 features currently are not supported by driver.
If user will know it he can try use to use tune2fs from e2fsprogs to remove unsupported features.

Test Plan

Output example:

/dev/md0 is 1GB

root@user:~ # mkfs.ext4 /dev/md0
root@user:~ # mount -t ext2fs /dev/md0 /mnt
root@user:~ # dmesg

WARNING: mount of md0 denied due to unsupported optional features: 64bit root@user:~ # mkfs.ext4 -O ^64bit /dev/md0 root@user:~ # mount -t ext2fs /dev/md0 /mnt WARNING: R/W mount of md0 denied due to unsupported optional features: huge_file uninit_groups dir_nlink

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.Jun 15 2017, 8:21 AM
pfg edited edge metadata.Jun 15 2017, 2:25 PM

This makes sense, but I am not sure we want to use a struct for that.
Not an objection, just a comment.

sys/fs/ext2fs/ext2_vfsops.c
296 ↗(On Diff #29642)

This had some similarity with our nitems() macro (sys/param.h)

fsu updated this revision to Diff 29661.Jun 15 2017, 3:12 PM

Sorry, but I did not get your idea, what use instead of struct in this case?

fsu marked an inline comment as done.Jun 15 2017, 3:13 PM

Fix for() loops.

pfg added a comment.Jun 15 2017, 3:33 PM
In D11208#231970, @thisisadrgreenthumb_gmail.com wrote:

Sorry, but I did not get your idea, what use instead of struct in this case?

Just thinking out loud ...
I don't like that we already have those as #defines and now we add them as a struct that works more like an enum.

As I said, it's not really an objection, I have no ideas at this time.

fsu added a comment.Jun 15 2017, 3:48 PM

Ok, I will try locally implement it as enum, if it will be suitable, I will update review

fsu updated this revision to Diff 29706.Jun 16 2017, 7:43 AM

Swap features defines to enums.

pfg added a comment.Jun 26 2017, 3:09 PM

I have to say it looks elegant. The big doubt is if it's worth it.
kevlo@ uses it more than I do ... any opinion?

sys/fs/ext2fs/ext2_vfsops.c
309 ↗(On Diff #29706)

Wondering what happens if the number is not on the list: ext4 has been adding more features and it is likely we cannot keep the list up to date with new things coming.

fsu added a comment.Jun 27 2017, 6:17 AM

I should add, that it is not critical change, because it does not have influence to functionality. So, we can really skip it.
From other side, user will know from dmesg. why his volume does not mounted, or mounted in ro instead of rw, so he can try to use debugfs in some cases, or in more rare case try to add new functionality by himself;)

kevlo edited edge metadata.Jun 29 2017, 7:53 AM
In D11208#235170, @pfg wrote:

I have to say it looks elegant. The big doubt is if it's worth it.
kevlo@ uses it more than I do ... any opinion?

I think it's very useful. As we know, Ubuntu 16.04 and later versions use the
version 1.43 of e2fsprogs to create ext4 file system, which enables
64bit feature by default. After applying Fedor's patch I can see warning like:

WARNING: mount of da0s1 denied due to unsupported optional features: 64bit

Speaking of unsupported feature 64bit, I discussed this issue with pfg@,
Unfortunately we haven't documented this on the wiki.

kevlo accepted this revision.Jun 29 2017, 7:54 AM

Looks good to me, also tested on amd64.

This revision is now accepted and ready to land.Jun 29 2017, 7:54 AM
pfg accepted this revision.Jun 30 2017, 4:56 AM
pfg added a comment.Jun 30 2017, 5:02 AM

Actually, on second thoughts, I see no advantage in using the enums so I will be committing the previous version.

This revision was automatically updated to reflect the committed changes.