I am more prefer to initialize stack variables immediately after declaration, but, seems like it is not so perfect practice.
Ok.

Yep, accepted.
The code of extents part should be slightly cleaned up, it is my "nearest" future plans)
Also, it would be great to little bit rename variables inside the ext2_ei_csum(), but I will do it by myself later to avoid influence to logic.

In D13964#293380, @pfg wrote:

In D13964#293266, @fsu wrote:

In D13964#293256, @pfg wrote:

Hmm.. it seems like we are getting to a rather complete ext4 implementation, which is positive. On the downside the code is diverging from UFS. We just can't have both I guess.

There are another features like RO_COMPAT_QUOTA, INCOMPAT_INLINE_DATA, etc, but from my current point of view, it is not so, important, if it will be required to implement it. I think it a not a problem.
The only feature, which will be difficult to implement is RO_COMPAT_BIGALLOC, but I hope it will be not widely used by linux.

I haven't looked at ext4 for a while but most of those features seem to require some obscure collaboration from the kernel, that is out of the scope of the driver. There's also the issue that ost of the interesting enhancements in linux assume there is journalling.

I am not sure, that journal feature is required for non-native file system, also it will be difficult to test it.

In any case , let me mention that EXT4F_RO_INCOMPAT_SUPP was created as a dirty hack: to workaround for features that we were not planning to support at all but that we needed to fake in order to read the partition.
We should remove EXT4F_RO_INCOMPAT_SUPP as it has fulfilled its purpose.

Yep, I will prepare the commit to remove it.

Have you tested performance improvements from these features? They could be the basis for a conference paper :).

I am not sure, that any of implemented ext4 features will provide the performance improvements, because these features a not targeted for it, so performance measurement is not actual.

Hmm.. right. Most of the features are actually enhancements for fsck: checksumming will probably hit performance.

Also, if you mean, comparing it with linux driver, freebsd will fail in all cases) because, it is "native" file-system for linux and freebsd driver implementation is slightly simplified comparing with it.

The objectives have been different but at some point our ext2 had the same basic performance of UFS1 and linux's ext2, which was actually faster than ext3. We have never made comparisons with ext4.

The comparing performance of ufs and extfs is an interesting. I sure, that ufs will be faster, but how many times... May be will make some measurements little bit later.

In D13964#293256, @pfg wrote:

Hmm.. it seems like we are getting to a rather complete ext4 implementation, which is positive. On the downside the code is diverging from UFS. We just can't have both I guess.

There are another features like RO_COMPAT_QUOTA, INCOMPAT_INLINE_DATA, etc, but from my current point of view, it is not so, important, if it will be required to implement it. I think it a not a problem.
The only feature, which will be difficult to implement is RO_COMPAT_BIGALLOC, but I hope it will be not widely used by linux.

In any case , let me mention that EXT4F_RO_INCOMPAT_SUPP was created as a dirty hack: to workaround for features that we were not planning to support at all but that we needed to fake in order to read the partition.
We should remove EXT4F_RO_INCOMPAT_SUPP as it has fulfilled its purpose.

Yep, I will prepare the commit to remove it.

Have you tested performance improvements from these features? They could be the basis for a conference paper :).

I am not sure, that any of implemented ext4 features will provide the performance improvements, because these features a not targeted for it, so performance measurement is not actual.
Also, if you mean, comparing it with linux driver, freebsd will fail in all cases) because, it is "native" file-system for linux and freebsd driver implementation is slightly simplified comparing with it.

In D13810#289909, @pfg wrote:

In D13810#289828, @fsu wrote:

In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

All fields, which are mentioned in SUMMERY, should be written to disk. If it will not be updated, we will have errors reported by e2fsck, or bunch of error messages from driver on linux side.

I am not complaining at all about what is being checksummed: I understand you want to checksum everything that is needed. The main doubt is when to checksum.

There are two possibilities: you either checksum everything as soon as you get it or you checksum at the last moment before writing to disk. Both options have their advantages It seems like the checksumming is happening all over the filesystem so you are doing the former, I was only wondering if we could do all the checksums in a single place, right when we are converting to the ext2fs format for writing.

I am not sure if such thing is poosible though, and your change is not incorrect so I'll let you think about it but I'll approve the patch.

In D13810#289543, @pfg wrote:

A thought : would it be possible to checksum only what we are going to write to disk?
Data will not, generally, corrupt in memory and the kernel has no use for the checksums at runtime so we only need to write stuff when it's going to be converted to the on-disk format in ext2_inode_cnv.c.

• Declare struct fuse_listxattr_* to not use fuse_getxattr_* in case of list().
• Remove ENOSYS error check in case of second fdisp_wait_answ() call under list().

More deep reinitialization of struct fuse_dispatcher fdi before second call to fdisp_wait_answ() under fuse_vnop_listextattr().

Ok, let's try to proceed...
Here is very initial version of vfs_extattr.c, where the io faults are handled.
It even somehow does not works with ufs, but works with ext2fs, I did not dig in this problem for now, because it is needed to figure out the direction and next steps for extattrs topic, the testing procedure will include all available filesystems with extattrs support and will be developed later.
So, @kib, could you please review it and provide the comments.

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

Also, cosmetic changes are extracted in https://reviews.freebsd.org/D13737.

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.

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.

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

Add linux_list stack variable to make listxattr() function more readable.

Ok), logic fixes are extracted in https://reviews.freebsd.org/D13528.

