Opened 7 years ago
Last modified 7 years 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: | ||
Platform: | All |
Description
Branch: https://github.com/hyche/haiku/tree/btrfs.
Commits to merge: From 760dcbe to 8067e1a
Attachments (6)
Change History (42)
comment:2 by , 7 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 , 7 years ago
Then I guess it is :) We usually try to have students use the ordinary way of submitting changes.
follow-up: 5 comment:4 by , 7 years ago
I would obviously leave the filesystem readonly though, and leave this commit https://github.com/hyche/haiku/commit/4b157e506feadcc5adef43e17bb4ba2b378a84c3 out.
comment:5 by , 7 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 , 7 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 , 7 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 , 7 years ago
Attachment: | btrfs.patchset added |
---|
Hyche's work as a patchset, for archiving purposes
comment:8 by , 7 years ago
patch: | 0 → 1 |
---|
comment:9 by , 7 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
follow-up: 18 comment:10 by , 7 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 , 7 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 , 7 years ago
In 8ee5e1382f7cc0bf66bb317e84a7295e8c07b84d:
I would name the method LogicalAddress(), rather than just Logical().
comment:13 by , 7 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 , 7 years ago
5314db1c524a05839ab3c3ff30ee0ed686f42b15:
Typo in commit message ('fixex')
comment:15 by , 7 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 , 7 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 , 7 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?
comment:18 by , 7 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 , 7 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.
follow-up: 21 comment:20 by , 7 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 , 7 years ago
Attachment: | btrfs.patchset2 added |
---|
comment:21 by , 7 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 , 7 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 , 7 years ago
Ok, I merged the parts that looked ready. Let us know when there is more.
by , 7 years ago
Attachment: | btrfs.patchset3 added |
---|
comment:24 by , 7 years ago
Commits to merge: all
Sorry for sending patchs in batch, because things are dependent on each other. Please review, thanks.
follow-up: 26 comment:25 by , 7 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
comment:26 by , 7 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 , 7 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 , 7 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.
comment:31 by , 7 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 , 7 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.
by , 7 years ago
Attachment: | btrfs.patchset4 added |
---|
comment:33 by , 7 years 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 , 7 years ago
Attachment: | btrfs.patchset5 added |
---|
comment:34 by , 7 years 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:35 by , 7 years ago
comment:36 by , 7 years ago
patch: | 1 → 0 |
---|
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