Looks good from ext2fs side.

@kib, you mentioned in SUMMARY:
"ext2fs change to reset clusterw on truncation should be a bug fix."

Thank you for fix.

In D23259#543519, @pfg wrote:

In D23259#543516, @fsu wrote:

In D23259#543509, @pfg wrote:

Sorry I meant to approve this:
https://reviews.freebsd.org/D24685

(fsu should have comandeered this one instead of opening a new one)

I did not managed with new diff uploading, diff did not want to attach to this revision.
Seems like, I can attach diffs only to revisions, which were opened by me.
Please correct me if I am wrong or lose something in the Phabricator UI.

You had to comandeer it in the Action box. You can't do it anymore because it is closed but I know because other people have comandeered my WIP changes ;).

No problem about the new revision though ... Good Job !

In D23259#543509, @pfg wrote:

Sorry I meant to approve this:
https://reviews.freebsd.org/D24685

(fsu should have comandeered this one instead of opening a new one)

In D13209#537208, @pfg wrote:

In D13209#537057, @trasz wrote:

Ping? What's the status of this?

FWIW, I recall kib had objections and fsu needed clarifications but he never got them.
There was never urgency as there is no known program using this code so the issue has been dormant for a long time.

As I remember, the task appeared to be more difficult than it was expected by me. More changes appeared to be required to implement, than it was expected from start.

I am not sure that dinode and superblock conversion will be enough, as I checked on linux side, the group descriptors should be converted too, possible same for bitmaps.
From https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout
"All fields in ext4 are written to disk in little-endian order"

I am not sure about "filesystem has full read and write support for ext4" because there some ext4 features, that uses rarely and we does not support it.
But may be do not dig into the details in this place.

In D13487#431879, @asomers wrote:

Indeed, this seems to have been abandoned and recreated as D13528, then committed as r338152. @fsu is there any reason not to close this review?

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.