[PATCH 1/7] f2fs: avoid needless lock for node pages when fsyncing a file

[PATCH 1/7] f2fs: avoid needless lock for node pages when fsyncing a file

[Jaegeuk Kim]

When fsync is called, sync_node_pages finds a proper direct node pages to flush.
But, it locks unrelated direct node pages together unnecessarily.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/node.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 095fc2c..cccee50 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1272,10 +1272,14 @@ next_step:
 			 * we should not skip writing node pages.
 			 */
 lock_node:
-			if (ino && ino_of_node(page) == ino)
-				lock_page(page);
-			else if (!trylock_page(page))
+			if (ino) {
+				if (ino_of_node(page) == ino)
+					lock_page(page);
+				else
+					continue;
+			} else if (!trylock_page(page)) {
 				continue;
+			}
 
 			if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
 continue_unlock:
-- 
2.6.3

[PATCH 2/7] f2fs: avoid writing 0'th page in volatile writes

[Jaegeuk Kim]

The first page of volatile writes usually contains a sort of header information
which will be used for recovery.
(e.g., journal header of sqlite)

If this is written without other journal data, user needs to handle the stale
journal information.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index c29bcf4..e54489b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1177,8 +1177,10 @@ write:
 		goto redirty_out;
 	if (f2fs_is_drop_cache(inode))
 		goto out;
-	if (f2fs_is_volatile_file(inode) && !wbc->for_reclaim &&
-			available_free_memory(sbi, BASE_CHECK))
+	/* we should not write 0'th page having journal header */
+	if (f2fs_is_volatile_file(inode) && (!page->index ||
+			(!wbc->for_reclaim &&
+			available_free_memory(sbi, BASE_CHECK))))
 		goto redirty_out;
 
 	/* Dentry blocks are controlled by checkpoint */
-- 
2.6.3

[PATCH 3/7] f2fs: split sync_node_pages with fsync_node_pages

[Jaegeuk Kim]

This patch splits the existing sync_node_pages into (f)sync_node_pages.
The fsync_node_pages is used for f2fs_sync_file only.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c |   2 +-
 fs/f2fs/f2fs.h       |   3 +-
 fs/f2fs/file.c       |   2 +-
 fs/f2fs/gc.c         |   2 +-
 fs/f2fs/node.c       | 108 +++++++++++++++++++++++++++++++++++++--------------
 5 files changed, 84 insertions(+), 33 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index b92782f..bf040b5 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -892,7 +892,7 @@ retry_flush_nodes:
 
 	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
 		up_write(&sbi->node_write);
-		err = sync_node_pages(sbi, 0, &wbc);
+		err = sync_node_pages(sbi, &wbc);
 		if (err) {
 			f2fs_unlock_all(sbi);
 			goto out;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 3f15513..269abe5 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1784,7 +1784,8 @@ void ra_node_page(struct f2fs_sb_info *, nid_t);
 struct page *get_node_page(struct f2fs_sb_info *, pgoff_t);
 struct page *get_node_page_ra(struct page *, int);
 void sync_inode_page(struct dnode_of_data *);
-int sync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *);
+int fsync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *);
+int sync_node_pages(struct f2fs_sb_info *, struct writeback_control *);
 bool alloc_nid(struct f2fs_sb_info *, nid_t *);
 void alloc_nid_done(struct f2fs_sb_info *, nid_t);
 void alloc_nid_failed(struct f2fs_sb_info *, nid_t);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 7de90e6..3d53ee0 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -256,7 +256,7 @@ go_write:
 		goto out;
 	}
 sync_nodes:
-	sync_node_pages(sbi, ino, &wbc);
+	fsync_node_pages(sbi, ino, &wbc);
 
 	/* if cp_error was enabled, we should avoid infinite loop */
 	if (unlikely(f2fs_cp_error(sbi))) {
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index b0051a9..e820465 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -841,7 +841,7 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
 				.nr_to_write = LONG_MAX,
 				.for_reclaim = 0,
 			};
