linux-f2fs-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation
@ 2016-04-12 17:42 Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 2/5] f2fs: add BUG_ON to avoid unnecessary flow Jaegeuk Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-04-12 17:42 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: Jaegeuk Kim

Previously, after trylock_page is succeeded, it doesn't check its mapping.
In order to fix that, we can just give PGP_LOCK to pagecache_get_page.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1a33de9..ade221c 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1202,13 +1202,10 @@ static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
 	if (!inode)
 		return;
 
-	page = pagecache_get_page(inode->i_mapping, 0, FGP_NOWAIT, 0);
+	page = pagecache_get_page(inode->i_mapping, 0, FGP_LOCK|FGP_NOWAIT, 0);
 	if (!page)
 		goto iput_out;
 
-	if (!trylock_page(page))
-		goto release_out;
-
 	if (!PageUptodate(page))
 		goto page_out;
 
@@ -1223,9 +1220,7 @@ static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
 	else
 		set_page_dirty(page);
 page_out:
-	unlock_page(page);
-release_out:
-	f2fs_put_page(page, 0);
+	f2fs_put_page(page, 1);
 iput_out:
 	iput(inode);
 }
-- 
2.6.3


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/5] f2fs: add BUG_ON to avoid unnecessary flow
  2016-04-12 17:42 [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation Jaegeuk Kim
@ 2016-04-12 17:42 ` Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 3/5] f2fs: fix dropping inmemory pages in a wrong time Jaegeuk Kim
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-04-12 17:42 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: Jaegeuk Kim

This patch adds BUG_ON instead of retrying loop.
In the case of node pages, we already got this inode page, but unlocked it.
By the fact that we don't truncate any node pages in operations, the page's
mapping should be unchangeable.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index ade221c..095fc2c 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -832,7 +832,7 @@ int truncate_inode_blocks(struct inode *inode, pgoff_t from)
 	trace_f2fs_truncate_inode_blocks_enter(inode, from);
 
 	level = get_node_path(inode, from, offset, noffset);
-restart:
+
 	page = get_node_page(sbi, inode->i_ino);
 	if (IS_ERR(page)) {
 		trace_f2fs_truncate_inode_blocks_exit(inode, PTR_ERR(page));
@@ -896,10 +896,7 @@ skip_partial:
 		if (offset[1] == 0 &&
 				ri->i_nid[offset[0] - NODE_DIR1_BLOCK]) {
 			lock_page(page);
-			if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
-				f2fs_put_page(page, 1);
-				goto restart;
-			}
+			BUG_ON(page->mapping != NODE_MAPPING(sbi));
 			f2fs_wait_on_page_writeback(page, NODE, true);
 			ri->i_nid[offset[0] - NODE_DIR1_BLOCK] = 0;
 			set_page_dirty(page);
-- 
2.6.3


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/5] f2fs: fix dropping inmemory pages in a wrong time
  2016-04-12 17:42 [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 2/5] f2fs: add BUG_ON to avoid unnecessary flow Jaegeuk Kim
@ 2016-04-12 17:42 ` Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 4/5] f2fs: unset atomic/volatile flag in f2fs_release_file Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 5/5] f2fs: remove redundant condition check Jaegeuk Kim
  3 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-04-12 17:42 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: Jaegeuk Kim

When one reader closes its file while the other writer is doing atomic writes,
f2fs_release_file drops atomic data resulting in an empty commit.
This patch fixes this wrong commit problem by checking openess of the file.

 Process0                       Process1
 				open file
 start atomic write
 write data
 read data
				close file
				f2fs_release_file()
				clear atomic data
 commit atomic write

Reported-by: Miao Xie <miaoxie@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 90d1157..e10eb61 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1254,6 +1254,14 @@ out:
 
 static int f2fs_release_file(struct inode *inode, struct file *filp)
 {
+	/*
+	 * f2fs_relase_file is called at every close calls. So we should
+	 * not drop any inmemory pages by close called by other process.
+	 */
+	if (!(filp->f_mode & FMODE_WRITE) ||
+			atomic_read(&inode->i_writecount) != 1)
+		return 0;
+
 	/* some remained atomic pages should discarded */
 	if (f2fs_is_atomic_file(inode))
 		drop_inmem_pages(inode);
-- 
2.6.3


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 4/5] f2fs: unset atomic/volatile flag in f2fs_release_file
  2016-04-12 17:42 [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 2/5] f2fs: add BUG_ON to avoid unnecessary flow Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 3/5] f2fs: fix dropping inmemory pages in a wrong time Jaegeuk Kim
