Prevent parallel writes to rotational media #176

zmeyc · 2021-03-08T10:23:48Z

During parallel plotting when copying multiple plots to dest simultaneously, if dest is HDD it'll result in slowdown and FS fragmentation.

This patch tries to detect whether dest is rotational media and if so, uses flock() to prevent multiple simultaneous writes to dest.

Linux only, on other platforms the behavior is unchanged.

Adds a log message notifying if parallel writing is enabled:

Starting plotting progress into temporary dirs: . and .
Final dir: . (parallel writing: disabled)
ID: 23023404333717545b0a6f0c0dde9710e4d3fe2d5cc6cc0a090a0b818bab0f17
Plot size is: 18
Buffer size is: 11MiB
Using 16 buckets
Using 2 threads of stripe size 4000

src/plotter_disk.hpp

src/disk_util.hpp

arvidn · 2021-03-09T07:45:03Z

since there's no way to gracefully abort plotting (yet), it's likely to be killed with SIGKILL. Is there any risk the lock on the directory will be orphaned and just keep it locked in that case? or is the lock associated with the process and released automatically if the process is killed?

zmeyc · 2021-03-09T10:19:43Z

@arvidn Yes, it's released automatically if process is killed.

zmeyc · 2021-03-09T10:42:25Z

@arvidn Added logging of time taken to acquire the lock / repushed.

arvidn

this looks reasonable to me. I just have some minor nitpicks. Let's see what @wjblanke thinks about it

src/disk_util.hpp

src/plotter_disk.hpp

zmeyc · 2021-03-09T11:58:44Z

@arvidn Added all the changes and cleaned up the commits.

src/disk_util.hpp

zmeyc · 2021-03-21T10:20:05Z

Fixed merge conflicts.

zmeyc · 2021-03-21T21:56:55Z

Please do not merge, found a problem with symlink (i.e. /mnt/dest -> /mnt/real-hdd) being incorrectly being detected as non-rotational media.

When multiple plotter instances are working, use flock to prevent them from copying files to destination disk simultaneously on rotational media. Assume media is non-rotational by default. Linux only, on Windows and MacOS the behavior is unchanged.

zmeyc · 2021-03-24T00:59:31Z

Now works correctly with symlinks.

zmeyc · 2021-03-24T01:14:45Z

@arvidn It was incorrect to zero-out minor device id. I changed the logic to generate a device path using both major and minor id, resolve the resulting symlink then traverse the final path up to the root looking for "queue/rotational". If not found, media is assumed to be non-rotational.

Example:

/mnt/testlink has major id 65, minor id 178
Generate path (symlink): "/sys/dev/block/65:178"
Resolve symlink to:
/sys/devices/pci0000:00/0000:00:03.1/0000:09:00.0/host2/target2:1:4/2:1:4:0/block/sde/sde2
Append "/queue/rotational" and check if file exists:

/sys/devices/pci0000:00/0000:00:03.1/0000:09:00.0/host2/target2:1:4/2:1:4:0/block/sde/sde2/queue/rotational - not found

Try root path until found:

/sys/devices/pci0000:00/0000:00:03.1/0000:09:00.0/host2/target2:1:4/2:1:4:0/block/sde/queue/rotational - found

If file contains '1' - media is rotational

zmeyc · 2021-04-05T12:32:32Z

@arvidn Would it be possible to merge this PR? If any changes needed, please let me know.

arvidn

Ideally there would be unit tests for as much of this as possible. A test that makes sure the directory lock works shouldn't be too difficult, and a test that ensures that the device ID returns something looking like what is expected also shouldn't be too hard.

Is there anything else that could be unit tested on CI?

arvidn · 2021-04-05T13:49:31Z

src/disk_util.hpp

+#if defined(__APPLE__) || defined(_WIN32)
+        return false;
+#else
+        struct stat s{};


I think this function is linux specific, right?
It should be conditioned by __linux__, since right now it's for everything except MacOS and Windows.
I also think it should have a comment explaining what it does and why it works. Ideally with links to documentation stating that the assumptions made by this function are safe to make.

arvidn · 2021-04-05T13:50:42Z

src/disk_util.hpp

+namespace DiskUtil {
+
+#if !defined(__APPLE__) && !defined(_WIN32)
+    inline fs::path DevicePath(dev_t dev_id)


this function also looks linux specific, it should be conditioned by __linux__.
Is the assumptions made by this function a part of the kernel or user space distro? Presumably the former, some links to documentations would be nice for readers.

arvidn · 2021-04-05T13:52:19Z

src/disk_util.hpp

+    }
+
+    inline int LockDirectory(
+        std::string dirname)


