In D19848#426959, @gnn wrote:

Some general comments.

1. Can you move all the DEFINE statements to the top of the file rather than interspresing them throughout? I know you're defining them near use but the general way we have coded these is to put the DECLARES and DEFINES at the top of the files.

Ok, I will add this change before landing.

2. The name "trace" is going to be a bit confusing to users as they already think they're tracing the system. You could drop the "trace" altogether and see if that makes sense.

I prefer to leave it as is to keep consistency with fusefs dtrace probes naming. See: https://reviews.freebsd.org/D19667

Otherwise this looks like a great addition, and thank you for doing it.

In D19848#426730, @pfg wrote:

I still hope to hear from the DTrace experts, but I like this very much. Thanks!

In D19599#423359, @kib wrote:

In D19599#423338, @fsu wrote:

In D19599#423334, @markj wrote:

In D19599#423330, @fsu wrote:

Hi, Mark.

Thanks a lot for adding me to review.
But could you please provide some information, how it is possible to test FIOSEEKDATA/FIOSEEKHOLE from user-space side.
I mean, did you write some user-space code to test it or some unit-tests exist somewhere?

I did not modify SEEK_HOLE. I tested SEEK_DATA with a program that truncates an empty file to a random length, and then calls SEEK_DATA on the file with a random starting offset, and verifies that the value returned is the starting offset of the last LBN in the file, since UFS seems to always put a data block at the end of a sparse file, even if the inode size % block size is 0. I also ran a private instance of syzkaller on the patch and verified that it exercises most of the branches in the new function (currently it cannot trigger errors from readindir(), for example).
I started writing some tests, but discarded them. It is hard to write useful tests since the implementation of file holes is highly filesystem-dependent, and even for UFS there is no blessed procedure for creating holes in a file AFAIK.

What you described above *is* the correct way to create holes on UFS. Seek after the EOF and write single byte, then all the content between old EOF and new block containing that byte is hole. The only detail is that if the byte is in direct block, then you might get fragment at the end, instead of the full block (does not matter for the tests).

So any tests for this will be coupled to filesystem internals.

Also, it is interesting how faster it became.

I did not attempt to compare them. The old algorithm is O(size of file) and the new one is effectively constant-time as long as the inode does not reference indirect blocks that are full of zeroes. With a sufficiently large sparse file the old algorithm will not complete in any reasonable amount of time.

I expected that some user-space tests exist, which could be possible to apply to ext2fs to ensure, that the implementation is compatible with ufs.

https://gist.github.com/5319d0d7d52e0f9199ddf2b5a75a6438
https://gist.github.com/a4f9c80a7699151e7f2e5ba051740aa7
This is what I used for vn_bmap_seekhole() implementation.

Ok, thank you. This is, what I tried to find, to avoid implement it from scratch.

But ok, I will try to find something and check it on ufs too.

Thanks.

In D19599#423334, @markj wrote:

In D19599#423330, @fsu wrote:

Hi, Mark.

Thanks a lot for adding me to review.
But could you please provide some information, how it is possible to test FIOSEEKDATA/FIOSEEKHOLE from user-space side.
I mean, did you write some user-space code to test it or some unit-tests exist somewhere?

I did not modify SEEK_HOLE. I tested SEEK_DATA with a program that truncates an empty file to a random length, and then calls SEEK_DATA on the file with a random starting offset, and verifies that the value returned is the starting offset of the last LBN in the file, since UFS seems to always put a data block at the end of a sparse file, even if the inode size % block size is 0. I also ran a private instance of syzkaller on the patch and verified that it exercises most of the branches in the new function (currently it cannot trigger errors from readindir(), for example).
I started writing some tests, but discarded them. It is hard to write useful tests since the implementation of file holes is highly filesystem-dependent, and even for UFS there is no blessed procedure for creating holes in a file AFAIK. So any tests for this will be coupled to filesystem internals.

Also, it is interesting how faster it became.

I did not attempt to compare them. The old algorithm is O(size of file) and the new one is effectively constant-time as long as the inode does not reference indirect blocks that are full of zeroes. With a sufficiently large sparse file the old algorithm will not complete in any reasonable amount of time.

Thanks a lot for adding me to review.
But could you please provide some information, how it is possible to test FIOSEEKDATA/FIOSEEKHOLE from user-space side.
I mean, did you write some user-space code to test it or some unit-tests exist somewhere?

In D19327#413791, @pfg wrote:

Note why we specifically set ip->i_flags = 0 since r294652:

In D19325#414951, @pfg wrote:

We should probably keep this panic for consistency with UFS, which also panics on this case.

In D18812#401528, @markj wrote:

Thanks.

The duplication of these stubs is rather unfortunate...

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.