@ 2016-04-12 17:42 ` Jaegeuk Kim
  2016-04-12 17:42 ` [PATCH 5/5] f2fs: remove redundant condition check Jaegeuk Kim
  3 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-04-12 17:42 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: Jaegeuk Kim

The atomic/volatile operation should be done in pair of start and commit
ioctl.
For example, if a killed process remains open-ended atomic operation, we should
drop its flag as well as its atomic data. Otherwise, if sqlite initiates another
operation which doesn't require atomic writes, it will lose every data, since
f2fs still treats with them as atomic writes; nobody will trigger its commit.

Reported-by: Miao Xie <miaoxie@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c    | 5 ++---
 fs/f2fs/segment.c | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e10eb61..ed389f6 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1266,6 +1266,7 @@ static int f2fs_release_file(struct inode *inode, struct file *filp)
 	if (f2fs_is_atomic_file(inode))
 		drop_inmem_pages(inode);
 	if (f2fs_is_volatile_file(inode)) {
+		clear_inode_flag(F2FS_I(inode), FI_VOLATILE_FILE);
 		set_inode_flag(F2FS_I(inode), FI_DROP_CACHE);
 		filemap_fdatawrite(inode->i_mapping);
 		clear_inode_flag(F2FS_I(inode), FI_DROP_CACHE);
@@ -1449,10 +1450,8 @@ static int f2fs_ioc_abort_volatile_write(struct file *filp)
 	if (ret)
 		return ret;
 
-	if (f2fs_is_atomic_file(inode)) {
-		clear_inode_flag(F2FS_I(inode), FI_ATOMIC_FILE);
+	if (f2fs_is_atomic_file(inode))
 		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);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 540669d..299c784 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -239,6 +239,8 @@ void drop_inmem_pages(struct inode *inode)
 {
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 
+	clear_inode_flag(F2FS_I(inode), FI_ATOMIC_FILE);
+
 	mutex_lock(&fi->inmem_lock);
 	__revoke_inmem_pages(inode, &fi->inmem_pages, true, false);
 	mutex_unlock(&fi->inmem_lock);
-- 
2.6.3


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 5/5] f2fs: remove redundant condition check
  2016-04-12 17:42 [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation Jaegeuk Kim
                   ` (2 preceding siblings ...)
  2016-04-12 17:42 ` [PATCH 4/5] f2fs: unset atomic/volatile flag in f2fs_release_file Jaegeuk Kim
@ 2016-04-12 17:42 ` Jaegeuk Kim
  3 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2016-04-12 17:42 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: Jaegeuk Kim

This patch resolves the redundant condition check reported by David.

Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 2 +-
 fs/f2fs/data.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 0955312..b92782f 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -211,7 +211,7 @@ void ra_meta_pages_cond(struct f2fs_sb_info *sbi, pgoff_t index)
 	bool readahead = false;
 
 	page = find_get_page(META_MAPPING(sbi), index);
-	if (!page || (page && !PageUptodate(page)))
+	if (!page || !PageUptodate(page))
 		readahead = true;
 	f2fs_put_page(page, 0);
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5dafb9c..c29bcf4 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1496,7 +1496,7 @@ restart:
 		} else {
 			/* hole case */
 			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
-			if (err || (!err && dn.data_blkaddr == NULL_ADDR)) {
+			if (err || dn.data_blkaddr == NULL_ADDR) {
 				f2fs_put_dnode(&dn);
 				f2fs_lock_op(sbi);
 				locked = true;
-- 
2.6.3


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-04-12 17:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-12 17:42 [PATCH 1/5] f2fs: use PGP_LOCK to check its truncation Jaegeuk Kim
2016-04-12 17:42 ` [PATCH 2/5] f2fs: add BUG_ON to avoid unnecessary flow Jaegeuk Kim
2016-04-12 17:42 ` [PATCH 3/5] f2fs: fix dropping inmemory pages in a wrong time Jaegeuk Kim
2016-04-12 17:42 ` [PATCH 4/5] f2fs: unset atomic/volatile flag in f2fs_release_file Jaegeuk Kim
2016-04-12 17:42 ` [PATCH 5/5] f2fs: remove redundant condition check Jaegeuk Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).