[f2fs] Fix tracking i_pino during inline directory rename
Previously, inline directory rename causes i_pino inconsistency.
In the case of the following scenario, a bug can occur.
Let's suppose there are three directories, "/a", "/b", and "/a/c".
- pino of "/a/c" is DIR_A_INO.
Then,
1. Rename "/a/c" to "/b/c"
2. Remount
The parent inode number of "/b/c" should be DIR_B_INO, but it is still DIR_A_INO.
It is because Dir::Rename has old_dir_page(the page having ".." dentry of target directory) and ipage(the inode page of target directory) simultaneously.
Once Dir::Rename is called, it updates i_pino of ipage and flushes it.
Then, it updates ".." dentry of old_dir_page and flushes it.
When target directory supports inline directory entry, old_dir_page becomes same to inode page.
So, old_dir_page overwrites i_pino information of inode page and rolls back to the previous parent inode number.
To fix it, i_pino is updated only in vnode, and updated to the page later.
Change-Id: Id82a36e3188954a1e6307920ad096cad386dd9df
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/613550
Commit-Queue: Chris Suter <csuter@google.com>
Reviewed-by: Chris Suter <csuter@google.com>
diff --git a/src/storage/f2fs/dir.cc b/src/storage/f2fs/dir.cc
index bdd540b..cd24d23 100644
--- a/src/storage/f2fs/dir.cc
+++ b/src/storage/f2fs/dir.cc
@@ -328,7 +328,6 @@
/* copy dentry info. to this inode page */
rn = static_cast<Node *>(PageAddress(ipage));
- rn->i.i_pino = CpuToLe(Ino());
rn->i.i_namelen = CpuToLe(vnode->GetNameLen());
memcpy(rn->i.i_name, vnode->GetName(), vnode->GetNameLen());
#if 0 // porting needed
diff --git a/src/storage/f2fs/namei.cc b/src/storage/f2fs/namei.cc
index f59cd00..3fee78b 100644
--- a/src/storage/f2fs/namei.cc
+++ b/src/storage/f2fs/namei.cc
@@ -539,6 +539,7 @@
}
}
+ old_vnode->SetParentNid(new_dir->Ino());
old_vnode->SetCTime(cur_time);
old_vnode->SetFlag(InodeInfoFlag::kNeedCp);
old_vnode->MarkInodeDirty();
diff --git a/src/storage/f2fs/test/unit/inline.cc b/src/storage/f2fs/test/unit/inline.cc
index 24f516b..c9561cc 100644
--- a/src/storage/f2fs/test/unit/inline.cc
+++ b/src/storage/f2fs/test/unit/inline.cc
@@ -285,5 +285,90 @@
EXPECT_EQ(Fsck(std::move(bc)), ZX_OK);
}
+TEST(InlineDirTest, InlineDirPino) {
+ std::unique_ptr<Bcache> bc;
+ FileTester::MkfsOnFakeDev(&bc);
+
+ std::unique_ptr<F2fs> fs;
+ MountOptions options{};
+
+ // Enable inline dir option
+ ASSERT_EQ(options.SetValue(options.GetNameView(kOptInlineDentry), 1), ZX_OK);
+ async::Loop loop(&kAsyncLoopConfigAttachToCurrentThread);
+ FileTester::MountWithOptions(loop.dispatcher(), options, &bc, &fs);
+
+ fbl::RefPtr<VnodeF2fs> root;
+ FileTester::CreateRoot(fs.get(), &root);
+
+ fbl::RefPtr<Dir> root_dir = fbl::RefPtr<Dir>::Downcast(std::move(root));
+
+ // Inline dir creation
+ fbl::RefPtr<fs::Vnode> vnode;
+ ASSERT_EQ(root_dir->Create("a", S_IFDIR, &vnode), ZX_OK);
+ fbl::RefPtr<Dir> a_dir = fbl::RefPtr<Dir>::Downcast(std::move(vnode));
+ ASSERT_EQ(a_dir->GetParentNid(), root_dir->Ino());
+
+ ASSERT_EQ(root_dir->Create("b", S_IFDIR, &vnode), ZX_OK);
+ fbl::RefPtr<Dir> b_dir = fbl::RefPtr<Dir>::Downcast(std::move(vnode));
+ ASSERT_EQ(b_dir->GetParentNid(), root_dir->Ino());
+
+ ASSERT_EQ(a_dir->Create("c", S_IFDIR, &vnode), ZX_OK);
+ fbl::RefPtr<Dir> c_dir = fbl::RefPtr<Dir>::Downcast(std::move(vnode));
+ ASSERT_EQ(c_dir->GetParentNid(), a_dir->Ino());
+
+ ASSERT_EQ(a_dir->Create("d", S_IFREG, &vnode), ZX_OK);
+ fbl::RefPtr<File> d1_file = fbl::RefPtr<File>::Downcast(std::move(vnode));
+ ASSERT_EQ(d1_file->GetParentNid(), a_dir->Ino());
+
+ ASSERT_EQ(b_dir->Create("d", S_IFREG, &vnode), ZX_OK);
+ fbl::RefPtr<File> d2_file = fbl::RefPtr<File>::Downcast(std::move(vnode));
+ ASSERT_EQ(d2_file->GetParentNid(), b_dir->Ino());
+
+ // rename "/a/c" to "/b/c" and "/a/d" to "/b/d"
+ ASSERT_EQ(a_dir->Rename(b_dir, "c", "c", true, true), ZX_OK);
+ ASSERT_EQ(a_dir->Rename(b_dir, "d", "d", false, false), ZX_OK);
+
+ // Check i_pino of renamed directory
+ ASSERT_EQ(c_dir->GetParentNid(), b_dir->Ino());
+ ASSERT_EQ(d1_file->GetParentNid(), b_dir->Ino());
+
+ ASSERT_EQ(d1_file->Close(), ZX_OK);
+ ASSERT_EQ(d2_file->Close(), ZX_OK);
+ ASSERT_EQ(c_dir->Close(), ZX_OK);
+ ASSERT_EQ(b_dir->Close(), ZX_OK);
+ ASSERT_EQ(a_dir->Close(), ZX_OK);
+ ASSERT_EQ(root_dir->Close(), ZX_OK);
+ root_dir = a_dir = b_dir = c_dir = nullptr;
+ d1_file = d2_file = nullptr;
+
+ // Remount
+ FileTester::Unmount(std::move(fs), &bc);
+ FileTester::MountWithOptions(loop.dispatcher(), options, &bc, &fs);
+
+ FileTester::CreateRoot(fs.get(), &root);
+ root_dir = fbl::RefPtr<Dir>::Downcast(std::move(root));
+
+ FileTester::Lookup(root_dir.get(), "b", &vnode);
+ b_dir = fbl::RefPtr<Dir>::Downcast(std::move(vnode));
+ FileTester::Lookup(b_dir.get(), "c", &vnode);
+ c_dir = fbl::RefPtr<Dir>::Downcast(std::move(vnode));
+ FileTester::Lookup(b_dir.get(), "d", &vnode);
+ d1_file = fbl::RefPtr<File>::Downcast(std::move(vnode));
+
+ // Check i_pino of renamed directory
+ ASSERT_EQ(c_dir->GetParentNid(), b_dir->Ino());
+ ASSERT_EQ(d1_file->GetParentNid(), b_dir->Ino());
+
+ ASSERT_EQ(d1_file->Close(), ZX_OK);
+ ASSERT_EQ(c_dir->Close(), ZX_OK);
+ ASSERT_EQ(b_dir->Close(), ZX_OK);
+ ASSERT_EQ(root_dir->Close(), ZX_OK);
+ root_dir = b_dir = c_dir = nullptr;
+ d1_file = nullptr;
+
+ FileTester::Unmount(std::move(fs), &bc);
+ EXPECT_EQ(Fsck(std::move(bc), &bc), ZX_OK);
+}
+
} // namespace
} // namespace f2fs
diff --git a/src/storage/f2fs/vnode.cc b/src/storage/f2fs/vnode.cc
index 9e5a3a2..20f848f0 100644
--- a/src/storage/f2fs/vnode.cc
+++ b/src/storage/f2fs/vnode.cc
@@ -319,6 +319,7 @@
ri->i_current_depth = CpuToLe(static_cast<uint32_t>(GetCurDirDepth()));
ri->i_xattr_nid = CpuToLe(GetXattrNid());
ri->i_flags = CpuToLe(GetInodeFlags());
+ ri->i_pino = CpuToLe(GetParentNid());
ri->i_generation = CpuToLe(GetGeneration());
ri->i_dir_level = GetDirLevel();
