-
Notifications
You must be signed in to change notification settings - Fork 522
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.
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
nbd: prevent mounted device to unmap #348
Conversation
I hava test the tool option in my own machine and it works well, see below:
NOTE: If you force unmap the device, it will lead to some unrecoverable error:
|
nbd/src/util.h
Outdated
@@ -60,6 +60,8 @@ extern int check_block_size(int nbd_index, uint64_t expected_size); | |||
extern int check_device_size(int nbd_index, uint64_t expected_size); | |||
// 如果当前系统还未加载nbd模块,则进行加载;如果已经加载,则不作任何操作 | |||
extern int load_module(NBDConfig *cfg); | |||
// Check the mount status and judge whether it can be unmap | |||
extern int check_mount_status(const NBDConfig *cfg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern
in here is redundant, and maybe check_dev_can_unmap
is more obvious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_dev_can_unmap
done.
nbd/src/util.cpp
Outdated
@@ -278,6 +280,37 @@ int get_mapped_info(int pid, NBDConfig *cfg) { | |||
return 0; | |||
} | |||
|
|||
int check_mount_status(const NBDConfig *cfg) { | |||
std::ifstream ifs; | |||
ifs.open("/proc/mounts", std::ifstream::in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can merge this as one line code std::ifstream ifs("/proc/mounts", std::ifstream::in);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
td::ifstream ifs("/proc/mounts", std::ifstream::in);
done.
nbd/src/util.cpp
Outdated
|
||
std::string device, mountPath; | ||
bool mounted = false; | ||
while (ifs >> device >> mountPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each line of /proc/mounts
has six columns, so the input in while condition
has a problem.
In the first time, device
stores the first column, and mountPath
stores the second column, but the next time, device
will store the third column - filesystem type, and mountPath
will store the fourth column which is the mount options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each line of
/proc/mounts
has six columns, so the input inwhile condition
has a problem.
In the first time,device
stores the first column, andmountPath
stores the second column, but the next time,device
will store the third column - filesystem type, andmountPath
will store the fourth column which is the mount options.
yeap, it's a BUG, i will fix it.
nbd/src/util.cpp
Outdated
|
||
std::string device, mountPath; | ||
bool mounted = false; | ||
while (ifs >> device >> mountPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe can read line and deal after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe can read line and deal after?
yeap, it's a way to deal with it.
@@ -88,6 +88,7 @@ static void Usage() { | |||
<< " --timeout <seconds> Set nbd request timeout\n" | |||
<< " --try-netlink Use the nbd netlink interface\n" | |||
<< "Unmap options:\n" | |||
<< " -f, --force Force unmap even if the device is mounted\n" // NOLINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force unmap will lead to filesystem readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force unmap will lead to filesystem readonly
yeap, it maybe lead to filesystem become readonly.
BTW: I have constructed a test, that client write one 4K IO not success (RPC error) and keep RPC retrying, and then i do the umount
and curve-nbd unmap
operation, the result is:
- we can't umount the device, even if we specify
-f
option - but we can do
curve-nbd unmap
success - after we done the
curve-nbd unmap
operation, the filesystem is still onread-write
mode, but if we read/write on the filesystem, it will becomeread-only
mode.
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List