-			sync_node_pages(sbi, 0, &wbc);
+			sync_node_pages(sbi, &wbc);
 		} else {
 			f2fs_submit_merged_bio(sbi, DATA, WRITE);
 		}
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index cccee50..675b730 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1222,12 +1222,84 @@ iput_out:
 	iput(inode);
 }
 
-int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
+int fsync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 					struct writeback_control *wbc)
 {
 	pgoff_t index, end;
 	struct pagevec pvec;
-	int step = ino ? 2 : 0;
+	int nwritten = 0;
+
+	pagevec_init(&pvec, 0);
+	index = 0;
+	end = ULONG_MAX;
+
+	while (index <= end) {
+		int i, nr_pages;
+		nr_pages = pagevec_lookup_tag(&pvec, NODE_MAPPING(sbi), &index,
+				PAGECACHE_TAG_DIRTY,
+				min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1);
+		if (nr_pages == 0)
+			break;
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page *page = pvec.pages[i];
+
+			if (unlikely(f2fs_cp_error(sbi))) {
+				pagevec_release(&pvec);
+				return -EIO;
+			}
+
+			if (!IS_DNODE(page) || !is_cold_node(page))
+				continue;
+			if (ino_of_node(page) != ino)
+				continue;
+
+			lock_page(page);
+
+			if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
+continue_unlock:
+				unlock_page(page);
+				continue;
+			}
+			if (ino_of_node(page) != ino)
+				goto continue_unlock;
+
+			if (!PageDirty(page)) {
+				/* someone wrote it for us */
+				goto continue_unlock;
+			}
+
+			f2fs_wait_on_page_writeback(page, NODE, true);
+			BUG_ON(PageWriteback(page));
+			if (!clear_page_dirty_for_io(page))
+				goto continue_unlock;
+
+			set_fsync_mark(page, 1);
+			if (IS_INODE(page))
+				set_dentry_mark(page,
+						need_dentry_mark(sbi, ino));
+			nwritten++;
+
+			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
+				unlock_page(page);
+
+			if (--wbc->nr_to_write == 0)
+				break;
+		}
+		pagevec_release(&pvec);
+		cond_resched();
+
+		if (wbc->nr_to_write == 0)
+			break;
+	}
+	return nwritten;
+}
+
+int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
+{
+	pgoff_t index, end;
+	struct pagevec pvec;
+	int step = 0;
 	int nwritten = 0;
 
 	pagevec_init(&pvec, 0);
@@ -1266,28 +1338,15 @@ next_step:
 			if (step == 2 && (!IS_DNODE(page) ||
 						!is_cold_node(page)))
 				continue;
-
-			/*
-			 * If an fsync mode,
-			 * we should not skip writing node pages.
-			 */
 lock_node:
-			if (ino) {
-				if (ino_of_node(page) == ino)
-					lock_page(page);
-				else
-					continue;
-			} else if (!trylock_page(page)) {
+			if (!trylock_page(page))
 				continue;
-			}
 
 			if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
 continue_unlock:
 				unlock_page(page);
 				continue;
 			}
-			if (ino && ino_of_node(page) != ino)
-				goto continue_unlock;
 
 			if (!PageDirty(page)) {
 				/* someone wrote it for us */
@@ -1295,7 +1354,7 @@ continue_unlock:
 			}
 
 			/* flush inline_data */
