ext2fs: add read-only support for extended attributes

[kevlo]

I got the following compile error:

vfs_bio.o: In function `vfs_bio_brelse':
/usr/src/sys/kern/vfs_bio.c:4429: multiple definition of `vfs_bio_brelse'
ext2_vnops.o:/usr/src/sys/fs/ext2fs/ext2_vnops.c:94: first defined here
vfs_bio.o: In function `vfs_bio_set_flags':
/usr/src/sys/kern/vfs_bio.c:4436: multiple definition of `vfs_bio_set_flags'
ext2_vnops.o:/usr/src/sys/fs/ext2fs/ext2_vnops.c:101: first defined here

• Error code 1

sys/fs/ext2fs/ext2_vnops.c

90 ↗

(On Diff #26697)

As a comment, you have to remove vfs_bio_brelse() and vfs_bio_set_flags()

[fsu]

The wrappers for functions, which are not available in 11.0 were removed.

[pfg]

sys/fs/ext2fs/ext2_extattr.c

3 ↗

(On Diff #26697)

Like for the header file, it seems like this code's ownership corresponds only to Fedor.

sys/fs/ext2fs/ext2_extattr.h

3 ↗

(On Diff #26697)

Neither Zheng Liu or VyacheslavMatyushin have code here ... I think.

79 ↗

(On Diff #26697)

All these macros should probably have an EXT2_ prefix.

sys/fs/ext2fs/ext2_inode_cnv.c

128 ↗

(On Diff #26726)

Hmm .. we don't really support EXT2F_INCOMPAT_64BIT: last time I looked the format was incompatible with the regular format we use for 32 bits so our headers don't even try to handle it.

I think what you should look at here is EXT2F_ROCOMPAT_HUGE_FILE: and more specifically E2DI_HAS_HUGE_FILE()

sys/fs/ext2fs/ext2_vnops.c

90 ↗

(On Diff #26697)

Probably leftover code?

[pfg]

sys/fs/ext2fs/ext2_inode_cnv.c

128 ↗

(On Diff #26726)

Woa .. looking at he code just a few lines above ...

Please just move the i_facl assignation about 4 lines up and do it along with ip->i_blocks. You can even re-use the if()s.

[pfg]

Sorry to be so picky, the code is looking good. After the minor fixes I will approve and if kevin confirms it works we can commit it.

sys/fs/ext2fs/ext2_inode_cnv.c

128 ↗

(On Diff #26726)

While here, I just noticed we are not supporting e2di_uid_high and e2di_uid_high. These were introduced in ext4 and they fit fine in our i_uid and igid. Since we don't really ahve a check for ext4 but we check for features instead we can resue the HUGE_FILE check.

ext2_i2ei() would also need some adjustment if we do that but since we don't write ext4 that is not a problem for now.

Of course this is an edge case:,the filesystem would have to have a LOT of users to notice and some way to translate the uids and gids will have to be figured out after translation anyways. The fix is unrelated (you don't have to do it) but the fs part seems easy to fix.

[fsu]

Have some questions before upload next version of patch.

sys/fs/ext2fs/ext2_extattr.c

3 ↗

(On Diff #26697)

Same question as for ext2_extattr.h header file.

sys/fs/ext2fs/ext2_extattr.h

3 ↗

(On Diff #26697)

I am sorry, I did not understand how to add myself to license header correctly, or may be I should not do it here? Could you please guide me some what I should add to header?

79 ↗

(On Diff #26697)

Agree, will do it in next diff review iteration.

sys/fs/ext2fs/ext2_inode_cnv.c

128 ↗

(On Diff #26726)

I learned linux kernel sources for updating ram inode representation and found:

if (ext4_has_feature_64bit(sb))

		ei->i_file_acl |=
			((__u64)le16_to_cpu(raw_inode->i_file_acl_high)) << 32;

in linux/fs/ext4/inode.c

If EXT2F_INCOMPAT_64BIT does not supported on freebsd side should I remove this if()? And make ei->i_file_acl 32 bit?

128 ↗

(On Diff #26726)

I followed the struct fields declaration order, ok. Will do it in the next iteration.

sys/fs/ext2fs/ext2_vnops.c

90 ↗

(On Diff #26697)

Done.

[pfg]

sys/fs/ext2fs/ext2_extattr.c

3 ↗

(On Diff #26697)

Same answer just drop the other guys since they didn't write this code.

sys/fs/ext2fs/ext2_extattr.h

3 ↗

(On Diff #26697)

We have a guideline for new files:
https://www.freebsd.org/doc/en/articles/committers-guide/pref-license.html

In essence you just add

• Copyright (c) 2017 Fedor Uporov
• All rights reserved.

You shouldn't add other people's names unless they wrote some of the code and added their names.

sys/fs/ext2fs/ext2_inode_cnv.c

128 ↗

(On Diff #26726)

The field is there for all ext4fs so I think we can re-use the E2DI_HAS_HUGE_FILE() from other fields. We are also more efficient by avoideing repeated access to the structures.

We cannot do it unconditionally because ext2/3 fs may hold garbage in those fields. I think they had reserved space for implementating fragments or something else but we can't trust the values will be 0.

[pfg]

Add trasz as reviewer: with the addition of EAs, at least read-only for now, implementing ACLS becomes possible.

sys/fs/ext2fs/ext2_extattr.c

3 ↗

(On Diff #26697)

Just to clarify, adding your name here is fine: it's just rhat since these files are new, it doesn't seem you need to add other people.

[fsu]

Next iteration of review.

[pfg]

OK.. another iteration of knits :)

sys/fs/ext2fs/ext2_extattr.c

139 ↗

(On Diff #26843)

Minor knit: the * should be by the `header name var, right?

250 ↗

(On Diff #26843)

Minor knit: the braces are unnecessary here since there is only one statement. style(9) permits the braces but Removing the brackes makes the code a bit more readable.

318 ↗

(On Diff #26843)

Minor knit: same as above per the braces.

sys/fs/ext2fs/ext2_inode_cnv.c

123 ↗

(On Diff #26843)

This is correct, however please move the assignment to line 117 and re-use the if, like this:

ip->i_blocks = ei->e2di_nblock;
ip->i_facl = ei->e2di_facl;
if (E2DI_HAS_HUGE_FILE(ip)) {
        ip->i_blocks |= (uint64_t)ei->e2di_nblock_high << 32;
        ip->i_facl |= (uint64_t)ei->e2di_facl_high << 32;
        if (ei->e2di_flags & EXT4_HUGE_FILE)
            (etc)

We avoid accessing ip twice, this way :).

Just for the record, I forgot to explain, even with HUGE_FILE the high values are very likely to be zero but we can trust the value is not garbage.

sys/fs/ext2fs/ext2_vnops.c

1518 ↗

(On Diff #26843)

Please put this error check inside the if (line 1514). the error can only be triggered by that if so we can save that check for the not-if case. You will need braces of course :).

1559 ↗

(On Diff #26843)

Same as above, the error only needs to be checked if the previous conditional was true.

[fsu]

Minors correction.

[pfg]

I can't really test that it works as intented but it looks good to me. Approved.
Can it be MFC'd all the way to 10-stable? Kevin can you do the honors?
I am still setting up stuff here.

sys/fs/ext2fs/inode.h

108 ↗

(On Diff #26864)

All other comments are shorter and end with a period. Perhaps use EA instead of Extended Attribute if you need space.

[fsu]

Fix the comment.