nbd: add auto map at boot #347

SeanHai · 2021-05-13T09:35:26Z

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

ilixiaocui · 2021-05-14T09:11:37Z

nbd/src/main.cpp

+static void AddRecord() {
+    std::fstream fs;
+    // check if exist
+    fs.open(CURVETAB_PATH, std::fstream::in);


Read-only open？

Read-only open？

std::fstream::in is read-only

ilixiaocui · 2021-05-14T09:13:16Z

nbd/src/main.cpp

+            while (getline(ss, str, '\t')) {
+                lineArray.push_back(str);
+            }
+            if (lineArray.size() != 2 && lineArray.size() != 3) {


Under what circumstances lineArray.size() == 2

Under what circumstances lineArray.size() == 2

if you only use curve-nbd map, will only image and device write in curvetab like: cbd:pool//test_test_ /dev/nbd0
then if you mkfs on the device, you need record the mount point in the curvetab manually like fstab.
reason for not using fstab is fstab run before rc or system.d at reboot.

ilixiaocui · 2021-05-14T09:14:10Z

nbd/src/main.cpp

+                fs.close();
+                return;
+            }
+            if (lineArray[0] == nbdConfig->imgname) {


check if the same mount point?

check if the same mount point?

Not needed, only record image and device here. Mount point is ot necessary in curvetab.
The correspondence between image device and mount-point is record manually after you mount like fstab

ilixiaocui · 2021-05-14T09:16:01Z

nbd/src/main.cpp

+    }
+    fs.close();
+    // write new record
+    fs.open(CURVETAB_PATH, std::fstream::app);


Is it a problem to mount multiple volumes at the same time?

Is it a problem to mount multiple volumes at the same time?

add flock

ilixiaocui · 2021-05-14T09:17:07Z

nbd/src/main.cpp

+    }
+
+    std::string lineString;
+    while (getline(fs, lineString)) {


Remarks：// Check if the record exists

Remarks：// Check if the record exists

done

ilixiaocui · 2021-05-14T09:18:35Z

nbd/src/main.cpp

+    std::fstream fs;
+    fs.open(CURVETAB_PATH, std::fstream::in);
+    if (!fs.is_open()) {
+        std::cerr << "curve-nbd: open curvetab file failed.";


return?

done

ilixiaocui · 2021-05-14T09:18:44Z

nbd/src/main.cpp

+    // check if exist
+    fs.open(CURVETAB_PATH, std::fstream::in);
+    if (!fs.is_open()) {
+        std::cerr << "curve-nbd: open curvetab file failed.";


return

done

return with error code?

ilixiaocui · 2021-05-14T09:19:04Z

nbd/src/main.cpp

+
+    std::string lineString;
+    std::string newContent;
+    while (getline(fs, lineString)) {


remarks: // find record

remarks: // find record

done.
// find record need deleted

ilixiaocui · 2021-05-14T09:19:28Z

nbd/src/main.cpp

+            while (getline(ss, str, '\t')) {
+                lineArray.push_back(str);
+            }
+            if (lineArray.size() != 2 && lineArray.size() != 3) {


Same as above

Same as above

done

ilixiaocui · 2021-05-14T09:20:38Z

nbd/src/main.cpp

+    }
+    fs.close();
+    // update record info
+    fs.open(CURVETAB_PATH, std::fstream::out);


judgment ： fs.is_open()

judgment ： fs.is_open()

done

Wine93 · 2021-05-18T06:33:00Z

nbd/nbd-package/usr/map_curve_disk.sh

+    do
+        image=$(echo $line | awk '{print $1}')
+        device=$(echo $line | awk '{print $2}')
+	mount=$(echo $line | awk '{print $3}')


indentation

indentation

done

Wine93 · 2021-05-18T06:50:40Z

nbd/src/main.cpp

+    if (0 == flock(fd, LOCK_SH )) {
+        // check if the record exists
+        std::string lineString;
+        while (getline(fs, lineString)) {


The logic for parse file in AddRecord and DelRecord is highly coincident, you can abstract the comon logic into one function.

Wine93 · 2021-05-18T06:51:48Z

nbd/src/main.cpp

+                }
+                if (lineArray.size() != 2 && lineArray.size() != 3) {
+                    std::cerr << "curve-nbd: curvetab file content error.";
+                    fs.close();


The close() is redunrant, because the file stream's destructor will close the file.

The close() is redunrant, because the file stream's destructor will close the file.

the fs will close when goes out of scope. but this fs reused latter in this function. so I think close is needed

Wine93 · 2021-05-18T07:01:53Z

nbd/src/main.cpp

+                }
+            }
+        }
+        fs.close();


You should invoke flock(fd, LOCK_UN) before fs.close().

You should invoke flock(fd, LOCK_UN) before fs.close().

I think this is ok , because flock() is tied to the file descriptor, it gets released when the file is closed. so flock(fd, LOCK_UN) even not write

Wine93 · 2021-05-18T07:03:14Z

nbd/src/main.cpp

@@ -95,6 +97,112 @@ static void Usage() {
        << std::endl;
 }

+// use for record image and device info to auto map when boot
+static void AddRecord() {
+    std::fstream fs;


You can use std::ifstream to replace std::fstream and open with std::fstream::in.

You can use std::ifstream to replace std::fstream and open with std::fstream::in.

because fs reused latter in this function as std::fstream::app

Wine93 · 2021-05-18T07:12:29Z

nbd/src/main.cpp

+    }
+    fd = static_cast<__gnu_cxx::stdio_filebuf< char > * const >
+                                            (fs.rdbuf())->fd();
+    if (0 == flock(fd, LOCK_SH )) {


If you want use the flock to sync the operation in file, i suggest use the FileLock class in nebd/src/common/file_lock.cpp.

Wine93 · 2021-05-18T07:18:18Z

nbd/src/main.cpp

+        fs.close();
+        flock(fd, LOCK_UN);
+    }
+    // update record info


Note there is time window in the read and write logic.

Note there is time window in the read and write logic.

what is mean?

Wine93 · 2021-05-19T01:48:54Z

nbd/src/define.h

@@ -61,6 +61,7 @@ namespace nbd {
 #define PROCESS_NAME "curve-nbd"
 #define NBD_PATH_PREFIX "/sys/block/nbd"
 #define DEV_PATH_PREFIX "/dev/nbd"
+#define CURVETAB_PATH "/etc/curve/curvetab"


Maybe it's better to named CURVETAB_LOG_PATH ?

Wine93 · 2021-05-19T02:00:58Z

nbd/nbd-package/usr/map_curve_disk.sh

+            unset recordmap[$device]
+        fi
+    done < ${confPath}
+    for key in ${!recordmap[@]};do


for key in ${!recordmap[@]};do -> for key in ${!recordmap[@]}; do

Wine93 · 2021-05-19T02:21:01Z

LGTM

SeanHai · 2021-05-19T03:41:35Z

recheck

SeanHai · 2021-05-19T04:23:39Z

recheck

SeanHai · 2021-05-19T05:42:23Z

recheck

SeanHai force-pushed the curve_nbd_automap branch 2 times, most recently from 5459eab to c119d4b Compare May 14, 2021 07:49

ilixiaocui reviewed May 14, 2021

View reviewed changes

SeanHai force-pushed the curve_nbd_automap branch 5 times, most recently from 5bfbddc to d719751 Compare May 18, 2021 06:27

Wine93 reviewed May 18, 2021

View reviewed changes

SeanHai force-pushed the curve_nbd_automap branch 3 times, most recently from 34e4652 to f89fa8d Compare May 18, 2021 13:50

Wine93 reviewed May 19, 2021

View reviewed changes

SeanHai force-pushed the curve_nbd_automap branch from f89fa8d to 593b92b Compare May 19, 2021 02:06

Wine93 approved these changes May 19, 2021

View reviewed changes

nbd: add auto map at boot

4fd181c

SeanHai force-pushed the curve_nbd_automap branch from 593b92b to 4fd181c Compare May 19, 2021 03:16

ilixiaocui approved these changes May 19, 2021

View reviewed changes

YunhuiChen merged commit 0a11418 into opencurve:master May 19, 2021

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

Merge pull request opencurve#347 from yongtang/coredns

cf71b55

nbd: add auto map at boot #347

nbd: add auto map at boot #347

Conversation

SeanHai commented May 13, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

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

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

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

Choose a reason for hiding this comment

SeanHai May 18, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wine93 commented May 19, 2021

SeanHai commented May 19, 2021

SeanHai commented May 19, 2021

SeanHai commented May 19, 2021

SeanHai May 18, 2021 •

edited