Back to the problem, if we skip out, then the f2fs_gc will go
into dead loop if the apps only atomic start but never atomic
commit. The main aim of my two patches is to remove the skip
action to avoid the dead loop.
On 2018/2/9 21:26, Chao Yu wrote:
On 2018/2/9 20:56, Yunlong Song wrote:
As what I point in last mail, if the atomic file is not committed
yet, gc_data_segment will register_inmem_page the GCed data pages.
We will skip GCing that page as below check:
- move_data_{page,block}
- f2fs_is_atomic_file()
skip out;
No?
Thanks,
This will cause these data pages written twice, the first write
happens in move_data_page->do_write_data_page, and the second
write happens in later __commit_inmem_pages->do_write_data_page.
On 2018/2/9 20:44, Chao Yu wrote:
On 2018/2/8 11:11, Yunlong Song wrote:
Then the GCed data pages are totally mixed with the inmem atomic pages,
If we add dio_rwsem, GC flow is exclude with atomic write flow. There
will be not race case to mix GCed page into atomic pages.
Or you mean:
- gc_data_segment
- move_data_page
- f2fs_is_atomic_file
- f2fs_ioc_start_atomic_write
- set_inode_flag(inode, FI_ATOMIC_FILE);
- f2fs_set_data_page_dirty
- register_inmem_page
In this case, GCed page can be mixed into database transaction, but could
it cause any problem except break rule of isolation for transaction.
this will cause the atomic commit ops write the GCed data pages twice
(the first write happens in GC).
How about using the early two patches to separate the inmem data pages
and GCed data pages, and use dio_rwsem instead of this patch to fix the
dnode page problem (dnode page commited but data page are not committed
for the GCed page)?
Could we fix the race case first, based on that fixing, and then find the
place that we can improve?
On 2018/2/7 20:16, Chao Yu wrote:
On 2018/2/6 11:49, Yunlong Song wrote:
This patch adds fi->commit_lock to avoid the case that GCed node pages
are committed but GCed data pages are not committed. This can avoid the
db file run into inconsistent state when sudden-power-off happens if
data pages of atomic file is allowed to be GCed before.
do_fsync: GC:
- mutex_lock(&fi->commit_lock);
- lock_page()
- mutex_lock(&fi->commit_lock);
- lock_page()
Well, please consider lock dependency & code complexity, IMO, reuse
fi->dio_rwsem[WRITE] will be enough as below:
---
fs/f2fs/file.c | 3 +++
fs/f2fs/gc.c | 5 -----
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 672a542e5464..1bdc11feb8d0 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1711,6 +1711,8 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
inode_lock(inode);
+ down_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
+
if (f2fs_is_volatile_file(inode))
goto err_out;
@@ -1729,6 +1731,7 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
}
err_out:
+ up_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
inode_unlock(inode);
mnt_drop_write_file(filp);
return ret;
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index b9d93fd532a9..e49416283563 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -622,9 +622,6 @@ static void move_data_block(struct inode *inode, block_t
bidx,
if (!check_valid_map(F2FS_I_SB(inode), segno, off))
goto out;
- if (f2fs_is_atomic_file(inode))
- goto out;
Seems that we need this check.
-
if (f2fs_is_pinned_file(inode)) {
f2fs_pin_file_control(inode, true);
goto out;
@@ -729,8 +726,6 @@ static void move_data_page(struct inode *inode, block_t
bidx, int gc_type,
if (!check_valid_map(F2FS_I_SB(inode), segno, off))
goto out;
- if (f2fs_is_atomic_file(inode))
- goto out;
Ditto.
Thanks,
if (f2fs_is_pinned_file(inode)) {
if (gc_type == FG_GC)
f2fs_pin_file_control(inode, true);
.
.
--
Thanks,
Yunlong Song
