Personal/nitinsingla/fscm changes by nitin-deamon · Pull Request #36 · linuxsmiths/AZNFS-mount

nitin-deamon · 2025-02-06T07:29:58Z

No description provided.

turbonfs/src/rpc_task.cpp

linuxsmiths · 2025-02-07T04:07:44Z

turbonfs/src/fcsm.cpp

+               task ? "blocking" : "non-blocking",
+               commit_bytes,
+               fmt::ptr(task));
+    assert(inode->is_flushing == true);


don't compare with true/false for booleans

turbonfs/src/fcsm.cpp

linuxsmiths · 2025-02-08T22:46:53Z

turbonfs/inc/file_cache.h

+     * that we need to write inline, else if not under memory pressure returns
+     * zero.
+     */
+    uint64_t get_inline_flush_bytes() const


we have do_inline_write() and get_inline_flush_bytes() bith which return "should we do inline writes" separately and may return different results, which is nto good.
we should have just one function.

I see that we don't use it. I'll remove it.

linuxsmiths · 2025-02-08T22:55:43Z

turbonfs/src/file_cache.cpp

+
    // TODO: Make it shared lock.
    const std::unique_lock<std::mutex> _lock(chunkmap_lock_43);
    auto it = chunkmap.lower_bound(0);


chunkmap.cbegin() is better

linuxsmiths · 2025-02-08T23:34:19Z

turbonfs/src/file_cache.cpp

@@ -2669,6 +2673,53 @@ std::vector<bytes_chunk> bytes_chunk_cache::get_commit_pending_bcs() const
            assert(!mb->is_dirty());


how do we prevent some write dirtying a commit pending membuf?

linuxsmiths · 2025-02-09T03:56:01Z

turbonfs/src/fcsm.cpp

+    inode->flush_lock();
+    inode->get_fcsm()->ensure_flush(offset, length, nullptr);
+    inode->flush_unlock();


why do we not have this inside "if (need_flush)"

linuxsmiths · 2025-02-09T04:19:46Z

turbonfs/src/fcsm.cpp

+        int64_t bytes = inode->get_filecache()->get_bytes_to_flush() -
+                          inode->get_filecache()->max_dirty_extent_bytes();


we need to make sure this doesn't result in small blocks being written

linuxsmiths · 2025-02-09T04:25:42Z

turbonfs/src/fcsm.cpp

+    if (commit_bytes == 0) {
+        AZLogDebug("COMMIT BYTES ZERO");
+    }
+


if no new bytes to commit, why keep the task waiting.. and why add the commit target

linuxsmiths · 2025-02-09T05:08:32Z

turbonfs/src/fcsm.cpp


    // Flush callback can only be called if FCSM is running.
-    assert(is_running());
+    // assert(is_running());


why did you comment this?

linuxsmiths · 2025-02-09T05:17:37Z

turbonfs/src/fcsm.cpp

     */
