Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storagefsm: Rewrite input handling #5375

Merged
merged 21 commits into from
Feb 25, 2021
Merged

storagefsm: Rewrite input handling #5375

merged 21 commits into from
Feb 25, 2021

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jan 18, 2021 •
edited
Loading

Closes #5356

The main goal here is to enable ingesting deal data in parallel - the old storagefsm interface had a global lock which meant that only one deal could be added to a sector at a time. It also wasn't very robust and could create corrupted sectors if the miner process was restarted at a wrong time (this PR doesn't fully solve this issue, but it makes it much better. Full fix will come in a followup PR and will require changing some worker APIs)

With this PR up to configured Sealing.MaxWaitDealsSectors deals can be processed in parallel (before it was only possible to process one deal in parallel per lotus-miner)

Status

  • CI Tests
  • Test on mainnet miner
  • Review

(Probably should land soon after v1.5.0 as it's a rather big refactor, so putting it in a mandatory release may not be a good idea)

@magik6k magik6k force-pushed the feat/refactor-fsm-input branch 2 times, most recently from bb0f31b to ad3aae1 Compare January 20, 2021 21:18
@magik6k magik6k force-pushed the feat/refactor-fsm-input branch 2 times, most recently from 3ce30b6 to 43184b4 Compare January 21, 2021 13:13
@magik6k magik6k changed the base branch from master to fix/market-precommit-diff January 21, 2021 13:14
Base automatically changed from fix/market-precommit-diff to master January 21, 2021 15:41
@magik6k magik6k marked this pull request as ready for review January 21, 2021 19:49
magik6k
magik6k commented Feb 4, 2021
View reviewed changes
extern/storage-sealing/input.go Outdated
}

m.inputLk.Lock()
if _, exist := m.pendingPieces[*deal.PublishCid]; exist {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this by publishcid is very broken with multiple deals per message, needs fixing

@magik6k
Copy link
Contributor Author

magik6k commented Feb 4, 2021

Another bug this has:

2021-02-04T15:41:00.255+0100    ERROR   evtsm   go-statemachine@v0.0.0-20200925024713-05bd7c71fbfe/machine.go:83        Executing event planner failed: running planner for state Proving failed:
    github.com/filecoin-project/lotus/extern/storage-sealing.(*Sealing).plan
        /home/magik6k/github.com/filecoin-project/go-lotus/extern/storage-sealing/fsm.go:242
  - planner for state Proving received unexpected event sealing.SectorAddPiece ({User:{}}):
    github.com/filecoin-project/lotus/extern/storage-sealing.planOne.func1
        /home/magik6k/github.com/filecoin-project/go-lotus/extern/storage-sealing/fsm.go:516

@magik6k
Copy link
Contributor Author

magik6k commented Feb 11, 2021

(did a bunch of manual testing on my miner, it seems to work quite well now)

@Kubuxu
Copy link
Contributor

Kubuxu commented Feb 11, 2021

I think we need to figure out a way to make the FSM more expressive, it is still a lot of boilerplate.

Kubuxu
Kubuxu reviewed Feb 11, 2021
View reviewed changes
extern/storage-sealing/input.go
return true, ctx.Send(SectorStartPacking{})
}

if used.Padded() == abi.PaddedPieceSize(ssize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably have some leeway, there is no reason to wait for the timeout because there is room for a 10byte deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you,ll never have less than the minimum deal size free, which is already configurable - but yes, we could add a separate config for this

Kubuxu
Kubuxu approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weak 👍 from me, with the code moving from one file to the other (and there not being a good commit to look at the changes to the moved code) review is harder than it could be.

Also, there are few TODOs in the code, it would be awesome if they got their own issue so if we ever hit these todos we can refer to them.

@cryptowhizzard cryptowhizzard mentioned this pull request Feb 13, 2021
Closed
@magik6k magik6k mentioned this pull request Feb 16, 2021
Merged
@magik6k magik6k changed the base branch from master to next February 25, 2021 13:27
@magik6k magik6k merged commit e49a412 into next Feb 25, 2021
@magik6k magik6k deleted the feat/refactor-fsm-input branch February 25, 2021 13:28
@magik6k magik6k mentioned this pull request Mar 8, 2021
Closed
@dependabot dependabot bot mentioned this pull request Mar 12, 2021
Closed
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Mar 22, 2021
@magik6k @bibibong
Merge pull request filecoin-project#5375 from filecoin-project/feat/r…
d863eb2 
…efactor-fsm-input

storagefsm: Rewrite input handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kubuxu Kubuxu Kubuxu approved these changes

@whyrusleeping whyrusleeping Awaiting requested review from whyrusleeping

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support processing multiple pieces at once in Storage FSM
2 participants
@magik6k @Kubuxu