BTRFS: Adding write support

[hyche]

Branch: ​https://github.com/hyche/haiku/tree/btrfs.

Commits to merge: From 760dcbe to 8067e1a

[tqh]

Hi Hyche, very nice work! However the correct way is by adding a patch to the ticket: https://dev.haiku-os.org/wiki/CodingGuidelines/SubmittingPatches#Preparingtocreatepatchfile

[hyche]

Replying to tqh:

Hi Hyche, very nice work! However the correct way is by adding a patch to the ticket: https://dev.haiku-os.org/wiki/CodingGuidelines/SubmittingPatches#Preparingtocreatepatchfile

Hi tqh, pulkomandy said that submit this way is ok. As you can see in his email ​https://www.freelists.org/post/haiku-gsoc/General-comments-after-first-month-of-coding-period

[tqh]

Then I guess it is :) We usually try to have students use the ordinary way of submitting changes.

[korli]

I would obviously leave the filesystem readonly though, and leave this commit ​https://github.com/hyche/haiku/commit/4b157e506feadcc5adef43e17bb4ba2b378a84c3 out.

[hyche]

Replying to korli:

I would obviously leave the filesystem readonly though, and leave this commit ​https://github.com/hyche/haiku/commit/4b157e506feadcc5adef43e17bb4ba2b378a84c3 out.

Thanks, fixed by rebasing and the end commit now is fb1352e, please look again.

[pulkomandy]

@tqh: it seems acceptable to review and cherry-pick changes from github, no matter if it comes from GSoC students or otherwise. I don't see why we should have people use format-patch, and then download the patch and use git am to apply it, when it is so much easier for everyone to just add a git remote and cherry-pick the commits from there.

What *is* important however, is to use tickets as a "request to merge", as is done here.

I'll try to review these changes over the week-end if no one else beats me to it.

[axeld]

FWIW I prefer format-patch for the following reasons:

• Adding a remote is easy, yes, but unless you want to fill your list of remotes endlessly, you have to clean up again.
• It's more work and error prone to cherry-pick a couple of commits (which ones exactly?) then to just apply a patch.
• A rebase of the repository will render all communicated commits invalid; a format-patch is here to stay, even if you take a year to apply it.
• Providing a format-patch gives a better overview over the changes for everyone looking at the ticket instantly, including the author of the patches.
• It greatly simplifies review, and comments in the ticket directly.

For these reasons, I prefer we'd stay with our usual way of requiring to use format-patch, please :-)

[pulkomandy]

Hyche's work as a patchset, for archiving purposes

[pulkomandy]

Btw, adding and removing a remote is easy, especially if you use ​https://github.com/github/hub (available in HaikuDepot):

hub remote add hyche/haiku
hub fetch hyche btrfs
...
hub remote del hyche

[pulkomandy]

In the first change (style fixes), in btrfs_read_attr_dir:

+       if (status != B_OK)
+               return status;
+
        if (status == B_ENTRY_NOT_FOUND) {
                *_num = 0;
                return B_OK;
-       } else if (status != B_OK)
-               return status;
+       }

The previous code would ignore the B_ENTRY_NOT_FOUND error and return B_OK instead. Now it will fail. Is that intentional?

[pulkomandy]

In the second commit, in src/tools/btrfs_shell/command_cat.cpp:

If the argc is < 2, the command exits without any message. Wouldn't it be better to print the help in that case?

[pulkomandy]

In 8ee5e1382f7cc0bf66bb317e84a7295e8c07b84d:

I would name the method LogicalAddress(), rather than just Logical().

[pulkomandy]

In 53b5221a2031c95017047b21a46ce9e155fbae9b:

I remember I was the one suggesting BTreeNode and BTreePath, but reading this code again I think using BTree::Node and BTree::Path may make more sense (as inner-classes or in a namespace?)

[pulkomandy]

5314db1c524a05839ab3c3ff30ee0ed686f42b15:

Typo in commit message ('fixex')

[pulkomandy]

4e041a19888709680dd144f930503ee70e2d5aee:

Now we have a Logical() here that is not the same thing as the Logical() in 8ee5e1382f7cc0bf66bb317e84a7295e8c07b84d. What was wrong with LogicalRoot()?

[pulkomandy]

40d013209a754df08702b02a651cb1d87191e395:

I assume the goal of the "read" flag is to create the entry if it does not exist, for write support. If that is the case, then createIfNotFound would be a more descriptive name than "read".

[pulkomandy]

05c45c421da0b51007b37e8450dbd8172f1edc97:

Add more line breaks in commit message.

Removing the whole test of if (to < from || from < 0 || to >= ItemCount() || ItemCount() == 0) sounds a bit dangerous, why not remove just the test for ItemCount() == 0 from it?

[hyche]

Replying to pulkomandy:

In the first change (style fixes), in btrfs_read_attr_dir:

+       if (status != B_OK)
+               return status;
+
        if (status == B_ENTRY_NOT_FOUND) {
                *_num = 0;
                return B_OK;
-       } else if (status != B_OK)
-               return status;
+       }

The previous code would ignore the B_ENTRY_NOT_FOUND error and return B_OK instead. Now it will fail. Is that intentional?

No, that was a mistake, I don't know why me at that time doing this, thanks for pointing out.

I remember I was the one suggesting BTreeNode and BTreePath, but reading this code again I > think using BTree::Node and BTree::Path may make more sense (as inner-classes or in a > namespace?)

I moved it into BTree class, but not sure about code style, please check again.

