Skip to content
This repository has been archived by the owner on Sep 22, 2020. It is now read-only.

aoe: flush aoe device when finished serving #412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

aoe: flush aoe device when finished serving #412

wants to merge 1 commit into from

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Nov 30, 2016

fixes #389

Copy link

@lpabon lpabon left a comment

Choose a reason for hiding this comment

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

Please add a comment to why this is necessary for others to understand at some future date.

@@ -116,6 +116,8 @@ func aoeAction(cmd *cobra.Command, args []string) error {
if err = as.Serve(ai); err != nil {
return fmt.Errorf("Failed to serve AoE: %v", err)
}
defer flush(fmt.Sprintf("e%d.%d", minor, major))
Copy link

Choose a reason for hiding this comment

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

Why use defer?

@nak3
Copy link
Contributor Author

nak3 commented Dec 10, 2016

Sure, I updated and added the comment.

@xiang90
Copy link
Contributor

xiang90 commented Dec 12, 2016

@nak3 Did you verify this that this patch fixes #389?

@nak3
Copy link
Contributor Author

nak3 commented Dec 12, 2016

Yes, I did verify it.

(test record)

1. Start and stop torusblk with aoe block device

[vagrant@torus1 torus]$ sudo ./bin/torusblk aoe test1 --debug lo 1 1
...
Attached to AoE device (lo, Major: 1, Minor: 1). Server loop begins ...
...
2016-12-13 12:28:08.869225 D | block: Syncing block volume: test1
^C
Received an interrupt, stopping services...
2016-12-13 12:28:11.547470 E | aoe: failed to handle frame: bad file descriptor
2016-12-13 12:28:11.547612 E | aoe: ReadFrom failed: bad file descriptor
2016-12-13 12:28:11.556207 D | aoe: canceling background goroutines
2016-12-13 12:28:11.556368 D | aoe: waiting for background goroutines to stop
2016-12-13 12:28:11.564921 D | aoe: stopping device sync goroutine
2016-12-13 12:28:11.565137 D | block: not syncing

2. Confirm /dev/etherd/e1.1 has been removed

[vagrant@torus1 torus]$ ls /dev/etherd/p
discover err flush interfaces revalidate

3. fdisk works

[vagrant@torus1 torus]$ sudo fdisk -l > /dev/null
[vagrant@torus1 torus]$ echo $?
0

@xiang90
Copy link
Contributor

xiang90 commented Dec 13, 2016

lgtm. defer to @lpabon

@mischief
Copy link
Contributor

mischief commented Jan 4, 2017

would this be better as a defer, or possibly in the aoe server's Close()?

@nak3
Copy link
Contributor Author

nak3 commented Jan 7, 2017

I got a review #412 (comment) before. And I agreed with reviewer that there are no reason to use defer.

For the idea of aoe server's Close(), as.Close() here should be called after NewServer() but before as.Serve(). So, since flush() should be surely called after the device was created, it should not be called with defer as.Close().

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aoe block device remains after torusblk process finished
4 participants