f2fs: fix to truncate inline data past EOF

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +8 -5

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64 from)
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +16 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before

fs/f2fs/inline.c +10 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;

fs/f2fs/inline.c +6 -3

+out:
+	f2fs_put_page(page, 1);
+	return err;
 }
 
 int f2fs_read_inline_data(struct inode *inode, struct page *page)

@@ -131,7 +178,7 @@ no_update:
 	set_inode_flag(F2FS_I(dn->inode), FI_APPEND_WRITE);
 
 	/* clear inline data and flag after data writeback */
-	truncate_inline_data(dn->inode_page);
+	truncate_inline_inode(dn->inode_page, 0);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -245,7 +292,7 @@ process_inline:
 	if (f2fs_has_inline_data(inode)) {
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(sbi, IS_ERR(ipage));
-		truncate_inline_data(ipage);
+		truncate_inline_inode(ipage, 0);
 		f2fs_clear_inline_inode(inode);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);

@@ -363,7 +410,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 	set_page_dirty(page);
 
 	/* clear inline dir and flag after data writeback */
-	truncate_inline_data(ipage);
+	truncate_inline_inode(ipage, 0);
 
 	stat_dec_inline_dir(dir);
 	clear_inode_flag(F2FS_I(dir), FI_INLINE_DENTRY);

Signature · 1 line

2.3.1

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, March 06, 2015 5:34 AM
To: Chao Yu
Cc: Changman Lee; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 111 lines · Chao Yu

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +8 -5

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64 from)
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +20 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before
+	 * truncating real data if new size is past max inline data size.
+	 */
+	f2fs_bug_on(sbi, from > MAX_INLINE_DATA);
+

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 80

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+	if (from == MAX_INLINE_DATA)
+		return 0;
+
+	page = grab_cache_page(inode->i_mapping, 0);
+	if (!page)
+		return -ENOMEM;

Do we need to grab and read #0 page?

How about checking its cached page after on-disk truncation?

1. page = get_node_page()
2. check_inline
3. truncate_inline_inode
4. f2fs_put_page(ipage)

5. truncate_partial_data_page(inode, from, force)

	if (!offset && !force)

return 0;

Quoted 14 lines

fs/f2fs/inline.c +14 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;
+	}
+
+	truncate_inline_inode(ipage, from);
+	set_page_dirty(ipage);

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 100

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+
+	read_inline_data(page, ipage);
+	f2fs_put_page(ipage, 1);
+
+	addr = inline_data_addr(page);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);

It needs to do likewise truncate_partial_data_page.

From 2e993eee4588cbf2c318eab9916a3aa548ce21f9 Mon Sep 17 00:00:00 2001

Hi Jaegeuk,

Quoted 121 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, March 06, 2015 5:34 AM
To: Chao Yu
Cc: Changman Lee; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 111 lines · Chao Yu

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +8 -5

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64 from)
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +20 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before
+	 * truncating real data if new size is past max inline data size.
+	 */
+	f2fs_bug_on(sbi, from > MAX_INLINE_DATA);
+

Quoted 8 lines

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 80

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+	if (from == MAX_INLINE_DATA)
+		return 0;
+
+	page = grab_cache_page(inode->i_mapping, 0);
+	if (!page)
+		return -ENOMEM;

Do we need to grab and read #0 page?

No need to read #0 page, sorry for the mistake.

Quoted 25 lines

How about checking its cached page after on-disk truncation?

1. page = get_node_page()
2. check_inline
3. truncate_inline_inode
4. f2fs_put_page(ipage)

5. truncate_partial_data_page(inode, from, force)

	if (!offset && !force)

return 0;

Quoted 14 lines

fs/f2fs/inline.c +14 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;
+	}
+
+	truncate_inline_inode(ipage, from);
+	set_page_dirty(ipage);

Quoted 8 lines

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 100

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+
+	read_inline_data(page, ipage);
+	f2fs_put_page(ipage, 1);
+
+	addr = inline_data_addr(page);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);

It needs to do likewise truncate_partial_data_page.

Do you mean like this?

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Monday, March 09, 2015 11:49 AM
To: Chao Yu
Cc: 'Changman Lee'; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 168 lines · Chao Yu

