Space leak in f2fs

Space leak in f2fs

[hujianyang]

Hi Jaegeuk,

I found a space leak problem in f2fs. This problem could lead to
ENOSPC error during stress tests, e.g. ltp.

<<<test_output>>>
growfiles(gf15): 11656 growfiles.c/2249: 16920 tlibio.c/739 write(6, buf, 1352) ret:-1, errno=28 No space left on device
gf15        1  TFAIL  :  growfiles.c:132: Test failed

...

And can be reproduced by these steps whether background_gc is on
or not:

1) format a 4GB f2fs partition
2) dd a 3G file,
3) unlink it.

Do these steps again and again. Soon, after one unlink operation,
you can see the space of the 3G file is not free.

Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks   Used Available Use% Mounted on
/dev/sdd3        4193280 301064   3854328   8% /mnt/f2fs
Fs-Server:/mnt/f2fs # dd if=/dev/zero of=./test bs=1M count=3072
3072+0 records in
3072+0 records out
3221225472 bytes (3.2 GB) copied, 3.1892 s, 1.0 GB/s
Fs-Server:/mnt/f2fs # unlink ./test
Fs-Server:/mnt/f2fs # dd if=/dev/zero of=./test bs=1M count=3072
3072+0 records in
3072+0 records out
3221225472 bytes (3.2 GB) copied, 3.44288 s, 936 MB/s
Fs-Server:/mnt/f2fs # unlink ./test
Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks    Used Available Use% Mounted on
/dev/sdd3        4193280 3449888    705504  84% /mnt/f2fs
Fs-Server:/mnt/f2fs # ls
Fs-Server:/mnt/f2fs # ls
Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks    Used Available Use% Mounted on
/dev/sdd3        4193280 3449888    705504  84% /mnt/f2fs
Fs-Server:/mnt/f2fs # dd if=/dev/zero of=./test bs=1M count=3072
dd: writing `./test': No space left on device
689+0 records in
688+0 records out
721719296 bytes (722 MB) copied, 0.618972 s, 1.2 GB/s
Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks    Used Available Use% Mounted on
/dev/sdd3        4193280 4155392         0 100% /mnt/f2fs
Fs-Server:/mnt/f2fs # ls -l test
-rw-r--r-- 1 root root 721719296 May 13 14:52 test


We can reuse the leaking space after a sync call:


Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks    Used Available Use% Mounted on
/dev/sdd3        4193280 4155392         0 100% /mnt/f2fs
Fs-Server:/mnt/f2fs # sync
Fs-Server:/mnt/f2fs # df .
Filesystem     1K-blocks    Used Available Use% Mounted on
/dev/sdd3        4193280 1006568   3148824  25% /mnt/f2fs


I found this may caused by .drop_inode in f2fs. see f2fs_drop_inode()

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 19438f2..7646d2a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -424,15 +424,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)

 static int f2fs_drop_inode(struct inode *inode)
 {
-       /*
-        * This is to avoid a deadlock condition like below.
-        * writeback_single_inode(inode)
-        *  - f2fs_write_data_page
-        *    - f2fs_gc -> iput -> evict
-        *       - inode_wait_for_writeback(inode)
-        */
-       if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
-               return 0;
        return generic_drop_inode(inode);
 }

After removing these code, this problem is fixed. But this function is
introduced by commit 531ad7d58c6476c5856653448b4c7d26427502b4 to fix
a deadlock problem.

I wish you and other developers in this list could help me to fix this
problem in a correct way.

Thanks,
Hu

Re: [f2fs-dev] Space leak in f2fs

[Jaegeuk Kim]

Hi Hu,

Thank you for the report.

When I tried to reproduce this, it seems that this can occur under somewhat
stressful condition. I could't reach out to this problem.

Nevertheless, I think this is a possible scenario, so I wrote a patch for this.
Could you test this patch?

Thanks,

---
 fs/f2fs/super.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 19438f2..4593cd1 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -431,8 +431,26 @@ static int f2fs_drop_inode(struct inode *inode)
 	 *    - f2fs_gc -> iput -> evict
 	 *       - inode_wait_for_writeback(inode)
 	 */
-	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
+	if (!inode_unhashed(inode) && inode->i_state & I_SYNC) {
+		if (!inode->i_nlink && !is_bad_inode(inode)) {
+			spin_unlock(&inode->i_lock);
+
+			i_size_write(inode, 0);
+
+			if (F2FS_HAS_BLOCKS(inode))
+				f2fs_truncate(inode);
+
+			f2fs_lock_op(F2FS_I_SB(inode));
+			remove_inode_page(inode);
+			f2fs_unlock_op(F2FS_I_SB(inode));
+
+			/* avoid any write_inode call */
+			clear_inode_flag(F2FS_I(inode), FI_DIRTY_INODE);
+
+			spin_lock(&inode->i_lock);
+		}
 		return 0;
+	}
 	return generic_drop_inode(inode);
 }
 
-- 
2.1.1

Re: Space leak in f2fs

[Jaegeuk Kim]

Hi Hu,

Found a bug in the previous patch.
Could you check this out?

Thanks,

---
 fs/f2fs/super.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 19438f2..647591b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -431,8 +431,17 @@ static int f2fs_drop_inode(struct inode *inode)
 	 *    - f2fs_gc -> iput -> evict
 	 *       - inode_wait_for_writeback(inode)
 	 */
-	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
+	if (!inode_unhashed(inode) && inode->i_state & I_SYNC) {
+		if (!inode->i_nlink && !is_bad_inode(inode)) {
+			spin_unlock(&inode->i_lock);
+			i_size_write(inode, 0);
+
+			if (F2FS_HAS_BLOCKS(inode))
+				f2fs_truncate(inode);
+			spin_lock(&inode->i_lock);
+		}
 		return 0;
+	}
 	return generic_drop_inode(inode);
 }
 
-- 
2.1.1


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: Space leak in f2fs

[hujianyang]

Hi Jaegeuk,

I've tested this patch. It's OK now. Seems this problem is fixed.

I'd like to push this patch to my local tree and run a formal
stress test next week. Will you push this patch to f2fs-dev branch?

If you have other modification to this fix, please let me know.

Thanks very much!
Hu

On 2015/5/14 8:24, Jaegeuk Kim wrote:
> Hi Hu,
> 
> Found a bug in the previous patch.
> Could you check this out?
> 
> Thanks,
> 
> ---
>  fs/f2fs/super.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 19438f2..647591b 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -431,8 +431,17 @@ static int f2fs_drop_inode(struct inode *inode)
>  	 *    - f2fs_gc -> iput -> evict
>  	 *       - inode_wait_for_writeback(inode)
>  	 */
> -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
> +	if (!inode_unhashed(inode) && inode->i_state & I_SYNC) {
> +		if (!inode->i_nlink && !is_bad_inode(inode)) {
> +			spin_unlock(&inode->i_lock);
> +			i_size_write(inode, 0);
> +
> +			if (F2FS_HAS_BLOCKS(inode))
> +				f2fs_truncate(inode);
> +			spin_lock(&inode->i_lock);
> +		}
>  		return 0;
> +	}
>  	return generic_drop_inode(inode);
>  }
>  
> 



------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: [f2fs-dev] Space leak in f2fs

[Jaegeuk Kim]

I'll push the patch into dev branch right now.
Let me know, if there is any problem.

Thanks,

On Thu, May 14, 2015 at 09:40:25AM +0800, hujianyang wrote:
> Hi Jaegeuk,
> 
> I've tested this patch. It's OK now. Seems this problem is fixed.
> 
> I'd like to push this patch to my local tree and run a formal
> stress test next week. Will you push this patch to f2fs-dev branch?
> 
> If you have other modification to this fix, please let me know.
> 
> Thanks very much!
> Hu
> 
> On 2015/5/14 8:24, Jaegeuk Kim wrote:
> > Hi Hu,
> > 
> > Found a bug in the previous patch.
> > Could you check this out?
> > 
> > Thanks,
> > 
> > ---
> >  fs/f2fs/super.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 19438f2..647591b 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -431,8 +431,17 @@ static int f2fs_drop_inode(struct inode *inode)
> >  	 *    - f2fs_gc -> iput -> evict
> >  	 *       - inode_wait_for_writeback(inode)
> >  	 */
> > -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
> > +	if (!inode_unhashed(inode) && inode->i_state & I_SYNC) {
> > +		if (!inode->i_nlink && !is_bad_inode(inode)) {
> > +			spin_unlock(&inode->i_lock);
> > +			i_size_write(inode, 0);
> > +
> > +			if (F2FS_HAS_BLOCKS(inode))
> > +				f2fs_truncate(inode);
> > +			spin_lock(&inode->i_lock);
> > +		}
> >  		return 0;
> > +	}
> >  	return generic_drop_inode(inode);
> >  }
> >  
> > 

Re: [f2fs-dev] Space leak in f2fs

[Jaegeuk Kim]

Hi Hu,

I've been rethinking about whole this issue differently.
And, now I'm starting to test with the below patch instead of previous one.

Thanks,

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
 fs/f2fs/data.c       |  4 ++++
 fs/f2fs/f2fs.h       |  1 +
 fs/f2fs/super.c      | 15 ---------------
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 7b7a9d8..74875fb 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -378,6 +378,20 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
 	spin_unlock(&im->ino_lock);
 }
 
+static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
+{
+	struct inode_management *im = &sbi->im[type];
+	struct ino_entry *e;
+	bool exist = false;
+
+	spin_lock(&im->ino_lock);
+	e = radix_tree_lookup(&im->ino_root, ino);
+	if (e)
+		exist = true;
+	spin_unlock(&im->ino_lock);
+	return exist;
+}
+
 void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
 {
 	/* add new dirty ino entry into list */
@@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
 	__remove_ino_entry(sbi, ino, ORPHAN_INO);
 }
 
+bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
+}
+
 static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
 {
 	struct inode *inode = f2fs_iget(sbi->sb, ino);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index b0cc2aa..1988f5f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1749,6 +1749,10 @@ write:
 		goto out;
 	}
 
+	/* if orphan inode, we don't need to write its data */
+	if (is_orphan_inode(sbi, inode->i_ino))
+		goto out;
+
 	if (!wbc->for_reclaim)
 		need_balance_fs = true;
 	else if (has_not_enough_free_secs(sbi, 0))
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 8f1f21a..697346a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1726,6 +1726,7 @@ int acquire_orphan_inode(struct f2fs_sb_info *);
 void release_orphan_inode(struct f2fs_sb_info *);
 void add_orphan_inode(struct f2fs_sb_info *, nid_t);
 void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
+bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
 void recover_orphan_inodes(struct f2fs_sb_info *);
 int get_valid_checkpoint(struct f2fs_sb_info *);
 void update_dirty_page(struct inode *, struct page *);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 19438f2..1d0973a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -422,20 +422,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	return &fi->vfs_inode;
 }
 
-static int f2fs_drop_inode(struct inode *inode)
-{
-	/*
-	 * This is to avoid a deadlock condition like below.
-	 * writeback_single_inode(inode)
-	 *  - f2fs_write_data_page
-	 *    - f2fs_gc -> iput -> evict
-	 *       - inode_wait_for_writeback(inode)
-	 */
-	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
-		return 0;
-	return generic_drop_inode(inode);
-}
-
 /*
  * f2fs_dirty_inode() is called from __mark_inode_dirty()
  *
@@ -759,7 +745,6 @@ restore_opts:
 
 static struct super_operations f2fs_sops = {
 	.alloc_inode	= f2fs_alloc_inode,
-	.drop_inode	= f2fs_drop_inode,
 	.destroy_inode	= f2fs_destroy_inode,
 	.write_inode	= f2fs_write_inode,
 	.dirty_inode	= f2fs_dirty_inode,
-- 
2.1.1

RE: [f2fs-dev] Space leak in f2fs

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, May 15, 2015 5:14 AM
> To: hujianyang
> Cc: linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] Space leak in f2fs
> 
> Hi Hu,
> 
> I've been rethinking about whole this issue differently.
> And, now I'm starting to test with the below patch instead of previous one.
> 
> Thanks,
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
>  fs/f2fs/data.c       |  4 ++++
>  fs/f2fs/f2fs.h       |  1 +
>  fs/f2fs/super.c      | 15 ---------------
>  4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 7b7a9d8..74875fb 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -378,6 +378,20 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int
> type)
>  	spin_unlock(&im->ino_lock);
>  }
> 
> +static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> +{
> +	struct inode_management *im = &sbi->im[type];
> +	struct ino_entry *e;
> +	bool exist = false;
> +
> +	spin_lock(&im->ino_lock);
> +	e = radix_tree_lookup(&im->ino_root, ino);
> +	if (e)
> +		exist = true;
> +	spin_unlock(&im->ino_lock);
> +	return exist;
> +}
> +
>  void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
>  {
>  	/* add new dirty ino entry into list */
> @@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
>  	__remove_ino_entry(sbi, ino, ORPHAN_INO);
>  }
> 
> +bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> +{
> +	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
> +}
> +
>  static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
>  {
>  	struct inode *inode = f2fs_iget(sbi->sb, ino);
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index b0cc2aa..1988f5f 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1749,6 +1749,10 @@ write:
>  		goto out;
>  	}
> 
> +	/* if orphan inode, we don't need to write its data */
> +	if (is_orphan_inode(sbi, inode->i_ino))
> +		goto out;

When user create a temp file by invoking open with O_TMPFILE flag,
in ->tmpfile our temp file will be added into orphan list as its
nlink is zero.

If we skip writting out data for this orphan inode, later, even though
we add nlink/directory entry for orphan inode by calling linkat,
our file will contain inconsistent data between in-memory and on-disk.

So how about considering for this case?

BTW, the previous fixing patch looks good to me.

Thanks,

> +
>  	if (!wbc->for_reclaim)
>  		need_balance_fs = true;
>  	else if (has_not_enough_free_secs(sbi, 0))
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 8f1f21a..697346a 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1726,6 +1726,7 @@ int acquire_orphan_inode(struct f2fs_sb_info *);
>  void release_orphan_inode(struct f2fs_sb_info *);
>  void add_orphan_inode(struct f2fs_sb_info *, nid_t);
>  void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
> +bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
>  void recover_orphan_inodes(struct f2fs_sb_info *);
>  int get_valid_checkpoint(struct f2fs_sb_info *);
>  void update_dirty_page(struct inode *, struct page *);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 19438f2..1d0973a 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -422,20 +422,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>  	return &fi->vfs_inode;
>  }
> 
> -static int f2fs_drop_inode(struct inode *inode)
> -{
> -	/*
> -	 * This is to avoid a deadlock condition like below.
> -	 * writeback_single_inode(inode)
> -	 *  - f2fs_write_data_page
> -	 *    - f2fs_gc -> iput -> evict
> -	 *       - inode_wait_for_writeback(inode)
> -	 */
> -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
> -		return 0;
> -	return generic_drop_inode(inode);
> -}
> -
>  /*
>   * f2fs_dirty_inode() is called from __mark_inode_dirty()
>   *
> @@ -759,7 +745,6 @@ restore_opts:
> 
>  static struct super_operations f2fs_sops = {
>  	.alloc_inode	= f2fs_alloc_inode,
> -	.drop_inode	= f2fs_drop_inode,
>  	.destroy_inode	= f2fs_destroy_inode,
>  	.write_inode	= f2fs_write_inode,
>  	.dirty_inode	= f2fs_dirty_inode,
> --
> 2.1.1
> 
> 
> 
> ------------------------------------------------------------------------------
> One dashboard for servers and applications across Physical-Virtual-Cloud
> Widest out-of-the-box monitoring support with 50+ applications
> Performance metrics, stats and reports that give you Actionable Insights
> Deep dive visibility with transaction tracing using APM Insight.
> http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] Space leak in f2fs

[Jaegeuk Kim]

Hi Chao,

On Fri, May 15, 2015 at 04:31:43PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 

[snip]

> > +	/* if orphan inode, we don't need to write its data */
> > +	if (is_orphan_inode(sbi, inode->i_ino))
> > +		goto out;
> 
> When user create a temp file by invoking open with O_TMPFILE flag,
> in ->tmpfile our temp file will be added into orphan list as its
> nlink is zero.
> 
> If we skip writting out data for this orphan inode, later, even though
> we add nlink/directory entry for orphan inode by calling linkat,
> our file will contain inconsistent data between in-memory and on-disk.
> 
> So how about considering for this case?

Right.
How about the below patch?

> 
> BTW, the previous fixing patch looks good to me.

But, my new concern here is a memory pressure. If we do not drop the inode
when iput was called, we need to wait for another time slot to reclaim its
memory.

Thanks,

---
 fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
 fs/f2fs/data.c       |  8 ++++++++
 fs/f2fs/dir.c        |  1 +
 fs/f2fs/f2fs.h       |  2 ++
 fs/f2fs/super.c      | 14 +++++++++++++-
 5 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 7b7a9d8..74875fb 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -378,6 +378,20 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
 	spin_unlock(&im->ino_lock);
 }
 
+static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
+{
+	struct inode_management *im = &sbi->im[type];
+	struct ino_entry *e;
+	bool exist = false;
+
+	spin_lock(&im->ino_lock);
+	e = radix_tree_lookup(&im->ino_root, ino);
+	if (e)
+		exist = true;
+	spin_unlock(&im->ino_lock);
+	return exist;
+}
+
 void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
 {
 	/* add new dirty ino entry into list */
@@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
 	__remove_ino_entry(sbi, ino, ORPHAN_INO);
 }
 
+bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
+{
+	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
+}
+
 static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
 {
 	struct inode *inode = f2fs_iget(sbi->sb, ino);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index b0cc2aa..d883c14 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1749,6 +1749,14 @@ write:
 		goto out;
 	}
 
+	/*
+	 * if orphan inode, we don't need to write its data,
+	 * but, tmpfile is not the case.
+	 */
+	if (is_orphan_inode(sbi, inode->i_ino) &&
+			!is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE))
+		goto out;
+
 	if (!wbc->for_reclaim)
 		need_balance_fs = true;
 	else if (has_not_enough_free_secs(sbi, 0))
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 3e92376..a2ea1b9 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -648,6 +648,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
 	update_inode(inode, page);
 	f2fs_put_page(page, 1);
 
+	set_inode_flag(F2FS_I(inode), FI_TMP_INODE);
 	clear_inode_flag(F2FS_I(inode), FI_NEW_INODE);
 fail:
 	up_write(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cdcae06..de21d38 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1337,6 +1337,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
 /* used for f2fs_inode_info->flags */
 enum {
 	FI_NEW_INODE,		/* indicate newly allocated inode */
+	FI_TMP_INODE,		/* indicate tmpfile */
 	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
 	FI_DIRTY_DIR,		/* indicate directory has dirty pages */
 	FI_INC_LINK,		/* need to increment i_nlink */
@@ -1726,6 +1727,7 @@ int acquire_orphan_inode(struct f2fs_sb_info *);
 void release_orphan_inode(struct f2fs_sb_info *);
 void add_orphan_inode(struct f2fs_sb_info *, nid_t);
 void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
+bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
 void recover_orphan_inodes(struct f2fs_sb_info *);
 int get_valid_checkpoint(struct f2fs_sb_info *);
 void update_dirty_page(struct inode *, struct page *);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7464d08..98af3bf 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -430,9 +430,21 @@ static int f2fs_drop_inode(struct inode *inode)
 	 *  - f2fs_write_data_page
 	 *    - f2fs_gc -> iput -> evict
 	 *       - inode_wait_for_writeback(inode)
+	 * In order to avoid that, f2fs_write_data_page does not write data
+	 * pages for orphan inode except tmpfile.
+	 * Nevertheless, we need to truncate the tmpfile's data to avoid
+	 * needless cleaning.
 	 */
-	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
+	if (is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE) &&
+						inode->i_state & I_SYNC) {
+		spin_unlock(&inode->i_lock);
+		i_size_write(inode, 0);
+
+		if (F2FS_HAS_BLOCKS(inode))
+			f2fs_truncate(inode);
+		spin_lock(&inode->i_lock);
 		return 0;
+	}
 	return generic_drop_inode(inode);
 }
 
-- 
2.1.1

RE: [f2fs-dev] Space leak in f2fs

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Saturday, May 16, 2015 8:56 AM
> To: Chao Yu
> Cc: 'hujianyang'; linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] Space leak in f2fs
> 
> Hi Chao,
> 
> On Fri, May 15, 2015 at 04:31:43PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> 
> [snip]
> 
> > > +	/* if orphan inode, we don't need to write its data */
> > > +	if (is_orphan_inode(sbi, inode->i_ino))
> > > +		goto out;
> >
> > When user create a temp file by invoking open with O_TMPFILE flag,
> > in ->tmpfile our temp file will be added into orphan list as its
> > nlink is zero.
> >
> > If we skip writting out data for this orphan inode, later, even though
> > we add nlink/directory entry for orphan inode by calling linkat,
> > our file will contain inconsistent data between in-memory and on-disk.
> >
> > So how about considering for this case?
> 
> Right.
> How about the below patch?
> 
> >
> > BTW, the previous fixing patch looks good to me.
> 
> But, my new concern here is a memory pressure. If we do not drop the inode
> when iput was called, we need to wait for another time slot to reclaim its
> memory.

Agree. Please see below.

> 
> Thanks,
> 
> ---
>  fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
>  fs/f2fs/data.c       |  8 ++++++++
>  fs/f2fs/dir.c        |  1 +
>  fs/f2fs/f2fs.h       |  2 ++
>  fs/f2fs/super.c      | 14 +++++++++++++-
>  5 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 7b7a9d8..74875fb 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -378,6 +378,20 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int
> type)
>  	spin_unlock(&im->ino_lock);
>  }
> 
> +static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> +{
> +	struct inode_management *im = &sbi->im[type];
> +	struct ino_entry *e;
> +	bool exist = false;
> +
> +	spin_lock(&im->ino_lock);
> +	e = radix_tree_lookup(&im->ino_root, ino);
> +	if (e)
> +		exist = true;
> +	spin_unlock(&im->ino_lock);
> +	return exist;
> +}
> +
>  void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
>  {
>  	/* add new dirty ino entry into list */
> @@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
>  	__remove_ino_entry(sbi, ino, ORPHAN_INO);
>  }
> 
> +bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> +{
> +	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
> +}
> +
>  static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
>  {
>  	struct inode *inode = f2fs_iget(sbi->sb, ino);
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index b0cc2aa..d883c14 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1749,6 +1749,14 @@ write:
>  		goto out;
>  	}
> 
> +	/*
> +	 * if orphan inode, we don't need to write its data,
> +	 * but, tmpfile is not the case.
> +	 */
> +	if (is_orphan_inode(sbi, inode->i_ino) &&
> +			!is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE))

For normal inode, all dirty pages will not be written out, and after that pages
can be reclaimed by VM any time due to they are be cleaned when flush. Then any
process who held the orphan inode may not read any original data correctly from
this inode.

And here is the unlink description in POSIX:
"If one or more processes have the file open when the last link is removed,
the link shall be removed before unlink() returns, but the removal of the
file contents shall be postponed until all references to the file are closed."

To my understanding for above description, we should keep data of helded orphan
inode in memory or on disk until it is not referenced by any processes.

How do you think of it?

using "if (is_orphan_inode(sbi, inode->i_ino) && !atomic_read(&inode->i_count))"
to skip writing at the beginning of ->writepage()?

Thanks,

> +		goto out;
> +
>  	if (!wbc->for_reclaim)
>  		need_balance_fs = true;
>  	else if (has_not_enough_free_secs(sbi, 0))
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 3e92376..a2ea1b9 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -648,6 +648,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
>  	update_inode(inode, page);
>  	f2fs_put_page(page, 1);
> 
> +	set_inode_flag(F2FS_I(inode), FI_TMP_INODE);
>  	clear_inode_flag(F2FS_I(inode), FI_NEW_INODE);
>  fail:
>  	up_write(&F2FS_I(inode)->i_sem);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index cdcae06..de21d38 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1337,6 +1337,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
>  /* used for f2fs_inode_info->flags */
>  enum {
>  	FI_NEW_INODE,		/* indicate newly allocated inode */
> +	FI_TMP_INODE,		/* indicate tmpfile */
>  	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
>  	FI_DIRTY_DIR,		/* indicate directory has dirty pages */
>  	FI_INC_LINK,		/* need to increment i_nlink */
> @@ -1726,6 +1727,7 @@ int acquire_orphan_inode(struct f2fs_sb_info *);
>  void release_orphan_inode(struct f2fs_sb_info *);
>  void add_orphan_inode(struct f2fs_sb_info *, nid_t);
>  void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
> +bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
>  void recover_orphan_inodes(struct f2fs_sb_info *);
>  int get_valid_checkpoint(struct f2fs_sb_info *);
>  void update_dirty_page(struct inode *, struct page *);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 7464d08..98af3bf 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -430,9 +430,21 @@ static int f2fs_drop_inode(struct inode *inode)
>  	 *  - f2fs_write_data_page
>  	 *    - f2fs_gc -> iput -> evict
>  	 *       - inode_wait_for_writeback(inode)
> +	 * In order to avoid that, f2fs_write_data_page does not write data
> +	 * pages for orphan inode except tmpfile.
> +	 * Nevertheless, we need to truncate the tmpfile's data to avoid
> +	 * needless cleaning.
>  	 */
> -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
> +	if (is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE) &&
> +						inode->i_state & I_SYNC) {
> +		spin_unlock(&inode->i_lock);
> +		i_size_write(inode, 0);
> +
> +		if (F2FS_HAS_BLOCKS(inode))
> +			f2fs_truncate(inode);
> +		spin_lock(&inode->i_lock);
>  		return 0;
> +	}
>  	return generic_drop_inode(inode);
>  }
> 
> --
> 2.1.1

Re: Space leak in f2fs

[Nicholas Krause]

On May 17, 2015 10:43:14 PM EDT, Chao Yu <chao2.yu@samsung.com> wrote:
>Hi Jaegeuk,
>
>> -----Original Message-----
>> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>> Sent: Saturday, May 16, 2015 8:56 AM
>> To: Chao Yu
>> Cc: 'hujianyang'; linux-fsdevel@vger.kernel.org;
>linux-f2fs-devel@lists.sourceforge.net
>> Subject: Re: [f2fs-dev] Space leak in f2fs
>> 
>> Hi Chao,
>> 
>> On Fri, May 15, 2015 at 04:31:43PM +0800, Chao Yu wrote:
>> > Hi Jaegeuk,
>> >
>> 
>> [snip]
>> 
>> > > +	/* if orphan inode, we don't need to write its data */
>> > > +	if (is_orphan_inode(sbi, inode->i_ino))
>> > > +		goto out;
>> >
>> > When user create a temp file by invoking open with O_TMPFILE flag,
>> > in ->tmpfile our temp file will be added into orphan list as its
>> > nlink is zero.
>> >
>> > If we skip writting out data for this orphan inode, later, even
>though
>> > we add nlink/directory entry for orphan inode by calling linkat,
>> > our file will contain inconsistent data between in-memory and
>on-disk.
>> >
>> > So how about considering for this case?
>> 
>> Right.
>> How about the below patch?
>> 
>> >
>> > BTW, the previous fixing patch looks good to me.
>> 
>> But, my new concern here is a memory pressure. If we do not drop the
>inode
>> when iput was called, we need to wait for another time slot to
>reclaim its
>> memory.
>
>Agree. Please see below.
>
>> 
>> Thanks,
>> 
>> ---
>>  fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
>>  fs/f2fs/data.c       |  8 ++++++++
>>  fs/f2fs/dir.c        |  1 +
>>  fs/f2fs/f2fs.h       |  2 ++
>>  fs/f2fs/super.c      | 14 +++++++++++++-
>>  5 files changed, 43 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 7b7a9d8..74875fb 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -378,6 +378,20 @@ static void __remove_ino_entry(struct
>f2fs_sb_info *sbi, nid_t ino, int
>> type)
>>  	spin_unlock(&im->ino_lock);
>>  }
>> 
>> +static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>int type)
>> +{
>> +	struct inode_management *im = &sbi->im[type];
>> +	struct ino_entry *e;
>> +	bool exist = false;
>> +
>> +	spin_lock(&im->ino_lock);
>> +	e = radix_tree_lookup(&im->ino_root, ino);
>> +	if (e)
>> +		exist = true;
>> +	spin_unlock(&im->ino_lock);
>> +	return exist;
>> +}
>> +
>>  void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
>>  {
>>  	/* add new dirty ino entry into list */
>> @@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info
>*sbi, nid_t ino)
>>  	__remove_ino_entry(sbi, ino, ORPHAN_INO);
>>  }
>> 
>> +bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
>> +{
>> +	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
>> +}
>> +
>>  static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t
>ino)
>>  {
>>  	struct inode *inode = f2fs_iget(sbi->sb, ino);
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index b0cc2aa..d883c14 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1749,6 +1749,14 @@ write:
>>  		goto out;
>>  	}
>> 
>> +	/*
>> +	 * if orphan inode, we don't need to write its data,
>> +	 * but, tmpfile is not the case.
>> +	 */
>> +	if (is_orphan_inode(sbi, inode->i_ino) &&
>> +			!is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE))
>
>For normal inode, all dirty pages will not be written out, and after
>that pages
>can be reclaimed by VM any time due to they are be cleaned when flush.
>Then any
>process who held the orphan inode may not read any original data
>correctly from
>this inode.
>
>And here is the unlink description in POSIX:
>"If one or more processes have the file open when the last link is
>removed,
>the link shall be removed before unlink() returns, but the removal of
>the
>file contents shall be postponed until all references to the file are
>closed."
>
>To my understanding for above description, we should keep data of
>helded orphan
>inode in memory or on disk until it is not referenced by any processes.
>
>How do you think of it?
>
>using "if (is_orphan_inode(sbi, inode->i_ino) &&
>!atomic_read(&inode->i_count))"
>to skip writing at the beginning of ->writepage()?
>
>Thanks,
>
Chao, 
Your correct here,  I was going to recommend this but my explanation 
was pretty badly worded.  Again also
I am not that well versed in the f2fs 
code base so I wasn't sure if my 
answer was correct. 
Nick
>> +		goto out;
>> +
>>  	if (!wbc->for_reclaim)
>>  		need_balance_fs = true;
>>  	else if (has_not_enough_free_secs(sbi, 0))
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index 3e92376..a2ea1b9 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -648,6 +648,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct
>inode *dir)
>>  	update_inode(inode, page);
>>  	f2fs_put_page(page, 1);
>> 
>> +	set_inode_flag(F2FS_I(inode), FI_TMP_INODE);
>>  	clear_inode_flag(F2FS_I(inode), FI_NEW_INODE);
>>  fail:
>>  	up_write(&F2FS_I(inode)->i_sem);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index cdcae06..de21d38 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -1337,6 +1337,7 @@ static inline void f2fs_change_bit(unsigned int
>nr, char *addr)
>>  /* used for f2fs_inode_info->flags */
>>  enum {
>>  	FI_NEW_INODE,		/* indicate newly allocated inode */
>> +	FI_TMP_INODE,		/* indicate tmpfile */
>>  	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
>>  	FI_DIRTY_DIR,		/* indicate directory has dirty pages */
>>  	FI_INC_LINK,		/* need to increment i_nlink */
>> @@ -1726,6 +1727,7 @@ int acquire_orphan_inode(struct f2fs_sb_info
>*);
>>  void release_orphan_inode(struct f2fs_sb_info *);
>>  void add_orphan_inode(struct f2fs_sb_info *, nid_t);
>>  void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
>> +bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
>>  void recover_orphan_inodes(struct f2fs_sb_info *);
>>  int get_valid_checkpoint(struct f2fs_sb_info *);
>>  void update_dirty_page(struct inode *, struct page *);
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 7464d08..98af3bf 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -430,9 +430,21 @@ static int f2fs_drop_inode(struct inode *inode)
>>  	 *  - f2fs_write_data_page
>>  	 *    - f2fs_gc -> iput -> evict
>>  	 *       - inode_wait_for_writeback(inode)
>> +	 * In order to avoid that, f2fs_write_data_page does not write data
>> +	 * pages for orphan inode except tmpfile.
>> +	 * Nevertheless, we need to truncate the tmpfile's data to avoid
>> +	 * needless cleaning.
>>  	 */
>> -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
>> +	if (is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE) &&
>> +						inode->i_state & I_SYNC) {
>> +		spin_unlock(&inode->i_lock);
>> +		i_size_write(inode, 0);
>> +
>> +		if (F2FS_HAS_BLOCKS(inode))
>> +			f2fs_truncate(inode);
>> +		spin_lock(&inode->i_lock);
>>  		return 0;
>> +	}
>>  	return generic_drop_inode(inode);
>>  }
>> 
>> --
>> 2.1.1
>
>
>
>------------------------------------------------------------------------------
>One dashboard for servers and applications across
>Physical-Virtual-Cloud 
>Widest out-of-the-box monitoring support with 50+ applications
>Performance metrics, stats and reports that give you Actionable
>Insights
>Deep dive visibility with transaction tracing using APM Insight.
>http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
>_______________________________________________
>Linux-f2fs-devel mailing list
>Linux-f2fs-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: [f2fs-dev] Space leak in f2fs

[Jaegeuk Kim]

Hi Chao,

On Mon, May 18, 2015 at 10:43:14AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Saturday, May 16, 2015 8:56 AM
> > To: Chao Yu
> > Cc: 'hujianyang'; linux-fsdevel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] Space leak in f2fs
> > 
> > Hi Chao,
> > 
> > On Fri, May 15, 2015 at 04:31:43PM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > 
> > [snip]
> > 
> > > > +	/* if orphan inode, we don't need to write its data */
> > > > +	if (is_orphan_inode(sbi, inode->i_ino))
> > > > +		goto out;
> > >
> > > When user create a temp file by invoking open with O_TMPFILE flag,
> > > in ->tmpfile our temp file will be added into orphan list as its
> > > nlink is zero.
> > >
> > > If we skip writting out data for this orphan inode, later, even though
> > > we add nlink/directory entry for orphan inode by calling linkat,
> > > our file will contain inconsistent data between in-memory and on-disk.
> > >
> > > So how about considering for this case?
> > 
> > Right.
> > How about the below patch?
> > 
> > >
> > > BTW, the previous fixing patch looks good to me.
> > 
> > But, my new concern here is a memory pressure. If we do not drop the inode
> > when iput was called, we need to wait for another time slot to reclaim its
> > memory.
> 
> Agree. Please see below.
> 
> > 
> > Thanks,
> > 
> > ---
> >  fs/f2fs/checkpoint.c | 19 +++++++++++++++++++
> >  fs/f2fs/data.c       |  8 ++++++++
> >  fs/f2fs/dir.c        |  1 +
> >  fs/f2fs/f2fs.h       |  2 ++
> >  fs/f2fs/super.c      | 14 +++++++++++++-
> >  5 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 7b7a9d8..74875fb 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -378,6 +378,20 @@ static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int
> > type)
> >  	spin_unlock(&im->ino_lock);
> >  }
> > 
> > +static bool __exist_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
> > +{
> > +	struct inode_management *im = &sbi->im[type];
> > +	struct ino_entry *e;
> > +	bool exist = false;
> > +
> > +	spin_lock(&im->ino_lock);
> > +	e = radix_tree_lookup(&im->ino_root, ino);
> > +	if (e)
> > +		exist = true;
> > +	spin_unlock(&im->ino_lock);
> > +	return exist;
> > +}
> > +
> >  void add_dirty_inode(struct f2fs_sb_info *sbi, nid_t ino, int type)
> >  {
> >  	/* add new dirty ino entry into list */
> > @@ -458,6 +472,11 @@ void remove_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> >  	__remove_ino_entry(sbi, ino, ORPHAN_INO);
> >  }
> > 
> > +bool is_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> > +{
> > +	return __exist_ino_entry(sbi, ino, ORPHAN_INO);
> > +}
> > +
> >  static void recover_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino)
> >  {
> >  	struct inode *inode = f2fs_iget(sbi->sb, ino);
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index b0cc2aa..d883c14 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1749,6 +1749,14 @@ write:
> >  		goto out;
> >  	}
> > 
> > +	/*
> > +	 * if orphan inode, we don't need to write its data,
> > +	 * but, tmpfile is not the case.
> > +	 */
> > +	if (is_orphan_inode(sbi, inode->i_ino) &&
> > +			!is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE))
> 
> For normal inode, all dirty pages will not be written out, and after that pages
> can be reclaimed by VM any time due to they are be cleaned when flush. Then any
> process who held the orphan inode may not read any original data correctly from
> this inode.

Urg, right.
Indeed, I have not to do this for orphan inodes.

> 
> And here is the unlink description in POSIX:
> "If one or more processes have the file open when the last link is removed,
> the link shall be removed before unlink() returns, but the removal of the
> file contents shall be postponed until all references to the file are closed."
> 
> To my understanding for above description, we should keep data of helded orphan
> inode in memory or on disk until it is not referenced by any processes.
> 
> How do you think of it?
> 
> using "if (is_orphan_inode(sbi, inode->i_ino) && !atomic_read(&inode->i_count))"
> to skip writing at the beginning of ->writepage()?

Hmm, IMO, we can't use i_count without i_lock. And this doesn't clearly address
the original race condition.

For now, simply we'd better keep v2 which only truncates data blocks in
f2fs_drop_inode.

Thank you for pointing this out.

Thanks,

> 
> Thanks,
> 
> > +		goto out;
> > +
> >  	if (!wbc->for_reclaim)
> >  		need_balance_fs = true;
> >  	else if (has_not_enough_free_secs(sbi, 0))
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index 3e92376..a2ea1b9 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -648,6 +648,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir)
> >  	update_inode(inode, page);
> >  	f2fs_put_page(page, 1);
> > 
> > +	set_inode_flag(F2FS_I(inode), FI_TMP_INODE);
> >  	clear_inode_flag(F2FS_I(inode), FI_NEW_INODE);
> >  fail:
> >  	up_write(&F2FS_I(inode)->i_sem);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index cdcae06..de21d38 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1337,6 +1337,7 @@ static inline void f2fs_change_bit(unsigned int nr, char *addr)
> >  /* used for f2fs_inode_info->flags */
> >  enum {
> >  	FI_NEW_INODE,		/* indicate newly allocated inode */
> > +	FI_TMP_INODE,		/* indicate tmpfile */
> >  	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
> >  	FI_DIRTY_DIR,		/* indicate directory has dirty pages */
> >  	FI_INC_LINK,		/* need to increment i_nlink */
> > @@ -1726,6 +1727,7 @@ int acquire_orphan_inode(struct f2fs_sb_info *);
> >  void release_orphan_inode(struct f2fs_sb_info *);
> >  void add_orphan_inode(struct f2fs_sb_info *, nid_t);
> >  void remove_orphan_inode(struct f2fs_sb_info *, nid_t);
> > +bool is_orphan_inode(struct f2fs_sb_info *, nid_t);
> >  void recover_orphan_inodes(struct f2fs_sb_info *);
> >  int get_valid_checkpoint(struct f2fs_sb_info *);
> >  void update_dirty_page(struct inode *, struct page *);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 7464d08..98af3bf 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -430,9 +430,21 @@ static int f2fs_drop_inode(struct inode *inode)
> >  	 *  - f2fs_write_data_page
> >  	 *    - f2fs_gc -> iput -> evict
> >  	 *       - inode_wait_for_writeback(inode)
> > +	 * In order to avoid that, f2fs_write_data_page does not write data
> > +	 * pages for orphan inode except tmpfile.
> > +	 * Nevertheless, we need to truncate the tmpfile's data to avoid
> > +	 * needless cleaning.
> >  	 */
> > -	if (!inode_unhashed(inode) && inode->i_state & I_SYNC)
> > +	if (is_inode_flag_set(F2FS_I(inode), FI_TMP_INODE) &&
> > +						inode->i_state & I_SYNC) {
> > +		spin_unlock(&inode->i_lock);
> > +		i_size_write(inode, 0);
> > +
> > +		if (F2FS_HAS_BLOCKS(inode))
> > +			f2fs_truncate(inode);
> > +		spin_lock(&inode->i_lock);
> >  		return 0;
> > +	}
> >  	return generic_drop_inode(inode);
> >  }
> > 
> > --
> > 2.1.1