-    if (inode->get_filecache()->is_flushing_in_progress()) {
+    if (inode->get_filecache()->is_flushing_in_progress() ||
+        !is_running() ||


and you have this check? I'm confused.
How can the state machine not be runnign when we are inside the callback, which state machine calls.

linuxsmiths · 2025-02-09T05:20:19Z

turbonfs/src/fcsm.cpp

-    if (inode->get_filecache()->is_flushing_in_progress()) {
+    if (inode->get_filecache()->is_flushing_in_progress() ||
+        !is_running() ||
+        inode->is_commit_in_progress()) {


also, how can commit be running while we are still inside the flush callback.
The whole point of fcsm is, that it'll serialize the flush and commit performed on a file.

linuxsmiths · 2025-02-09T05:30:40Z

turbonfs/src/fcsm.cpp

+     * If we flushed, we should trigger commit so that memory is released.
     */
-    if (!ftgtq.empty() && (ftgtq.front().flush_seq > flushing_seq_num)) {
+    if (!ctgtq.empty() && (ctgtq.front().commit_seq < flushed_seq_num)) {


shouldn't it be <=?
if commit target is asking to commit till seq 100 and we have flushed till seq 100, why should we not commit

linuxsmiths · 2025-02-09T09:41:09Z

turbonfs/src/fcsm.cpp

+        // commit_membufs() will update committing_seq_num() and mark fcsm running.
+        inode->commit_membufs(bc_vec);
+        assert(committing_seq_num >= bytes);
+    } else if ((!ftgtq.empty() && (ftgtq.front().flush_seq > flushed_seq_num)) ||


shouldn't this check be
(ftgtq.front().flush_seq > flushing_seq_num)

since we want to flush only if it's not already flushingl

similarly the next check should be against committing_seq_num?
In on_flush_complete() why should we check against committed_seq_num, as committed_seq_num can only change in on_commit_complete()

infact the check should be against flushing_seq_num, since we want to check if there is a commit target asking more bytes to be committed than the flushing_seq_num, which would mean that we want to flush more.

linuxsmiths · 2025-02-09T09:52:42Z

turbonfs/src/fcsm.cpp

+
+        if (inode->is_stable_write()) {
+            // We should have all the dirty data in the chunkmap.
+            assert(bytes >= (ftgtq.front().flush_seq - flushed_seq_num));


again, this should be flushing_seq_num

linuxsmiths · 2025-02-09T09:59:27Z

turbonfs/src/fcsm.cpp

+         * If commit is in progress, then we should not clear
+         * the running flag. Most likely it's issued from flush_cache_and_wait().
         */
-        clear_running();
+        if (!inode->is_commit_in_progress()) {


why do we need to special case flush_cache_and_wait().
can it also call ensure_flush(), maybe with a special value indicating "flush all".
That will simplify things further, since only way flush and commit could be running is from the state machine, which knows how to serialize them.

linuxsmiths · 2025-02-09T10:18:18Z

turbonfs/src/fcsm.cpp

+    assert(!inode->is_commit_in_progress());
+    assert(committed_seq_num == committing_seq_num);
+    assert(flushed_seq_num == committed_seq_num);
+    assert(!inode->is_stable_write() || ctgtq.empty());


why should we be inside on_commit_complete() for unstable writes

linuxsmiths · 2025-02-09T10:35:32Z

turbonfs/src/fcsm.cpp

+        /*
+         * It may happen flush initiated from flush_cache_and_wait(), so
+         * we should not clear the running flag.
+         */
+        if(!inode->get_filecache()->is_flushing_in_progress()) {


again, better to update flush_cache_and_wait() to use the fscm

linuxsmiths · 2025-02-09T10:54:20Z

turbonfs/src/fcsm.cpp

 #endif
+        if (task == nullptr &&
+             (target_flushed_seq_num == last_flush_seq)) {
+            assert(is_running());


we are here inside "if (is_running())", why additional check, here and at L529

nitin-deamon force-pushed the personal/nitinsingla/fscm-changes branch 3 times, most recently from 437d54a to 746af47 Compare February 6, 2025 18:04

linuxsmiths reviewed Feb 7, 2025

View reviewed changes

turbonfs/src/rpc_task.cpp Outdated Show resolved Hide resolved

linuxsmiths reviewed Feb 7, 2025

View reviewed changes

turbonfs/src/fcsm.cpp Outdated Show resolved Hide resolved

nitin-deamon added 6 commits February 8, 2025 11:57

SOme changes.

dad0532

Working changes.

f932006

Final Changes.

bffccb0

Added cleanup.

c52258a

Added JukeBox Error changes.

d6a5fde

Code refactoring.

c116f4e

nitin-deamon force-pushed the personal/nitinsingla/fscm-changes branch from 815fc5e to c116f4e Compare February 8, 2025 12:01

nitin-deamon added 2 commits February 8, 2025 12:14

Fix the build error.

d78acb7

Fix the random write issue.

9e89216

linuxsmiths reviewed Feb 8, 2025

View reviewed changes

linuxsmiths reviewed Feb 9, 2025

View reviewed changes

linuxsmiths force-pushed the main branch 5 times, most recently from ed07eba to f024ccb Compare February 19, 2025 12:55

		@@ -2669,6 +2673,53 @@ std::vector<bytes_chunk> bytes_chunk_cache::get_commit_pending_bcs() const
		assert(!mb->is_dirty());

		int64_t bytes = inode->get_filecache()->get_bytes_to_flush() -
		inode->get_filecache()->max_dirty_extent_bytes();

Conversation

nitin-deamon commented Feb 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linuxsmiths Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

linuxsmiths Feb 8, 2025 •

edited

Loading