Hi Jaegeuk,

Quoted 121 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, March 06, 2015 5:34 AM
To: Chao Yu
Cc: Changman Lee; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 111 lines · Chao Yu

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +8 -5

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64 from)
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +20 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before
+	 * truncating real data if new size is past max inline data size.
+	 */
+	f2fs_bug_on(sbi, from > MAX_INLINE_DATA);
+

Quoted 8 lines

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 80

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+	if (from == MAX_INLINE_DATA)
+		return 0;
+
+	page = grab_cache_page(inode->i_mapping, 0);
+	if (!page)
+		return -ENOMEM;

Do we need to grab and read #0 page?

No need to read #0 page, sorry for the mistake.

Quoted 33 lines

How about checking its cached page after on-disk truncation?

1. page = get_node_page()
2. check_inline
3. truncate_inline_inode
4. f2fs_put_page(ipage)

5. truncate_partial_data_page(inode, from, force)

	if (!offset && !force)

return 0;

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 100

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+	}
+
+	truncate_inline_inode(ipage, from);
+	set_page_dirty(ipage);
+
+	read_inline_data(page, ipage);
+	f2fs_put_page(ipage, 1);
+
+	addr = inline_data_addr(page);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);

It needs to do likewise truncate_partial_data_page.

Quoted 1 line

Do you mean like this?

I meant this.

Anyway, I also wrote a patch about your testing scenario wrt inline_data. So, let me submit the patch sooner or later to xfstests mailing list.

Thanks,

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 10 +++++++---
 fs/f2fs/inline.c | 12 +++++++-----
 3 files changed, 15 insertions(+), 8 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 31b440e..b222b3a 100644
@@ -1760,6 +1760,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+void truncate_inline_inode(struct page *, u64);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +0 -0

index f1341c7..ab249dd 100644
@@ -456,12 +456,12 @@ void truncate_data_blocks(struct dnode_of_data *dn)
 	truncate_data_blocks_range(dn, ADDRS_PER_BLOCK);

fs/f2fs/file.c +1 -0

 		return 0;
 
 	page = find_data_page(inode, from >> PAGE_CACHE_SHIFT, false);

@@ -489,6 +489,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	pgoff_t free_from;
 	int count = 0, err = 0;
 	struct page *ipage;
+	bool truncate_page = false;
 
 	trace_f2fs_truncate_blocks_enter(inode, from);
 

fs/f2fs/file.c +2 -1

+		truncate_page = true;
 		f2fs_put_page(ipage, 1);
 		goto out;
 	}

@@ -535,7 +539,7 @@ out:
 
 	/* lastly zero out the first data page */
 	if (!err)
-		err = truncate_partial_data_page(inode, from);
+		err = truncate_partial_data_page(inode, from, truncate_page);
 
 	trace_f2fs_truncate_blocks_exit(inode, err);
 	return err;

fs/f2fs/inline.c +7 -6

index ee0984c..b236e30 100644
@@ -50,10 +50,12 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
 }
 
 int f2fs_read_inline_data(struct inode *inode, struct page *page)

@@ -131,7 +133,7 @@ no_update:
 	set_inode_flag(F2FS_I(dn->inode), FI_APPEND_WRITE);
 
 	/* clear inline data and flag after data writeback */
-	truncate_inline_data(dn->inode_page);
+	truncate_inline_inode(dn->inode_page, 0);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -245,7 +247,7 @@ process_inline:
 	if (f2fs_has_inline_data(inode)) {
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(sbi, IS_ERR(ipage));
-		truncate_inline_data(ipage);
+		truncate_inline_inode(ipage, 0);
 		f2fs_clear_inline_inode(inode);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);

@@ -363,7 +365,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 	set_page_dirty(page);
 
 	/* clear inline dir and flag after data writeback */
-	truncate_inline_data(ipage);
+	truncate_inline_inode(ipage, 0);
 
 	stat_dec_inline_dir(dir);
 	clear_inode_flag(F2FS_I(dir), FI_INLINE_DENTRY);
--

2.1.1

Hi Jaegeuk,

Quoted 179 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Monday, March 09, 2015 11:49 AM
To: Chao Yu
Cc: 'Changman Lee'; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 168 lines · Chao Yu