-			if (!ino && is_inline_node(page)) {
+			if (is_inline_node(page)) {
 				clear_inline_node(page);
 				unlock_page(page);
 				flush_inline_data(sbi, ino_of_node(page));
@@ -1308,17 +1367,8 @@ continue_unlock:
 			if (!clear_page_dirty_for_io(page))
 				goto continue_unlock;
 
-			/* called by fsync() */
-			if (ino && IS_DNODE(page)) {
-				set_fsync_mark(page, 1);
-				if (IS_INODE(page))
-					set_dentry_mark(page,
-						need_dentry_mark(sbi, ino));
-				nwritten++;
-			} else {
-				set_fsync_mark(page, 0);
-				set_dentry_mark(page, 0);
-			}
+			set_fsync_mark(page, 0);
+			set_dentry_mark(page, 0);
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
@@ -1466,7 +1516,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
 
 	diff = nr_pages_to_write(sbi, NODE, wbc);
 	wbc->sync_mode = WB_SYNC_NONE;
-	sync_node_pages(sbi, 0, wbc);
+	sync_node_pages(sbi, wbc);
 	wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
 	return 0;
 
-- 
2.6.3

[PATCH 4/7] f2fs: report unwritten status in fsync_node_pages

[Jaegeuk Kim]

The fsync_node_pages should return pass or failure so that user could know
fsync is completed or not.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c |  4 +++-
 fs/f2fs/node.c | 13 ++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3d53ee0..60fd64c 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -256,7 +256,9 @@ go_write:
 		goto out;
 	}
 sync_nodes:
-	fsync_node_pages(sbi, ino, &wbc);
+	ret = fsync_node_pages(sbi, ino, &wbc);
+	if (ret)
+		goto out;
 
 	/* if cp_error was enabled, we should avoid infinite loop */
 	if (unlikely(f2fs_cp_error(sbi))) {
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 675b730..8a1e211 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1227,7 +1227,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 {
 	pgoff_t index, end;
 	struct pagevec pvec;
-	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 	index = 0;
@@ -1278,21 +1278,20 @@ continue_unlock:
 			if (IS_INODE(page))
 				set_dentry_mark(page,
 						need_dentry_mark(sbi, ino));
-			nwritten++;
 
-			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
+			ret = NODE_MAPPING(sbi)->a_ops->writepage(page, wbc);
+			if (ret) {
 				unlock_page(page);
-
-			if (--wbc->nr_to_write == 0)
 				break;
+			}
 		}
 		pagevec_release(&pvec);
 		cond_resched();
 
-		if (wbc->nr_to_write == 0)
+		if (ret)
 			break;
 	}
-	return nwritten;
+	return ret ? -EIO: 0;
 }
 
 int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
-- 
2.6.3

[PATCH 5/7] f2fs: set fsync mark only for the last dnode

[Jaegeuk Kim]

In order to give atomic writes, we should consider power failure during
sync_node_pages in fsync.
So, this patch marks fsync flag only in the last dnode block.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h     |   4 +-
 fs/f2fs/file.c     |  14 +++++--
 fs/f2fs/node.c     | 106 +++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/f2fs/recovery.c |   9 ++---
 4 files changed, 113 insertions(+), 20 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 269abe5..ca828b0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -159,7 +159,6 @@ struct fsync_inode_entry {
 	struct inode *inode;	/* vfs inode pointer */
 	block_t blkaddr;	/* block address locating the last fsync */
 	block_t last_dentry;	/* block address locating the last dentry */
-	block_t last_inode;	/* block address locating the last inode */
 };
 
 #define nats_in_cursum(jnl)		(le16_to_cpu(jnl->n_nats))
@@ -1784,7 +1783,8 @@ void ra_node_page(struct f2fs_sb_info *, nid_t);
 struct page *get_node_page(struct f2fs_sb_info *, pgoff_t);
 struct page *get_node_page_ra(struct page *, int);
 void sync_inode_page(struct dnode_of_data *);
-int fsync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *);
+int fsync_node_pages(struct f2fs_sb_info *, nid_t, struct writeback_control *,
+								bool);
 int sync_node_pages(struct f2fs_sb_info *, struct writeback_control *);
 bool alloc_nid(struct f2fs_sb_info *, nid_t *);
 void alloc_nid_done(struct f2fs_sb_info *, nid_t);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 60fd64c..dc47d5c 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -182,7 +182,8 @@ static void try_to_fix_pino(struct inode *inode)
 	}
 }
 