Now we have a Logical() here that is not the same thing as the Logical() in 8ee5e1382f7cc0bf66bb317e84a7295e8c07b84d. What was wrong with LogicalRoot()?

A tree has only one root, so logical address of the tree would be understood as LogicalRoot, that is my original thought, but anyway fix it.

I assume the goal of the "read" flag is to create the entry if it does not exist, for write >support. If that is the case, then createIfNotFound would be a more descriptive name than >"read".

Yes, but it doesn't create inside the find function, it only retrieve the slot for later. Beside, I just let it there to separate write and read operations, I won't touch it until next week.

Add more line breaks in commit message.

Done.

Removing the whole test of if (to < from || from < 0 || to >= ItemCount() || ItemCount() == > 0) sounds a bit dangerous, why not remove just the test for ItemCount() == 0 from it?

I felt it kinda reduntdant..., anyway fix it.

Also, original branch for someone who lost: ​https://github.com/hyche/haiku/commits/btrfs_review

[pulkomandy]

In the second commit ("add cat command"):

if (argc <= 2 && !strcmp(argv[1], "--help"))

This doesn't work. If argc is 1, argv[1] is not set.

Try this:

    if (argc < 2 || strcmp(argv[1], "--help") == 0)

In this case:

• If there is only one argument, the second part of the condition isn't tested and we enter the if clause,
• If there are two or more arguments, we test the first one to be equal to "--help".

Side note: I don't know if the coding style mandates it, but I use strcmp (and any *cmp function) with == operator. It reads like "string compares equal" and can also work with <, >, etc. The use of the ! operator is more confusing to me.

There is the downside that you can't cat a file named --help unless we also handle the -- option, but that sounds like an acceptable limitation for the btrfs_shell.

[pulkomandy]

In general, the format of your commit messages does not follow the guidelines.

The format should be:

• 1 short line describing the changes
• 1 blank line
• More lines with details about the changes

Using this format makes the output of several tools, including cgit and git log --format=oneline, much more useful and easy to follow.

I think the first line is supposed to be limited to 60 chars, but usually I find this too short so I tend to ignore this restriction. Try to keep it to a reasonable length, still. The blank line is important, otherwise git puts everything in the first line.

I am rewording your commit messages to match this, but think about it for future commits.

[hyche]

Replying to pulkomandy:

[...]

Fix as you said. Commits to merge: 7698541 -> 0c2d2d7, bbaeac0, 70c027a.

Also, the rest commits are not done, I will try to finish it this week.

[pulkomandy]

There is no hurry in getting everything merged, so take your time for the "not done" commits. Just remember to tell us when you have something ready.

[pulkomandy]

Ok, I merged the parts that looked ready. Let us know when there is more.

[hyche]

Commits to merge: all Sorry for sending patchs in batch, because things are dependent on each other. Please review, thanks.

[pulkomandy]

Patch 3 doesn't look right. If you have multiple BTRFS volumes with different block or sector sizes, the last one to set the static variables will erase the value for others.

This is useless, delete is already doing the check:

if (fIterator != NULL)
    delete fIterator

[hyche]

Replying to pulkomandy:

Patch 3 doesn't look right. If you have multiple BTRFS volumes with different block or sector sizes, the last one to set the static variables will erase the value for others.

I don't think it is valid to have volumes with different block/sector sizes to use them simutaneously. Because we have to format them at once. For instance

mkfs.btrfs device1 device2 device3 <-- have same block/sector size

Although each volume have its superblock to indicate its block/sector size, but they are all the same.

This is useless, delete is already doing the check:

if (fIterator != NULL)
    delete fIterator

Thanks fix it! and new thing to learn too :)

[pulkomandy]

This code is used each time you mount a volume. You do not know where the volume comes from, maybe it was freshly formatted by mkfs, or maybe it is from an USB stick coming from another computer.

According to ​https://btrfs.wiki.kernel.org/index.php/Manpage/mkfs.btrfs, the node size even changed accross btrfs versions, and both the node and sector size depend on the page size of the machine the FS was created on.

So, if one carries a filesystem from one system to another (USB stick, or just booting different OS on the same machine), or keeps an old filesystem next to a newer one, you can easily end up with filesystems of different block sizes. We need to handle that correctly.

[hyche]

I got it, and I don't know if there is better way to retrieve them, so just let it how the way it was.

[hyche]

lot of changes since the old one

[hyche]

Last patchs for GSoC, please review

Thanks.

[tqh]

Nice. I hope korli or pulkomandy can help review?

[pulkomandy]

These are quite large patchsets.

I did not spot anything obviously wrong in the first one, but I'm not very knowledgeable about btrfs (everything I learnt about the internal data structures is from your blogposts, basically). I'll review the second one later as it's getting late here.

[hyche]

I changed and scrubbed a little on patch 7 of first patchset (​FindNext function of CachedExtentTree), ​SearchSlot() and ​here

Also there is a ​line (12th of second patchset) here is I don't know if it is right, because _data can be NULL but it did compile and run fine.

[waddlesplash]

Patchsets 3 and 4 applied in hrev51666 (with the exception of the commit to enable write mode; I don't think we want to do that in the main tree just yet.)

[pulkomandy]

In patch 5/6, don't we need to malloc space also for the extra attribute data, not just the name?

For the logs, maybe you could use the func identifier to log the function names without errors.

[pulkomandy]

Patchset 5 moved to Gerrit:

• ​https://review.haiku-os.org/33
• ​https://review.haiku-os.org/34
• ​https://review.haiku-os.org/35
• ​https://review.haiku-os.org/36
• ​https://review.haiku-os.org/37
• ​https://review.haiku-os.org/38