Hi Jaegeuk,

Quoted 121 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, March 06, 2015 5:34 AM
To: Chao Yu
Cc: Changman Lee; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 111 lines · Chao Yu

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +8 -5

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64 from)
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +20 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before
+	 * truncating real data if new size is past max inline data size.
+	 */
+	f2fs_bug_on(sbi, from > MAX_INLINE_DATA);
+

Quoted 8 lines

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 80

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+	if (from == MAX_INLINE_DATA)
+		return 0;
+
+	page = grab_cache_page(inode->i_mapping, 0);
+	if (!page)
+		return -ENOMEM;

Do we need to grab and read #0 page?

No need to read #0 page, sorry for the mistake.

Quoted 33 lines

How about checking its cached page after on-disk truncation?

1. page = get_node_page()
2. check_inline
3. truncate_inline_inode
4. f2fs_put_page(ipage)

5. truncate_partial_data_page(inode, from, force)

	if (!offset && !force)

return 0;

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 100

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+	}
+
+	truncate_inline_inode(ipage, from);
+	set_page_dirty(ipage);
+
+	read_inline_data(page, ipage);
+	f2fs_put_page(ipage, 1);
+
+	addr = inline_data_addr(page);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);

It needs to do likewise truncate_partial_data_page.

Quoted 6 lines

Quoted 1 line

Do you mean like this?

I meant this.

Anyway, I also wrote a patch about your testing scenario wrt inline_data. So, let me submit the patch sooner or later to xfstests mailing list.

Sounds good! :)

Quoted 27 lines

Thanks,

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 10 +++++++---
 fs/f2fs/inline.c | 12 +++++++-----
 3 files changed, 15 insertions(+), 8 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 31b440e..b222b3a 100644
@@ -1760,6 +1760,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+void truncate_inline_inode(struct page *, u64);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +0 -0

index f1341c7..ab249dd 100644
@@ -456,12 +456,12 @@ void truncate_data_blocks(struct dnode_of_data *dn)
 	truncate_data_blocks_range(dn, ADDRS_PER_BLOCK);

fs/f2fs/file.c void truncate_data_blocks(struct dnode_of_data *dn) · line 464

Quoted 10 lines

fs/f2fs/file.c +2 -2

 }
 
-static int truncate_partial_data_page(struct inode *inode, u64 from)
+static int truncate_partial_data_page(struct inode *inode, u64 from, bool force)
 {
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
-	if (!offset)
+	if (!offset && !force)

We truncate on-disk inode page and #0 page separately, so IMO there is a very small chance the following issue will occur.

(0 < truncated_size < MAX_INLINE_SIZE)
->f2fs_setattr
->truncate_setsize #0 page has been truncated partially
->f2fs_truncate
->truncate_blocks
invalidate #0 page by reclaimer
update #0 page with original data in inode page by reader

->truncate_inline_inode
->truncate_partial_data_page
we will fail to truncate #0 page because we can't find valid blkaddr for #0 page in find_data_page(,,false)

How about using find_data_page(,,true) to check weather this page is update or not for this condition.

Quoted 15 lines

fs/f2fs/file.c +1 -0

 		return 0;
 
 	page = find_data_page(inode, from >> PAGE_CACHE_SHIFT, false);

@@ -489,6 +489,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	pgoff_t free_from;
 	int count = 0, err = 0;
 	struct page *ipage;
+	bool truncate_page = false;
 
 	trace_f2fs_truncate_blocks_enter(inode, from);
 

@@ -504,6 +505,9 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 

fs/f2fs/file.c int truncate_blocks(struct inode *inode, u64 from, bool lock) · line 509

Quoted 3 lines

fs/f2fs/file.c +2 -0

 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
+		set_page_dirty(ipage);

If @from is the same as MAX_INLINE_DATA, we will change nothing in inode page, How about skipping set_page_dirty for inode page in this condition?

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Tuesday, March 10, 2015 12:16 AM
To: Chao Yu
Cc: 'Changman Lee'; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 67 lines · Chao Yu

Hi Jaegeuk,

Quoted 65 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Monday, March 09, 2015 11:49 AM
To: Chao Yu
Cc: 'Changman Lee'; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 54 lines · Chao Yu

Hi Jaegeuk,

Quoted 52 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, March 06, 2015 5:34 AM
To: Chao Yu
Cc: Changman Lee; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 42 lines · Chao Yu

Previously if inode is with inline data, we will try to invalid partial inline data in page #0 when we truncate size of inode in truncate_partial_data_page(). And then we set page #0 to dirty, after this we can synchronize inode page with page #0 at ->writepage().

But sometimes we will fail to operate page #0 in truncate_partial_data_page() due to below reason:
a) if offset is zero, we will skip setting page #0 to dirty.
b) if page #0 is not uptodate, we will fail to update it as it has no mapping data.