-int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
+static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
+						int datasync, bool atomic)
 {
 	struct inode *inode = file->f_mapping->host;
 	struct f2fs_inode_info *fi = F2FS_I(inode);
@@ -256,7 +257,7 @@ go_write:
 		goto out;
 	}
 sync_nodes:
-	ret = fsync_node_pages(sbi, ino, &wbc);
+	ret = fsync_node_pages(sbi, ino, &wbc, atomic);
 	if (ret)
 		goto out;
 
@@ -290,6 +291,11 @@ out:
 	return ret;
 }
 
+int f2fs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
+{
+	return f2fs_do_sync_file(file, start, end, datasync, false);
+}
+
 static pgoff_t __get_first_dirty_index(struct address_space *mapping,
 						pgoff_t pgofs, int whence)
 {
@@ -1407,7 +1413,7 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 		}
 	}
 
-	ret = f2fs_sync_file(filp, 0, LLONG_MAX, 0);
+	ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
 err_out:
 	mnt_drop_write_file(filp);
 	return ret;
@@ -1465,7 +1471,7 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 		drop_inmem_pages(inode);
 	if (f2fs_is_volatile_file(inode)) {
 		clear_inode_flag(F2FS_I(inode), FI_VOLATILE_FILE);
-		ret = f2fs_sync_file(filp, 0, LLONG_MAX, 0);
+		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
 	}
 
 	mnt_drop_write_file(filp);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 8a1e211..de070a5 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1222,13 +1222,81 @@ iput_out:
 	iput(inode);
 }
 
+static struct page *last_fsync_dnode(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	pgoff_t index, end;
+	struct pagevec pvec;
+	struct page *last_page = NULL;
+
+	pagevec_init(&pvec, 0);
+	index = 0;
+	end = ULONG_MAX;
+
+	while (index <= end) {
+		int i, nr_pages;
+		nr_pages = pagevec_lookup_tag(&pvec, NODE_MAPPING(sbi), &index,
+				PAGECACHE_TAG_DIRTY,
+				min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1);
+		if (nr_pages == 0)
+			break;
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page *page = pvec.pages[i];
+
+			if (unlikely(f2fs_cp_error(sbi))) {
+				f2fs_put_page(last_page, 0);
+				pagevec_release(&pvec);
+				return ERR_PTR(-EIO);
+			}
+
+			if (!IS_DNODE(page) || !is_cold_node(page))
+				continue;
+			if (ino_of_node(page) != ino)
+				continue;
+
+			lock_page(page);
+
+			if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
+continue_unlock:
+				unlock_page(page);
+				continue;
+			}
+			if (ino_of_node(page) != ino)
+				goto continue_unlock;
+
+			if (!PageDirty(page)) {
+				/* someone wrote it for us */
+				goto continue_unlock;
+			}
+
+			if (last_page)
+				f2fs_put_page(last_page, 0);
+
+			get_page(page);
+			last_page = page;
+			unlock_page(page);
+		}
+		pagevec_release(&pvec);
+		cond_resched();
+	}
+	return last_page;
+}
+
 int fsync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
