nbd: prevent mounted device to unmap #348

Wine93 · 2021-05-14T07:45:20Z

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

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

Wine93 · 2021-05-14T08:32:41Z

I hava test the tool option in my own machine and it works well, see below:

STEP 1: map image

root@work:~# curve-nbd map cbd:pool//test_test_
root@work:~# curve-nbd list-mapped
id      image                device
1654834 cbd:pool//test_test_ /dev/nbd0

STEP 2: unmap without mount -> success

root@work:~# curve-nbd unmap /dev/nbd0
root@work:~# curve-nbd list-mapped

STEP 3: remap image and mount it

root@work:~# curve-nbd map cbd:pool//test_test_
root@work:~# curve-nbd list-mapped
id      image                device
1655046 cbd:pool//test_test_ /dev/nbd0

root@work:~# mount /dev/nbd0 /home/data

STEP 4: unmap without -f param -> failed

root@work:~# curve-nbd unmap /dev/nbd0
2021-05-14 16:17:45 1657770 nbd/src/util.cpp:309] curve-nbd: the /dev/nbd0 is still mount on /home/data, you can't unmap it or specify -f parameter
root@work:~# curve-nbd list-mapped
id      image                device
1655046 cbd:pool//test_test_ /dev/nbd0

STEP 5: map with -f param -> success

root@work:~# curve-nbd unmap -f /dev/nbd0
2021-05-14 16:18:03 1657900 nbd/src/util.cpp:304] curve-nbd: the /dev/nbd0 is still mount on /home/data, force unmap it
root@work:~# curve-nbd list-mapped

NOTE: If you force unmap the device, it will lead to some unrecoverable error:

If the process still read/write the filesystem, the process will be the TASK_UNINTERRUPTIBLE state because the system can't complete the IO, it will lead to you can't kill the process and umount the directory.
Probability, the device can't mapped with other image.

wu-hanqing · 2021-05-17T02:08:25Z

nbd/src/util.h

@@ -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);


extern in here is redundant, and maybe check_dev_can_unmap is more obvious

check_dev_can_unmap

done.

wu-hanqing · 2021-05-17T02:21:44Z

nbd/src/util.cpp

@@ -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);


You can merge this as one line code std::ifstream ifs("/proc/mounts", std::ifstream::in);

td::ifstream ifs("/proc/mounts", std::ifstream::in);

done.

wu-hanqing · 2021-05-17T02:28:12Z

nbd/src/util.cpp

+
+    std::string device, mountPath;
+    bool mounted = false;
+    while (ifs >> device >> mountPath) {


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.

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.

yeap, it's a BUG, i will fix it.

SeanHai · 2021-05-19T02:35:48Z

nbd/src/util.cpp

+
+    std::string device, mountPath;
+    bool mounted = false;
+    while (ifs >> device >> mountPath) {


maybe can read line and deal after?

maybe can read line and deal after?

yeap, it's a way to deal with it.

ilixiaocui · 2021-05-19T09:27:45Z

nbd/src/main.cpp

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


force unmap will lead to filesystem readonly

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 on read-write mode, but if we read/write on the filesystem, it will become read-only mode.

wu-hanqing reviewed May 17, 2021

View reviewed changes

SeanHai reviewed May 19, 2021

View reviewed changes

nbd: prevent mounted device to unmap

a2fe2ff

wu-hanqing approved these changes May 19, 2021

View reviewed changes

SeanHai approved these changes May 19, 2021

View reviewed changes

ilixiaocui reviewed May 19, 2021

View reviewed changes

ilixiaocui approved these changes May 21, 2021

View reviewed changes

ilixiaocui merged commit c658b51 into opencurve:master May 21, 2021

ilixiaocui pushed a commit to ilixiaocui/curve that referenced this pull request Feb 6, 2023

Merge pull request opencurve#348 from andylokandy/master

43633eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nbd: prevent mounted device to unmap #348

nbd: prevent mounted device to unmap #348

Wine93 commented May 14, 2021

Wine93 commented May 14, 2021 •

edited

Loading

wu-hanqing May 17, 2021

Wine93 May 19, 2021 •

edited

Loading

wu-hanqing May 17, 2021

Wine93 May 19, 2021

wu-hanqing May 17, 2021

Wine93 May 19, 2021

SeanHai May 19, 2021

Wine93 May 19, 2021

ilixiaocui May 19, 2021

Wine93 May 21, 2021 •

edited

Loading

nbd: prevent mounted device to unmap #348

nbd: prevent mounted device to unmap #348

Conversation

Wine93 commented May 14, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Wine93 commented May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Wine93 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Wine93 commented May 14, 2021 •

edited

Loading

Wine93 May 19, 2021 •

edited

Loading

Wine93 May 21, 2021 •

edited

Loading