So with following operations, we will meet recent data which should be truncated.

1.write inline data to file
2.sync first data page to inode page
3.truncate file size to 0
4.truncate file size to max_inline_size
5.echo 1 > /proc/sys/vm/drop_caches
6.read file --> meet original inline data which is remained in inode page.

This patch renames truncate_inline_data() to truncate_inline_inode() for code readability, then intruduces new truncate_inline_data() and use it to truncate inline data in page #0 and inode page in truncate_partial_data_page() for fixing.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 13 ++++++++-----
 fs/f2fs/inline.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 61 insertions(+), 10 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..55dbbe7 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+int truncate_inline_data(struct inode *, u64 from);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

fs/f2fs/file.c static int truncate_partial_data_page(struct inode *inode, u64

Quoted 8 lines

fs/f2fs/f2fs.h +0 -0

 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +0 -0

index f1341c7..6c4443e 100644
@@ -461,6 +461,9 @@ static int truncate_partial_data_page(struct inode *inode, u64

from)

Quoted 156 lines

Quoted 106 lines

Quoted 106 lines

Quoted 61 lines

Quoted 61 lines

fs/f2fs/file.c +8 -5

 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
+	if (f2fs_has_inline_data(inode))
+		return 0;
+
 	if (!offset)
 		return 0;
 

@@ -497,14 +500,14 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (lock)
 		f2fs_lock_op(sbi);
 
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
+	if (f2fs_has_inline_data(inode)) {
+		err = truncate_inline_data(inode, from);
 		goto out;
 	}
 
-	if (f2fs_has_inline_data(inode)) {
-		f2fs_put_page(ipage, 1);
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
 		goto out;
 	}
 

fs/f2fs/inline.c +20 -2

index 4ba9732..a8189c92 100644
@@ -50,10 +50,57 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+static void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+}
+
+int truncate_inline_data(struct inode *inode, u64 from)
+{
+	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
+	struct page *page, *ipage;
+	void *addr;
+	int err = 0;
+
+	/*
+	 * we should never truncate inline data past max inline data size,
+	 * because we always convert inline inode to normal one before
+	 * truncating real data if new size is past max inline data size.
+	 */
+	f2fs_bug_on(sbi, from > MAX_INLINE_DATA);
+

Quoted 8 lines

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 80

Quoted 6 lines

fs/f2fs/inline.c +6 -0

+	if (from == MAX_INLINE_DATA)
+		return 0;
+
+	page = grab_cache_page(inode->i_mapping, 0);
+	if (!page)
+		return -ENOMEM;

Do we need to grab and read #0 page?

No need to read #0 page, sorry for the mistake.

Quoted 33 lines

How about checking its cached page after on-disk truncation?

1. page = get_node_page()
2. check_inline
3. truncate_inline_inode
4. f2fs_put_page(ipage)

5. truncate_partial_data_page(inode, from, force)

	if (!offset && !force)

return 0;

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto out;
+	}
+
+	if (!f2fs_has_inline_data(inode)) {
+		f2fs_put_page(ipage, 1);
+		goto out;

fs/f2fs/inline.c void read_inline_data(struct page *page, struct page *ipage) · line 100

Quoted 10 lines

fs/f2fs/inline.c +10 -0

+	}
+
+	truncate_inline_inode(ipage, from);
+	set_page_dirty(ipage);
+
+	read_inline_data(page, ipage);
+	f2fs_put_page(ipage, 1);
+
+	addr = inline_data_addr(page);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);

It needs to do likewise truncate_partial_data_page.

Quoted 6 lines