It is too difficult to me to implement it now, so, seems like, I have a questions to @kib.
As I currently understand, what we want to achive to is, not allow page faults to happen during extattr operations, is insert some sort of logic, like happen between vn_io_fault() and vn_read/vn_write or VOP_READ/VOP_WRITE.
It should be inserted in the extattr_*vp() between uio initialization and VOP_*EXTATTR call.

Seems like, it is not clearly understandable what was fixed. Ok, let me explain:

• The was a bug in list() function when uio was not passed, the size of linux list (not converted to bsd was returned).
• Case when neither uio nor size were not passed to list() and get(), this logic could be used to check does extattr with some key exist or not.

It is difficult to find a value to restrict extattr value size. Because, as I know, in case of ntfs and hfs, the extattr sizes are "infinity".
So I decided to use the same check for uio size, so we will have two checks one on extattr_*vp() layer and one on upper layer on system call pass.
My opinio is that, the check in the extattr_*vp() layer is needed because this function will be exported as part of kernel interface.

Ok, here the buffer size checks in the extattr_*_vp() are moved out from vnode locks:
https://reviews.freebsd.org/D13405

Switch extattr_*_vp() to use input buffers from kernelspace.

Ok, if I understood correctly, the idea is to use vp_extattr_*() to use in case of calls from filesystem, and extattr_*_vp() - in case of system calls.
Let's get back to:
https://reviews.freebsd.org/D13267
I want to solve the interface questions before getting back to linuxulator xattrs implementation.
Robert, you are already invited to the review above.

Updated after discussion with @rwatson in the:
https://reviews.freebsd.org/D13209

Ok, I was wrong with with MAC and user extattrs, the MAC checking functions should be restored.
Also, the general question, just to be sure, does MAC check is responsibility of VFS or the filesystem?

Ok, thanks for suggest.
My choice was closer to (1), because it was not required to modify kernel code, so it was faster in case of debugging, because it is not needed to rebuild kernel every time. But (2) is more clear.
So, I will not commit https://reviews.freebsd.org/D13267 for now, but I will update it little bit later together with current review.

As could be seen from https://reviews.freebsd.org/D13267,
all extattr_*_vp() functions were exported, but, the only extattr_delete_vp() is used by current review iteration.
To use all the extattr_*_vp() functions I need to pull up the vnode locking logic out of this functions,
because I need one vnode lock per one system call, as I understand.
So, I should decide, where is the better way:

1. Use the current way, where from linux_*_common() I will call VOP_*'s.
2. Try to make extattr_*_vp() from vfs_extattr.c more generic. I mean remove vnode locking logic and add additional checks.

In D13209#276930, @cem wrote:

In D13209#276929, @pfg wrote:

In one of the early comments Fedor attached a shell script with the tests extracted from the Linux Test Project.
Alternatively you can check all the test suite here:

https://linux-test-project.github.io/

There are several at testcases/kernel/syscalls

So I need a Linux system to compile them, right? Do they compile statically or do I need some Linux shared libraries as well? Thanks!

Fix different marks from pfg and cem + split the patch additionaly (see comment above).

Make additional commit split:
https://reviews.freebsd.org/D13267

In case of linux acls worked thru xattr syscalls, but with different namespace:
% strace setfacl -m m::rx ./FILE0
...
getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=64*1024}) = 0
lstat("./FILE0", {st_mode=S_IFREG|0654, st_size=5, ...}) = 0
getxattr("./FILE0", "system.posix_acl_access", "\2\0\0\0\1\0\6\0\377\377\377\377\4\0\6\0\377\377\377\377\20\0\5\0\377\377\377\377 \0\4\0\377\377\377\377", 132) = 36
exit_group(0)

Test helper script for LTP running.

Everything noticed above will be fixed before commit.
Conrad, I should say you thanks for your review and time, which you spent.
Also, it would be great if you can, not review, but just look to ext2fs extents diff, where you was invited.

• Add name length check.
• Add comments to function arguments.
• Some refactoring.

• Swap inplace names conversion to malloced.
• Reorder names conversion function arguments.
• Fix style.

• remove extattr namespaces sysctl.
• revet err check according style(9)
• swap printf() to debug_printf()

In D12087#259854, @pfg wrote:

Add cem@, he may want to review it before it lands.

The diff updated.
Also, it would be great to decide something with this new namespaces sysctl, leave it or remove.
May be somebody know some experienced persons, who worked with fuse fs, and can add them to this review, to clarify this question.

Here are some fixes according @cem's suggestions.

Update diff to have full context.

Update diff to have full context.

Move huge_file discussions here:
https://reviews.freebsd.org/D12131

Ok, I am not sure that it is possible to overflow this variable.
We have 16 bit from e2di_nblock_high and 32 bit from e2di_nblock.
So:
pow(2, 48) * 512 / pow(1024,4) = 131072
Where result is in TBs.
The we can see from https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout (File System Maximums table)
That for 64k blocks in both 32bit and 64bit cases we have maximum file size 256TB.

In D12087#251372, @pfg wrote:

I might have missed it, but have you considered the case that requires writing EXT4_HUGE_FILEs?

Yep, you can see it from attached test script, the mkfs option -O huge_file was added in all cases.

Of course, I am waiting for kevlo@ to chime in as he has more experience with this than me.

Me too:)

Remove forgotten unneeded stuff.

Fix small knits.

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.

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

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.

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;)

As, I learned linux sources you can use uid/gid high without E2DI_HAS_XTIME(ip) check, because it depends only from mount option on linux side, not from inode size.
So, seems like

ip->i_uid |= (uint32_t)ei->e2di_uid_high << 16;
ip->i_gid |= (uint32_t)ei->e2di_gid_high << 16;