Opened 2 years ago

Last modified 21 months ago

#13612 new enhancement

BTRFS: Adding write support

Reported by: hyche Owned by: korli
Priority: normal Milestone: Unscheduled
Component: File Systems/Btrfs Version: R1/Development
Keywords: gsoc2017 Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Commits to merge: From 760dcbe to 8067e1a

Attachments (6)

btrfs.patchset (164.6 KB ) - added by pulkomandy 2 years ago.
Hyche's work as a patchset, for archiving purposes
btrfs.patchset2 (50.6 KB ) - added by hyche 2 years ago.
btrfs.patchset3 (70.3 KB ) - added by hyche 2 years ago.
btrfs.patchset3_2 (75.1 KB ) - added by hyche 2 years ago.
lot of changes since the old one
btrfs.patchset4 (81.6 KB ) - added by hyche 2 years ago.
btrfs.patchset5 (8.9 KB ) - added by hyche 21 months ago.

Download all attachments as: .zip

Change History (42)

comment:1 by tqh, 2 years ago

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

Last edited 2 years ago by tqh (previous) (diff)

in reply to:  1 comment:2 by hyche, 2 years ago

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

comment:3 by tqh, 2 years ago

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

comment:4 by korli, 2 years ago

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

in reply to:  4 comment:5 by hyche, 2 years ago

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.

comment:6 by pulkomandy, 2 years ago

@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.

comment:7 by axeld, 2 years ago

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

by pulkomandy, 2 years ago

Attachment: btrfs.patchset added

Hyche's work as a patchset, for archiving purposes

comment:8 by pulkomandy, 2 years ago

Has a Patch: set

comment:9 by pulkomandy, 2 years ago

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

comment:10 by pulkomandy, 2 years ago

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?

comment:11 by pulkomandy, 2 years ago

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?

comment:12 by pulkomandy, 2 years ago

In 8ee5e1382f7cc0bf66bb317e84a7295e8c07b84d:

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

comment:13 by pulkomandy, 2 years ago

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

comment:14 by pulkomandy, 2 years ago

5314db1c524a05839ab3c3ff30ee0ed686f42b15:

Typo in commit message ('fixex')

comment:15 by pulkomandy, 2 years ago

4e041a19888709680dd144f930503ee70e2d5aee:

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

comment:16 by pulkomandy, 2 years ago

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".

comment:17 by pulkomandy, 2 years ago

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?

Last edited 2 years ago by pulkomandy (previous) (diff)

in reply to:  10 comment:18 by hyche, 2 years ago

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

comment:19 by pulkomandy, 2 years ago

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.

comment:20 by pulkomandy, 2 years ago

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.

by hyche, 2 years ago

Attachment: btrfs.patchset2 added

in reply to:  20 comment:21 by hyche, 2 years ago

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.

comment:22 by pulkomandy, 2 years ago

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.

comment:23 by pulkomandy, 2 years ago

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

by hyche, 2 years ago

Attachment: btrfs.patchset3 added

comment:24 by hyche, 2 years ago

Commits to merge: all

Sorry for sending patchs in batch, because things are dependent on each other. Please review, thanks.

Last edited 2 years ago by hyche (previous) (diff)

comment:25 by pulkomandy, 2 years ago

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

in reply to:  25 comment:26 by hyche, 2 years ago

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

comment:27 by pulkomandy, 2 years ago

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.

comment:28 by hyche, 2 years ago

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.

by hyche, 2 years ago

Attachment: btrfs.patchset3_2 added

lot of changes since the old one

comment:29 by hyche, 2 years ago

Last patchs for GSoC, please review

Thanks.

comment:30 by tqh, 2 years ago

Nice. I hope korli or pulkomandy can help review?

comment:31 by pulkomandy, 2 years ago

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.

comment:32 by hyche, 2 years ago

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.

Last edited 2 years ago by hyche (previous) (diff)

by hyche, 2 years ago

Attachment: btrfs.patchset4 added

comment:33 by waddlesplash, 22 months ago

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

by hyche, 21 months ago

Attachment: btrfs.patchset5 added

comment:34 by pulkomandy, 21 months ago

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.

comment:36 by pulkomandy, 21 months ago

Has a Patch: unset
Note: See TracTickets for help on using tickets.