Quoted 1 line

Do you mean like this?

I meant this.

Anyway, I also wrote a patch about your testing scenario wrt inline_data. So, let me submit the patch sooner or later to xfstests mailing list.

Sounds good! :)

Quoted 27 lines

Thanks,

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   | 10 +++++++---
 fs/f2fs/inline.c | 12 +++++++-----
 3 files changed, 15 insertions(+), 8 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 31b440e..b222b3a 100644
@@ -1760,6 +1760,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+void truncate_inline_inode(struct page *, u64);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +0 -0

index f1341c7..ab249dd 100644
@@ -456,12 +456,12 @@ void truncate_data_blocks(struct dnode_of_data *dn)
 	truncate_data_blocks_range(dn, ADDRS_PER_BLOCK);

fs/f2fs/file.c void truncate_data_blocks(struct dnode_of_data *dn) · line 464

Quoted 10 lines

fs/f2fs/file.c +2 -2

 }
 
-static int truncate_partial_data_page(struct inode *inode, u64 from)
+static int truncate_partial_data_page(struct inode *inode, u64 from, bool force)
 {
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
-	if (!offset)
+	if (!offset && !force)

We truncate on-disk inode page and #0 page separately, so IMO there is a very small chance the following issue will occur.

Quoted 7 lines

(0 < truncated_size < MAX_INLINE_SIZE)
->f2fs_setattr
->truncate_setsize #0 page has been truncated partially
->f2fs_truncate
->truncate_blocks
invalidate #0 page by reclaimer
update #0 page with original data in inode page by reader

truncate_setsize called truncate_pagesize. so #0 page was truncated successfully.

Quoted 8 lines

->truncate_inline_inode
->truncate_partial_data_page
we will fail to truncate #0 page because we can't find valid blkaddr for #0 page in find_data_page(,,false)

How about using find_data_page(,,true) to check weather this page is update or not for this condition.

Oh, I realized that we don't need to call truncate_partial_data_page, since the cached #0 page was truncated already. We don't care about that.

So, do we need to add just truncate_inline_inode() like below?

Quoted 15 lines

Quoted 15 lines

fs/f2fs/file.c +1 -0

 		return 0;
 
 	page = find_data_page(inode, from >> PAGE_CACHE_SHIFT, false);

@@ -489,6 +489,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	pgoff_t free_from;
 	int count = 0, err = 0;
 	struct page *ipage;
+	bool truncate_page = false;
 
 	trace_f2fs_truncate_blocks_enter(inode, from);
 

@@ -504,6 +505,9 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 

Quoted 6 lines

fs/f2fs/file.c int truncate_blocks(struct inode *inode, u64 from, bool lock)

Quoted 3 lines

fs/f2fs/file.c +2 -0

 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
+		set_page_dirty(ipage);

If @from is the same as MAX_INLINE_DATA, we will change nothing in inode page, How about skipping set_page_dirty for inode page in this condition?

Agreed.
How about adding this in truncate_inline_inode?

	if (from >= MAX_INLINE_DATA)
		return;
	...
	set_page_dirty(ipage);

Thanks,

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   |  1 +
 fs/f2fs/inline.c | 16 +++++++++++-----
 3 files changed, 13 insertions(+), 5 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..5cd3a2f 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+void truncate_inline_inode(struct page *, u64);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +1 -0

index f1341c7..b6b6a04 100644
@@ -504,6 +504,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 
 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
 		f2fs_put_page(ipage, 1);
 		goto out;
 	}

fs/f2fs/inline.c +11 -6

index 4ba9732..40bbf96 100644
@@ -50,10 +50,16 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
+	if (from >= MAX_INLIE_DATA)
+		return;
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+	set_page_dirty(ipage);
 }
 
 int f2fs_read_inline_data(struct inode *inode, struct page *page)

@@ -131,7 +137,7 @@ no_update:
 	set_inode_flag(F2FS_I(dn->inode), FI_APPEND_WRITE);
 
 	/* clear inline data and flag after data writeback */
-	truncate_inline_data(dn->inode_page);
+	truncate_inline_inode(dn->inode_page, 0);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -245,7 +251,7 @@ process_inline:
 	if (f2fs_has_inline_data(inode)) {
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(sbi, IS_ERR(ipage));
-		truncate_inline_data(ipage);
+		truncate_inline_inode(ipage, 0);
 		f2fs_clear_inline_inode(inode);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);