is there a good reason for this to be string instead of fs::path?

arvidn · 2021-04-05T13:53:24Z

src/plotter_disk.hpp

@@ -373,8 +381,14 @@ class DiskPlotter {
                }
            } else {
                if (!bCopied) {
+                    bool should_lock = DiskUtil::ShouldLock(final_dirname);


this should probably be const

arvidn · 2021-04-05T13:54:05Z

src/plotter_disk.hpp

@@ -153,9 +156,14 @@ class DiskPlotter {
        }
 #endif /* defined(_WIN32) || defined(__x86_64__) */

+        const char is_parallel_writing_enabled = !DiskUtil::ShouldLock(final_dirname);


the name suggests that this should be a bool, yet, it's a char. Is there a good reason for this?

github-actions · 2021-08-12T11:05:23Z

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch 3 times, most recently from 61db4da to e1208a5 Compare March 8, 2021 11:08

arvidn reviewed Mar 8, 2021

View reviewed changes

src/plotter_disk.hpp Outdated Show resolved Hide resolved

arvidn reviewed Mar 8, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

arvidn reviewed Mar 8, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

arvidn reviewed Mar 8, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch from bdc9db5 to c2f2cb6 Compare March 8, 2021 23:34

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch from c2f2cb6 to c77eb7a Compare March 9, 2021 10:41

arvidn reviewed Mar 9, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

src/disk_util.hpp Outdated Show resolved Hide resolved

src/disk_util.hpp Outdated Show resolved Hide resolved

src/disk_util.hpp Outdated Show resolved Hide resolved

src/plotter_disk.hpp Outdated Show resolved Hide resolved

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch from c77eb7a to f069c53 Compare March 9, 2021 11:56

zmeyc mentioned this pull request Mar 9, 2021

Allow limiting number of plotting processes during Stage 1 #177

Open

arvidn reviewed Mar 9, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

arvidn reviewed Mar 9, 2021

View reviewed changes

src/disk_util.hpp Outdated Show resolved Hide resolved

zmeyc requested a review from arvidn March 10, 2021 00:30

Base automatically changed from master to main March 15, 2021 18:39

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch from e1e5f98 to adffb0d Compare March 21, 2021 10:18

zmeyc marked this pull request as draft March 21, 2021 21:56

zmeyc force-pushed the no-parallel-writes-on-rotational-media branch from adffb0d to 7988adc Compare March 24, 2021 00:55

zmeyc marked this pull request as ready for review March 24, 2021 00:57

arvidn reviewed Apr 5, 2021

View reviewed changes

github-actions bot added the stale-pr label Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent parallel writes to rotational media #176

Prevent parallel writes to rotational media #176

zmeyc commented Mar 8, 2021

arvidn commented Mar 9, 2021

zmeyc commented Mar 9, 2021

zmeyc commented Mar 9, 2021

arvidn left a comment

zmeyc commented Mar 9, 2021

zmeyc commented Mar 21, 2021

zmeyc commented Mar 21, 2021

zmeyc commented Mar 24, 2021

zmeyc commented Mar 24, 2021

zmeyc commented Apr 5, 2021

arvidn left a comment

arvidn Apr 5, 2021

arvidn Apr 5, 2021

arvidn Apr 5, 2021

arvidn Apr 5, 2021

arvidn Apr 5, 2021

github-actions bot commented Aug 12, 2021

Prevent parallel writes to rotational media #176

Are you sure you want to change the base?

Prevent parallel writes to rotational media #176

Conversation

zmeyc commented Mar 8, 2021

arvidn commented Mar 9, 2021

zmeyc commented Mar 9, 2021

zmeyc commented Mar 9, 2021

arvidn left a comment

Choose a reason for hiding this comment

zmeyc commented Mar 9, 2021

zmeyc commented Mar 21, 2021

zmeyc commented Mar 21, 2021

zmeyc commented Mar 24, 2021

zmeyc commented Mar 24, 2021

zmeyc commented Apr 5, 2021

arvidn left a comment

Choose a reason for hiding this comment

arvidn Apr 5, 2021

Choose a reason for hiding this comment

arvidn Apr 5, 2021

Choose a reason for hiding this comment

arvidn Apr 5, 2021

Choose a reason for hiding this comment

arvidn Apr 5, 2021

Choose a reason for hiding this comment

arvidn Apr 5, 2021

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2021