-					struct writeback_control *wbc)
+			struct writeback_control *wbc, bool atomic)
 {
 	pgoff_t index, end;
 	struct pagevec pvec;
 	int ret = 0;
+	struct page *last_page = NULL;
+	bool marked = false;
 
+	if (atomic) {
+		last_page = last_fsync_dnode(sbi, ino);
+		if (IS_ERR_OR_NULL(last_page))
+			return PTR_ERR_OR_ZERO(last_page);
+	}
+retry:
 	pagevec_init(&pvec, 0);
 	index = 0;
 	end = ULONG_MAX;
@@ -1245,6 +1313,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 			struct page *page = pvec.pages[i];
 
 			if (unlikely(f2fs_cp_error(sbi))) {
+				f2fs_put_page(last_page, 0);
 				pagevec_release(&pvec);
 				return -EIO;
 			}
@@ -1264,33 +1333,54 @@ continue_unlock:
 			if (ino_of_node(page) != ino)
 				goto continue_unlock;
 
-			if (!PageDirty(page)) {
+			if (!PageDirty(page) && page != last_page) {
 				/* someone wrote it for us */
 				goto continue_unlock;
 			}
 
 			f2fs_wait_on_page_writeback(page, NODE, true);
 			BUG_ON(PageWriteback(page));
-			if (!clear_page_dirty_for_io(page))
-				goto continue_unlock;
 
-			set_fsync_mark(page, 1);
-			if (IS_INODE(page))
-				set_dentry_mark(page,
+			if (!atomic || page == last_page) {
+				set_fsync_mark(page, 1);
+				if (IS_INODE(page))
+					set_dentry_mark(page,
 						need_dentry_mark(sbi, ino));
+				/*  may be written by other thread */
+				if (!PageDirty(page))
+					set_page_dirty(page);
+			}
+
+			if (!clear_page_dirty_for_io(page))
+				goto continue_unlock;
 
 			ret = NODE_MAPPING(sbi)->a_ops->writepage(page, wbc);
 			if (ret) {
 				unlock_page(page);
+				f2fs_put_page(last_page, 0);
+				break;
+			}
+			if (page == last_page) {
+				f2fs_put_page(page, 0);
+				marked = true;
 				break;
 			}
 		}
 		pagevec_release(&pvec);
 		cond_resched();
 
-		if (ret)
+		if (ret || marked)
 			break;
 	}
+	if (!ret && atomic && !marked) {
+		f2fs_msg(sbi->sb, KERN_DEBUG,
+			"Retry to write fsync mark: ino=%u, idx=%lx",
+					ino, last_page->index);
+		lock_page(last_page);
+		set_page_dirty(last_page);
+		unlock_page(last_page);
+		goto retry;
+	}
 	return ret ? -EIO: 0;
 }
 
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 2c87c12..a646d3b 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -257,11 +257,8 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 		}
 		entry->blkaddr = blkaddr;
 
-		if (IS_INODE(page)) {
-			entry->last_inode = blkaddr;
-			if (is_dent_dnode(page))
-				entry->last_dentry = blkaddr;
-		}
+		if (IS_INODE(page) && is_dent_dnode(page))
+			entry->last_dentry = blkaddr;
 next:
 		/* check next segment */
 		blkaddr = next_blkaddr_of_node(page);
@@ -521,7 +518,7 @@ static int recover_data(struct f2fs_sb_info *sbi, struct list_head *head)
 		 * In this case, we can lose the latest inode(x).
 		 * So, call recover_inode for the inode update.
 		 */
-		if (entry->last_inode == blkaddr)
+		if (IS_INODE(page))
 			recover_inode(entry->inode, page);
 		if (entry->last_dentry == blkaddr) {
 			err = recover_dentry(entry->inode, page);
-- 
2.6.3

[PATCH 6/7] f2fs: issue cache flush on direct IO

[Jaegeuk Kim]

Under direct IO path with O_(D)SYNC, it needs to set proper APPEND or UPDATE
flags, so taht f2fs_sync_file can make its data safe.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/data.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index e54489b..38ce5d6 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -673,6 +673,9 @@ next_block:
 					err = reserve_new_block(&dn);
 			} else {
 				err = __allocate_data_block(&dn);
+				if (!err)
+					set_inode_flag(F2FS_I(inode),
+							FI_APPEND_WRITE);
 			}
 			if (err)
 				goto sync_out;
@@ -1685,8 +1688,12 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
 	err = blockdev_direct_IO(iocb, inode, iter, offset, get_data_block_dio);