@@ -363,7 +369,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 	set_page_dirty(page);
 
 	/* clear inline dir and flag after data writeback */
-	truncate_inline_data(ipage);
+	truncate_inline_inode(ipage, 0);
 
 	stat_dec_inline_dir(dir);
 	clear_inode_flag(F2FS_I(dir), FI_INLINE_DENTRY);
--

2.1.1

Hi Jaegeuk,

Quoted 19 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +2 -1

+static int truncate_partial_data_page(struct inode *inode, u64 from, bool force)
 {
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
-	if (!offset)
+	if (!offset && !force)

We truncate on-disk inode page and #0 page separately, so IMO there is a very small chance the following issue will occur.

Quoted 7 lines

(0 < truncated_size < MAX_INLINE_SIZE)
->f2fs_setattr
->truncate_setsize #0 page has been truncated partially
->f2fs_truncate
->truncate_blocks
invalidate #0 page by reclaimer
update #0 page with original data in inode page by reader

Quoted 2 lines

truncate_setsize called truncate_pagesize. so #0 page was truncated successfully.

Yeah, but it's partially truncating as 0 < truncated_size < MAX_INLINE_SIZE, After truncating, we still keep [0, truncated_size) valid data in #0 page, and our #0 page status is uptodate | !dirty.

So what I mean is that mm can reclaim this #0 page, then if someone else read the #0 page again, we will update the #0 page with original data in inode page since inode page haven't truncated yet.

If this happened, if we don't truncate #0 page in following code of truncate_blocks, we will remain the original data for user, it's wrong.

IMO, it's better to truncate inode page and #0 page together, or truncate #0 page in truncate_partial_data_page.

How do you think?

Quoted 11 lines

Quoted 8 lines

->truncate_inline_inode
->truncate_partial_data_page
we will fail to truncate #0 page because we can't find valid blkaddr for #0 page in find_data_page(,,false)

How about using find_data_page(,,true) to check weather this page is update or not for this condition.

Oh, I realized that we don't need to call truncate_partial_data_page, since the cached #0 page was truncated already. We don't care about that.

IMO, we should care about this #0 page if above example can happen.

Quoted 24 lines

So, do we need to add just truncate_inline_inode() like below?

Quoted 15 lines

Quoted 15 lines

(quoted diff) +1 -0

 		return 0;
 
 	page = find_data_page(inode, from >> PAGE_CACHE_SHIFT, false);

@@ -489,6 +489,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	pgoff_t free_from;
 	int count = 0, err = 0;
 	struct page *ipage;
+	bool truncate_page = false;
 
 	trace_f2fs_truncate_blocks_enter(inode, from);
 

@@ -504,6 +505,9 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 

Quoted 6 lines

fs/f2fs/file.c int truncate_blocks(struct inode *inode, u64 from, bool lock)

Quoted 3 lines

fs/f2fs/file.c +2 -0

 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
+		set_page_dirty(ipage);

If @from is the same as MAX_INLINE_DATA, we will change nothing in inode page, How about skipping set_page_dirty for inode page in this condition?

Quoted 7 lines

Agreed.
How about adding this in truncate_inline_inode?

	if (from >= MAX_INLINE_DATA)
		return;
	...
	set_page_dirty(ipage);

Yeah, that's good.

And What I suggest is:

bool truncate_inline_inode(struct page *ipage, u64 from)

{
	/*
	 * we should never truncate inline data past max inline data size,
	 * because we always convert inline inode to normal one before
	 * truncating real data if new size is past max inline data size.
	 */
	f2fs_bug_on(F2FS_P_SB(ipage), from > MAX_INLINE_DATA);

	if (from == MAX_INLINE_DATA)

		return false;
	...
	return true;
}

in truncate_blocks()

 	if (f2fs_has_inline_data(inode)) {
		if (truncate_inline_inode(ipage, from))
			set_page_dirty(ipage);

So by this way, we can distinguish between a bug and invalid truncating in truncate_inline_inode, and also we can avoid unneeded set_dirty_page for inode page in other call path.

How do you think of this?

Quoted 86 lines

Thanks,

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h   |  1 +
 fs/f2fs/file.c   |  1 +
 fs/f2fs/inline.c | 16 +++++++++++-----
 3 files changed, 13 insertions(+), 5 deletions(-)

fs/f2fs/f2fs.h +1 -0

index 511d6cd..5cd3a2f 100644
@@ -1754,6 +1754,7 @@ extern struct kmem_cache *inode_entry_slab;
  */
 bool f2fs_may_inline(struct inode *);
 void read_inline_data(struct page *, struct page *);
+void truncate_inline_inode(struct page *, u64);
 int f2fs_read_inline_data(struct inode *, struct page *);
 int f2fs_convert_inline_page(struct dnode_of_data *, struct page *);
 int f2fs_convert_inline_inode(struct inode *);

fs/f2fs/file.c +1 -0

index f1341c7..b6b6a04 100644
@@ -504,6 +504,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 
 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
 		f2fs_put_page(ipage, 1);
 		goto out;
 	}

fs/f2fs/inline.c +11 -6

index 4ba9732..40bbf96 100644
@@ -50,10 +50,16 @@ void read_inline_data(struct page *page, struct page *ipage)
 	SetPageUptodate(page);
 }
 
