replace flock with fcntl based locks

Signed-off-by: Utkarsh Srivastava <[email protected]>

add comment to point to the GH discussion

Signed-off-by: Utkarsh Srivastava <[email protected]>

Author: tangledbytes

src/manage_nsfs/manage_nsfs_glacier.js

@@ -223,7 +223,7 @@ async function lock_and_run(fs_context, lockfilename, cb) {
     const lockfd = await nb_native().fs.open(fs_context, path.join(config.NSFS_GLACIER_LOGS_DIR, lockfilename), 'w');
 
     try {
-        await lockfd.flock(fs_context, 'EXCLUSIVE');
+        await lockfd.fcntllock(fs_context, 'EXCLUSIVE');
         await cb();
     } finally {
         await lockfd.close(fs_context);

src/native/fs/fs_napi.cpp

@@ -14,6 +14,7 @@
 #include <map>
 #include <math.h>
 #include <stdlib.h>
+#include <sys/fcntl.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
@@ -1363,6 +1364,7 @@ struct FileWrap : public Napi::ObjectWrap<FileWrap>
                 InstanceMethod<&FileWrap::stat>("stat"),
                 InstanceMethod<&FileWrap::fsync>("fsync"),
                 InstanceMethod<&FileWrap::flock>("flock"),
+                InstanceMethod<&FileWrap::fcntllock>("fcntllock"),
                 InstanceAccessor<&FileWrap::getfd>("fd"),
             }));
         constructor.SuppressDestruct();
@@ -1392,6 +1394,7 @@ struct FileWrap : public Napi::ObjectWrap<FileWrap>
     Napi::Value fsync(const Napi::CallbackInfo& info);
     Napi::Value getfd(const Napi::CallbackInfo& info);
     Napi::Value flock(const Napi::CallbackInfo& info);
+    Napi::Value fcntllock(const Napi::CallbackInfo& info);
 };
 
 Napi::FunctionReference FileWrap::constructor;
@@ -1732,8 +1735,10 @@ struct FileFlock : public FSWrapWorker<FileWrap>
                 lock_mode = LOCK_EX;
             } else if (mode == "UNLOCK") {
                 lock_mode = LOCK_UN;
-            } else {
+            } else if (mode == "SHARED") {
                 lock_mode = LOCK_SH;
+            } else {
+                SetError("invalid lock type");
             }
         }
 
@@ -1747,6 +1752,45 @@ struct FileFlock : public FSWrapWorker<FileWrap>
     }
 };
 
+struct FileFcntlLock : public FSWrapWorker<FileWrap>
+{
+    struct flock fl;
+    FileFcntlLock(const Napi::CallbackInfo& info)
+        : FSWrapWorker<FileWrap>(info)
+        , fl()
+    {
+        // lock entire file
+        fl.l_whence = SEEK_SET;
+        fl.l_start = 0;
+        fl.l_len = 0;
+        fl.l_pid = 0;
+        fl.l_type = F_RDLCK;
+
+        if (info.Length() > 1 && !info[1].IsUndefined()) {
+            auto mode = info[1].As<Napi::String>().Utf8Value();
+            if (mode == "EXCLUSIVE") {
+                fl.l_type = F_WRLCK;
+            } else if (mode == "UNLOCK") {
+                fl.l_type = F_UNLCK;
+            } else if (mode == "SHARED") {
+                fl.l_type = F_RDLCK;
+            } else {
+                SetError("invalid lock type");
+            }
+        }
+
+        Begin(XSTR() << "FileFcntlLock" << DVAL(_wrap->_path));
+    }
+    virtual void Work()
+    {
+        int fd = _wrap->_fd;
+        CHECK_WRAP_FD(fd);
+        // This uses F_OFD_SETLKW instead for discussion related to this choice
+        // refer: https://github.com/noobaa/noobaa-core/pull/8174
+        SYSCALL_OR_RETURN(fcntl(fd, F_OFD_SETLKW, &fl));
+    }
+};
+
 struct RealPath : public FSWorker
 {
     std::string _path;
@@ -1899,6 +1943,12 @@ FileWrap::flock(const Napi::CallbackInfo& info)
     return api<FileFlock>(info);
 }
 
+Napi::Value
+FileWrap::fcntllock(const Napi::CallbackInfo& info)
+{
+    return api<FileFcntlLock>(info);
+}
+
 /**
  *
  */

src/sdk/nb.d.ts

@@ -1002,8 +1002,9 @@ interface NativeFile {
     replacexattr(fs_context: NativeFSContext, xattr: NativeFSXattr, clear_prefix?: string): Promise<void>;
     linkfileat(fs_context: NativeFSContext, path: string, fd?: number): Promise<void>;
     fsync(fs_context: NativeFSContext): Promise<void>;
-    fd: number
+    fd: number;
     flock(fs_context: NativeFSContext, operation: "EXCLUSIVE" | "SHARED" | "UNLOCK"): Promise<void>;
+    fcntllock(fs_context: NativeFSContext, operation: "EXCLUSIVE" | "SHARED" | "UNLOCK"): Promise<void>;
 }
 
 interface NativeDir {
@@ -1124,4 +1125,4 @@ interface RestoreStatus {
   state: nb.RestoreState;
   ongoing?: boolean;
   expiry_time?: Date;
-}
+}

src/util/file_reader.js

@@ -115,8 +115,13 @@ class NewlineReader {
     async init() {
         let fh = null;
         try {
-            fh = await nb_native().fs.open(this.fs_context, this.path, 'r');
-            if (this.lock) await fh.flock(this.fs_context, this.lock);
+            // here we are opening the file with both read and write to make sure
+            // fcntlock can acquire both `EXCLUSIVE` as well as `SHARED` lock based
+            // on the need.
+            // If incompatible file descriptor and lock types are used then fcntl
+            // throws `EBADF`.
+            fh = await nb_native().fs.open(this.fs_context, this.path, '+');
+            if (this.lock) await fh.fcntllock(this.fs_context, this.lock);
 
             this.fh = fh;
         } catch (error) {

src/util/persistent_logger.js

@@ -59,7 +59,7 @@ class PersistentLogger {
                 let fh = null;
                 try {
                     fh = await this._open();
-                    if (this.locking) await fh.flock(this.fs_context, this.locking);
+                    if (this.locking) await fh.fcntllock(this.fs_context, this.locking);
 
                     const fh_stat = await fh.stat(this.fs_context);
                     const path_stat = await nb_native().fs.stat(this.fs_context, this.active_path);
@@ -222,7 +222,13 @@ class PersistentLogger {
 
     async _open() {
         await native_fs_utils._create_path(this.dir, this.fs_context);
-        return nb_native().fs.open(this.fs_context, this.active_path, 'as');
+
+        // here we are opening the file with both read and write to make sure
+        // fcntlock can acquire both `EXCLUSIVE` as well as `SHARED` lock based
+        // on the need.
+        // If incompatible file descriptor and lock types are used then fcntl
+        // throws `EBADF`.
+        return nb_native().fs.open(this.fs_context, this.active_path, 'as+');
     }
 
     _poll_active_file_change(poll_interval) {