-	if (err < 0 && iov_iter_rw(iter) == WRITE)
-		f2fs_write_failed(mapping, offset + count);
+	if (iov_iter_rw(iter) == WRITE) {
+		if (err > 0)
+			set_inode_flag(F2FS_I(inode), FI_UPDATE_WRITE);
+		else if (err < 0)
+			f2fs_write_failed(mapping, offset + count);
+	}
 
 	trace_f2fs_direct_IO_exit(inode, offset, count, iov_iter_rw(iter), err);
 
-- 
2.6.3

[PATCH 7/7] f2fs: should check the remaining dentry bits

[Jaegeuk Kim]

Let's consider a race condition between f2fs_add_regular_entry and
find_target_dentry.

1.
- f2fs_add_regular_entry updated len: 24 first.
       |
Bits:  0 0 0 1
Lens: 24 0 0 3 (name: foo)
       |->
- find_target_dentry checks the first bit to find "foo", then ++pointer.

2.
- f2fs_add_regular_entry updates bits.
       |>|>|
Bits:  1 1 1 1
Lens: 24 0 0 3 (name: foo)
         |
- find_target_dentry is checking second bit, but it's len is zero, which
makes the process being terminated.

In this case, user can add additional dentry named "foo" accordingly.
This patch enables to check the remaining bits.

Signed-off-by: He YunLei <heyunlei@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e90380d..aed5e6d 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -149,9 +149,9 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname,
 
 		/* remain bug on condition */
 		if (unlikely(!de->name_len))
-			d->max = -1;
-
-		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
+			bit_pos++;
+		else
+			bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}
 
 	de = NULL;
-- 
2.6.3

Re: [PATCH v2 7/7] f2fs: should check the remaining dentry bits

[Jaegeuk Kim]

Change log from v1:
 o remove wron f2fs_bug_on()

>From 545c0c9055b0d8dc5d134d9340b3cd80eeecdafa Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Mon, 25 Apr 2016 14:24:44 -0700
Subject: [PATCH] f2fs: should check the remaining dentry bits

Let's consider a race condition between f2fs_add_regular_entry and
find_target_dentry.

1.
- f2fs_add_regular_entry updated len: 24 first.
       |
Bits:  0 0 0 1
Lens: 24 0 0 3 (name: foo)
       |->
- find_target_dentry checks the first bit to find "foo", then ++pointer.

2.
- f2fs_add_regular_entry updates bits.
       |>|>|
Bits:  1 1 1 1
Lens: 24 0 0 3 (name: foo)
         |
- find_target_dentry is checking second bit, but it's len is zero, which
makes the process being terminated.

In this case, user can add additional dentry named "foo" accordingly.
This patch enables to check the remaining bits.

Signed-off-by: He YunLei <heyunlei@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e90380d..0ad7b9a 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -100,12 +100,6 @@ static struct f2fs_dir_entry *find_in_block(struct page *dentry_page,
 		*res_page = dentry_page;
 	else
 		kunmap(dentry_page);
-
-	/*
-	 * For the most part, it should be a bug when name_len is zero.
-	 * We stop here for figuring out where the bugs has occurred.
-	 */
-	f2fs_bug_on(F2FS_P_SB(dentry_page), d.max < 0);
 	return de;
 }
 
@@ -149,9 +143,9 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname,
 
 		/* remain bug on condition */
 		if (unlikely(!de->name_len))
-			d->max = -1;
-
-		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
+			bit_pos++;
+		else
+			bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}
 
 	de = NULL;
-- 
2.6.3

Re: [f2fs-dev] [PATCH 7/7] f2fs: should check the remaining dentry bits

[Chao Yu]

Hi Jaegeuk, Yunlei,

On 2016/4/26 8:07, Jaegeuk Kim wrote:
> Let's consider a race condition between f2fs_add_regular_entry and
> find_target_dentry.
> 
> 1.
> - f2fs_add_regular_entry updated len: 24 first.
>        |
> Bits:  0 0 0 1
> Lens: 24 0 0 3 (name: foo)
>        |->
> - find_target_dentry checks the first bit to find "foo", then ++pointer.
> 
> 2.
> - f2fs_add_regular_entry updates bits.
>        |>|>|
> Bits:  1 1 1 1
> Lens: 24 0 0 3 (name: foo)
>          |
> - find_target_dentry is checking second bit, but it's len is zero, which
> makes the process being terminated.