-static void truncate_inline_data(struct page *ipage)
+void truncate_inline_inode(struct page *ipage, u64 from)
 {
+	void *addr = inline_data_addr(ipage);
+
+	if (from >= MAX_INLIE_DATA)
+		return;
+
 	f2fs_wait_on_page_writeback(ipage, NODE);
-	memset(inline_data_addr(ipage), 0, MAX_INLINE_DATA);
+	memset(addr + from, 0, MAX_INLINE_DATA - from);
+	set_page_dirty(ipage);
 }
 
 int f2fs_read_inline_data(struct inode *inode, struct page *page)

@@ -131,7 +137,7 @@ no_update:
 	set_inode_flag(F2FS_I(dn->inode), FI_APPEND_WRITE);
 
 	/* clear inline data and flag after data writeback */
-	truncate_inline_data(dn->inode_page);
+	truncate_inline_inode(dn->inode_page, 0);
 clear_out:
 	stat_dec_inline_inode(dn->inode);
 	f2fs_clear_inline_inode(dn->inode);

@@ -245,7 +251,7 @@ process_inline:
 	if (f2fs_has_inline_data(inode)) {
 		ipage = get_node_page(sbi, inode->i_ino);
 		f2fs_bug_on(sbi, IS_ERR(ipage));
-		truncate_inline_data(ipage);
+		truncate_inline_inode(ipage, 0);
 		f2fs_clear_inline_inode(inode);
 		update_inode(inode, ipage);
 		f2fs_put_page(ipage, 1);

@@ -363,7 +369,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 	set_page_dirty(page);
 
 	/* clear inline dir and flag after data writeback */
-	truncate_inline_data(ipage);
+	truncate_inline_inode(ipage, 0);
 
 	stat_dec_inline_dir(dir);
 	clear_inode_flag(F2FS_I(dir), FI_INLINE_DENTRY);
--

2.1.1

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Tuesday, March 10, 2015 11:00 AM
To: Chao Yu
Cc: 'Changman Lee'; [email protected]; [email protected]
Subject: Re: [PATCH] f2fs: fix to truncate inline data past EOF

Hi Chao,

Quoted 2 lines · Chao Yu

Hi Jaegeuk,

[snip]

Quoted 22 lines

Quoted 19 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +2 -1

+static int truncate_partial_data_page(struct inode *inode, u64 from, bool force)
 {
 	unsigned offset = from & (PAGE_CACHE_SIZE - 1);
 	struct page *page;
 
-	if (!offset)
+	if (!offset && !force)

We truncate on-disk inode page and #0 page separately, so IMO there is a very small chance the following issue will occur.

Quoted 7 lines

(0 < truncated_size < MAX_INLINE_SIZE)
->f2fs_setattr
->truncate_setsize #0 page has been truncated partially
->f2fs_truncate
->truncate_blocks
invalidate #0 page by reclaimer
update #0 page with original data in inode page by reader

Quoted 2 lines

truncate_setsize called truncate_pagesize. so #0 page was truncated successfully.

Quoted 7 lines

Yeah, but it's partially truncating as 0 < truncated_size < MAX_INLINE_SIZE, After truncating, we still keep [0, truncated_size) valid data in #0 page, and our #0 page status is uptodate | !dirty.

So what I mean is that mm can reclaim this #0 page, then if someone else read the #0 page again, we will update the #0 page with original data in inode page since inode page haven't truncated yet.

The truncate_blocks dropped inline_data in inode page, which is modified by this patch.
And, the cached #0 page was truncated by truncate_setsize.
Even if this page was evicted and reloaded again, the data would be filled with the inode page having truncated inline_data.
So, it seems there is no problem.

BTW, do you mean like this scenario?

-> f2fs_setattr
->truncate_setsize: #0 page has been truncated partially ->f2fs_truncate

(quoted diff) +0 -0

 					invalidate #0 page by reclaimer
 					update #0 page with original data in inode page by reader
 
    ->truncate_blocks
      (*)-> truncate_inline_inode
    (*)-> truncate_partial_data_page(,, force)
       find_data_page(,, force)  <- we can use *force* here.

Then, I agree that two functions as noted (*) above would be necessary.

Quoted 68 lines

If this happened, if we don't truncate #0 page in following code of truncate_blocks, we will remain the original data for user, it's wrong.

IMO, it's better to truncate inode page and #0 page together, or truncate #0 page in truncate_partial_data_page.

How do you think?

Quoted 11 lines

Quoted 8 lines

->truncate_inline_inode
->truncate_partial_data_page
we will fail to truncate #0 page because we can't find valid blkaddr for #0 page in find_data_page(,,false)

How about using find_data_page(,,true) to check weather this page is update or not for this condition.

Oh, I realized that we don't need to call truncate_partial_data_page, since the cached #0 page was truncated already. We don't care about that.

IMO, we should care about this #0 page if above example can happen.

Quoted 24 lines

So, do we need to add just truncate_inline_inode() like below?

Quoted 15 lines

Quoted 15 lines

(quoted diff) +1 -0

 		return 0;
 
 	page = find_data_page(inode, from >> PAGE_CACHE_SHIFT, false);

@@ -489,6 +489,7 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	pgoff_t free_from;
 	int count = 0, err = 0;
 	struct page *ipage;
+	bool truncate_page = false;
 
 	trace_f2fs_truncate_blocks_enter(inode, from);
 

@@ -504,6 +505,9 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	}
 

Quoted 6 lines

fs/f2fs/file.c int truncate_blocks(struct inode *inode, u64 from, bool lock)

Quoted 3 lines

fs/f2fs/file.c +2 -0

 	if (f2fs_has_inline_data(inode)) {
+		truncate_inline_inode(ipage, from);
+		set_page_dirty(ipage);

If @from is the same as MAX_INLINE_DATA, we will change nothing in inode page, How about skipping set_page_dirty for inode page in this condition?

Quoted 7 lines

Agreed.
How about adding this in truncate_inline_inode?

	if (from >= MAX_INLINE_DATA)
		return;
	...
	set_page_dirty(ipage);

Yeah, that's good.

And What I suggest is:

bool truncate_inline_inode(struct page *ipage, u64 from)

{
	/*
	 * we should never truncate inline data past max inline data size,
	 * because we always convert inline inode to normal one before
	 * truncating real data if new size is past max inline data size.
	 */
	f2fs_bug_on(F2FS_P_SB(ipage), from > MAX_INLINE_DATA);

Quoted 1 line

	if (from == MAX_INLINE_DATA)

if (from >= MAX_INLINE_DATA) to handle when f2fs_bug_on is bypassed.

Quoted 12 lines

		return false;
	...
	return true;
}

in truncate_blocks()

 	if (f2fs_has_inline_data(inode)) {
		if (truncate_inline_inode(ipage, from))
			set_page_dirty(ipage);

Quoted 3 lines

So by this way, we can distinguish between a bug and invalid truncating in truncate_inline_inode, and also we can avoid unneeded set_dirty_page for inode page in other call path.

Ok, it's not a big deal.