As Pengyang reminded, there are no racing condition between find_target_dentry
and f2fs_add_regular_entry since i_mutex lock make each of operations being
atomical. So seems above condition can not happen.

But still we should handle dirent with zero-sized length correctly, as it may
cause deadloop. So how do you think of following patch?

From: Chao Yu <yuchao0@huawei.com>
Subject: [PATCH] f2fs: be aware of invalid filename length

The filename length in dirent of may become zero-sized after random junk
data injection, once encounter such dirent, find_target_dentry or
f2fs_add_inline_entries will run into an infinite loop. So let f2fs being
aware of that to avoid deadloop.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/dir.c    | 14 +++++---------
 fs/f2fs/inline.c | 14 ++++++--------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e90380d..3b1c14e 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -101,11 +101,6 @@ static struct f2fs_dir_entry *find_in_block(struct page
*dentry_page,
 	else
 		kunmap(dentry_page);

-	/*
-	 * For the most part, it should be a bug when name_len is zero.
-	 * We stop here for figuring out where the bugs has occurred.
-	 */
-	f2fs_bug_on(F2FS_P_SB(dentry_page), d.max < 0);
 	return de;
 }

@@ -130,6 +125,11 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,

 		de = &d->dentry[bit_pos];

+		if (unlikely(!de->name_len)) {
+			bit_pos++;
+			continue;
+		}
+
 		/* encrypted case */
 		de_name.name = d->filename[bit_pos];
 		de_name.len = le16_to_cpu(de->name_len);
@@ -147,10 +147,6 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,
 			*max_slots = max_len;
 		max_len = 0;

-		/* remain bug on condition */
-		if (unlikely(!de->name_len))
-			d->max = -1;
-
 		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}

diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 7720565..e61084c 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -303,11 +303,6 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode *dir,
 	else
 		f2fs_put_page(ipage, 0);

-	/*
-	 * For the most part, it should be a bug when name_len is zero.
-	 * We stop here for figuring out where the bugs has occurred.
-	 */
-	f2fs_bug_on(sbi, d.max < 0);
 	return de;
 }

@@ -437,6 +432,12 @@ static int f2fs_add_inline_entries(struct inode *dir,
 		}

 		de = &d.dentry[bit_pos];
+
+		if (unlikely(!de->name_len)) {
+			bit_pos++;
+			continue;
+		}
+
 		new_name.name = d.filename[bit_pos];
 		new_name.len = de->name_len;

@@ -448,9 +449,6 @@ static int f2fs_add_inline_entries(struct inode *dir,
 		if (err)
 			goto punch_dentry_pages;

-		if (unlikely(!de->name_len))
-			d.max = -1;
-
 		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
 	}
 	return 0;
-- 
2.7.2

Re: [f2fs-dev] [PATCH 7/7] f2fs: should check the remaining dentry bits

[Jaegeuk Kim]

Looks better.
Merged. :)

Thanks,

On Wed, Apr 27, 2016 at 10:22:20PM +0800, Chao Yu wrote:
> Hi Jaegeuk, Yunlei,
> 
> On 2016/4/26 8:07, Jaegeuk Kim wrote:
> > Let's consider a race condition between f2fs_add_regular_entry and
> > find_target_dentry.
> > 
> > 1.
> > - f2fs_add_regular_entry updated len: 24 first.
> >        |
> > Bits:  0 0 0 1
> > Lens: 24 0 0 3 (name: foo)
> >        |->
> > - find_target_dentry checks the first bit to find "foo", then ++pointer.
> > 
> > 2.
> > - f2fs_add_regular_entry updates bits.
> >        |>|>|
> > Bits:  1 1 1 1
> > Lens: 24 0 0 3 (name: foo)
> >          |
> > - find_target_dentry is checking second bit, but it's len is zero, which
> > makes the process being terminated.
> 
> As Pengyang reminded, there are no racing condition between find_target_dentry
> and f2fs_add_regular_entry since i_mutex lock make each of operations being
> atomical. So seems above condition can not happen.
> 
> But still we should handle dirent with zero-sized length correctly, as it may
> cause deadloop. So how do you think of following patch?
> 
> From: Chao Yu <yuchao0@huawei.com>
> Subject: [PATCH] f2fs: be aware of invalid filename length
> 
> The filename length in dirent of may become zero-sized after random junk
> data injection, once encounter such dirent, find_target_dentry or
> f2fs_add_inline_entries will run into an infinite loop. So let f2fs being
> aware of that to avoid deadloop.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/dir.c    | 14 +++++---------
>  fs/f2fs/inline.c | 14 ++++++--------
>  2 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index e90380d..3b1c14e 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -101,11 +101,6 @@ static struct f2fs_dir_entry *find_in_block(struct page
> *dentry_page,
>  	else
>  		kunmap(dentry_page);
> 
> -	/*
> -	 * For the most part, it should be a bug when name_len is zero.
> -	 * We stop here for figuring out where the bugs has occurred.
> -	 */
> -	f2fs_bug_on(F2FS_P_SB(dentry_page), d.max < 0);
>  	return de;
>  }
> 
> @@ -130,6 +125,11 @@ struct f2fs_dir_entry *find_target_dentry(struct
> fscrypt_name *fname,
> 
>  		de = &d->dentry[bit_pos];
> 
> +		if (unlikely(!de->name_len)) {
> +			bit_pos++;
> +			continue;
> +		}
> +
>  		/* encrypted case */
>  		de_name.name = d->filename[bit_pos];
>  		de_name.len = le16_to_cpu(de->name_len);
> @@ -147,10 +147,6 @@ struct f2fs_dir_entry *find_target_dentry(struct
> fscrypt_name *fname,
>  			*max_slots = max_len;
>  		max_len = 0;
> 
> -		/* remain bug on condition */
> -		if (unlikely(!de->name_len))
> -			d->max = -1;
> -
>  		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
>  	}
> 
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index 7720565..e61084c 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -303,11 +303,6 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode *dir,
>  	else
>  		f2fs_put_page(ipage, 0);
> 
> -	/*
> -	 * For the most part, it should be a bug when name_len is zero.
> -	 * We stop here for figuring out where the bugs has occurred.
> -	 */
> -	f2fs_bug_on(sbi, d.max < 0);
>  	return de;
>  }
> 
> @@ -437,6 +432,12 @@ static int f2fs_add_inline_entries(struct inode *dir,
>  		}
> 
>  		de = &d.dentry[bit_pos];
> +
> +		if (unlikely(!de->name_len)) {
> +			bit_pos++;
> +			continue;
> +		}
> +
>  		new_name.name = d.filename[bit_pos];
>  		new_name.len = de->name_len;
> 
> @@ -448,9 +449,6 @@ static int f2fs_add_inline_entries(struct inode *dir,
>  		if (err)
>  			goto punch_dentry_pages;
> 
> -		if (unlikely(!de->name_len))
> -			d.max = -1;
> -
>  		bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
>  	}
>  	return 0;
> -- 
> 2.7.2

Re: [f2fs-dev] [PATCH 1/7] f2fs: avoid needless lock for node pages when fsyncing a file

[Chao Yu]

Hi Jaegeuk,

On 2016/4/26 8:06, Jaegeuk Kim wrote:
> When fsync is called, sync_node_pages finds a proper direct node pages to flush.
> But, it locks unrelated direct node pages together unnecessarily.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

I've reviewed this patchset, it looks good to me, please add:

Acked-by: Chao Yu <yuchao0@huawei.com>