[PATCH 1/4] f2fs: introduce prepare_write_begin to clean up

[PATCH 1/4] f2fs: introduce prepare_write_begin to clean up

[Jaegeuk Kim]

This patch adds prepare_write_begin to clean f2fs_write_begin.
The major role of this function is to convert any inline_data and allocate
or find block address.

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

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 958d826..49092da 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1410,6 +1410,51 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	}
 }
 
+static int prepare_write_begin(struct f2fs_sb_info *sbi,
+			struct page *page, loff_t pos, unsigned len,
+			block_t *blk_addr, bool *node_changed)
+{
+	struct inode *inode = page->mapping->host;
+	pgoff_t index = page->index;
+	struct dnode_of_data dn;
+	struct page *ipage;
+	int err = 0;
+
+	f2fs_lock_op(sbi);
+
+	/* check inline_data */
+	ipage = get_node_page(sbi, inode->i_ino);
+	if (IS_ERR(ipage)) {
+		err = PTR_ERR(ipage);
+		goto unlock_out;
+	}
+
+	set_new_dnode(&dn, inode, ipage, ipage, 0);
+
+	if (f2fs_has_inline_data(inode)) {
+		if (pos + len <= MAX_INLINE_DATA) {
+			read_inline_data(page, ipage);
+			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
+			sync_inode_page(&dn);
+			goto done;
+		} else {
+			err = f2fs_convert_inline_page(&dn, page);
+			if (err)
+				goto err_out;
+		}
+	}
+	err = f2fs_get_block(&dn, index);
+done:
+	/* convert_inline_page can make node_changed */
+	*blk_addr = dn.data_blkaddr;
+	*node_changed = dn.node_changed;
+err_out:
+	f2fs_put_dnode(&dn);
+unlock_out:
+	f2fs_unlock_op(sbi);
+	return err;
+}
+
 static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 		loff_t pos, unsigned len, unsigned flags,
 		struct page **pagep, void **fsdata)
@@ -1417,9 +1462,9 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 	struct inode *inode = mapping->host;
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct page *page = NULL;
-	struct page *ipage;
 	pgoff_t index = ((unsigned long long) pos) >> PAGE_CACHE_SHIFT;
-	struct dnode_of_data dn;
+	bool need_balance = false;
+	block_t blkaddr = NULL_ADDR;
 	int err = 0;
 
 	trace_f2fs_write_begin(inode, pos, len, flags);
@@ -1443,37 +1488,12 @@ repeat:
 
 	*pagep = page;
 
-	f2fs_lock_op(sbi);
-
-	/* check inline_data */
-	ipage = get_node_page(sbi, inode->i_ino);
-	if (IS_ERR(ipage)) {
-		err = PTR_ERR(ipage);
-		goto unlock_fail;
-	}
-
-	set_new_dnode(&dn, inode, ipage, ipage, 0);
-
-	if (f2fs_has_inline_data(inode)) {
-		if (pos + len <= MAX_INLINE_DATA) {
-			read_inline_data(page, ipage);
-			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
-			sync_inode_page(&dn);
-			goto put_next;
-		}
-		err = f2fs_convert_inline_page(&dn, page);
-		if (err)
-			goto put_fail;
-	}
-
-	err = f2fs_get_block(&dn, index);
+	err = prepare_write_begin(sbi, page, pos + len,
+					&blkaddr, &need_balance);
 	if (err)
-		goto put_fail;
-put_next:
-	f2fs_put_dnode(&dn);
-	f2fs_unlock_op(sbi);
+		goto fail;
 
-	if (dn.node_changed && has_not_enough_free_secs(sbi, 0)) {
+	if (need_balance && has_not_enough_free_secs(sbi, 0)) {
 		unlock_page(page);
 		f2fs_balance_fs(sbi);
 		lock_page(page);
@@ -1488,7 +1508,7 @@ put_next:
 
 	/* wait for GCed encrypted page writeback */
 	if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
-		f2fs_wait_on_encrypted_page_writeback(sbi, dn.data_blkaddr);
+		f2fs_wait_on_encrypted_page_writeback(sbi, blkaddr);
 
 	if (len == PAGE_CACHE_SIZE)
 		goto out_update;
@@ -1504,14 +1524,14 @@ put_next:
 		goto out_update;
 	}
 
-	if (dn.data_blkaddr == NEW_ADDR) {
+	if (blkaddr == NEW_ADDR) {
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 	} else {
 		struct f2fs_io_info fio = {
 			.sbi = sbi,
 			.type = DATA,
 			.rw = READ_SYNC,
-			.blk_addr = dn.data_blkaddr,
+			.blk_addr = blkaddr,
 			.page = page,
 			.encrypted_page = NULL,
 		};
@@ -1542,10 +1562,6 @@ out_clear:
 	clear_cold_data(page);
 	return 0;
 
-put_fail:
-	f2fs_put_dnode(&dn);
-unlock_fail:
-	f2fs_unlock_op(sbi);
 fail:
 	f2fs_put_page(page, 1);
 	f2fs_write_failed(mapping, pos + len);
-- 
2.5.4 (Apple Git-61)

[PATCH 2/4] f2fs: return early when trying to read null nid

[Jaegeuk Kim]

If get_node_page() gets zero nid, we can return early without getting a wrong
page. For example, get_dnode_of_data() can try to do that.

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

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 341de5d..e17128d 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1059,6 +1059,9 @@ struct page *get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
 {
 	struct page *page;
 	int err;
+
+	if (!nid)
+		return ERR_PTR(-ENOENT);
 repeat:
 	page = grab_cache_page(NODE_MAPPING(sbi), nid);
 	if (!page)
-- 
2.5.4 (Apple Git-61)

[PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Jaegeuk Kim]

If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
order to avoid the checkpoint latency in the write syscall.

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

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 49092da..d53cf4f 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
 	pgoff_t index = page->index;
 	struct dnode_of_data dn;
 	struct page *ipage;
+	bool locked = false;
+	struct extent_info ei;
 	int err = 0;
 
-	f2fs_lock_op(sbi);
-
+	if (f2fs_has_inline_data(inode) ||
+			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
+		f2fs_lock_op(sbi);
+		locked = true;
+	}
+restart:
 	/* check inline_data */
 	ipage = get_node_page(sbi, inode->i_ino);
 	if (IS_ERR(ipage)) {
@@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
 			read_inline_data(page, ipage);
 			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
 			sync_inode_page(&dn);
-			goto done;
 		} else {
 			err = f2fs_convert_inline_page(&dn, page);
 			if (err)
-				goto err_out;
+				goto out;
+			err = f2fs_get_block(&dn, index);
+		}
+	} else if (locked) {
+		err = f2fs_get_block(&dn, index);
+	} else {
+		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
+			dn.data_blkaddr = ei.blk + index - ei.fofs;
+		} else {
+			bool restart = false;
+
+			/* hole case */
+			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
+			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
+				restart = true;
+			if (restart) {
+				f2fs_put_dnode(&dn);
+				f2fs_lock_op(sbi);
+				locked = true;
+				goto restart;
+			}
 		}
 	}
-	err = f2fs_get_block(&dn, index);
-done:
+
 	/* convert_inline_page can make node_changed */
 	*blk_addr = dn.data_blkaddr;
 	*node_changed = dn.node_changed;
-err_out:
+out:
 	f2fs_put_dnode(&dn);
 unlock_out:
-	f2fs_unlock_op(sbi);
+	if (locked)
+		f2fs_unlock_op(sbi);
 	return err;
 }
 
@@ -1488,7 +1513,7 @@ repeat:
 
 	*pagep = page;
 
-	err = prepare_write_begin(sbi, page, pos + len,
+	err = prepare_write_begin(sbi, page, pos,  len,
 					&blkaddr, &need_balance);
 	if (err)
 		goto fail;
-- 
2.5.4 (Apple Git-61)

[PATCH 4/4] f2fs: declare static function

[Jaegeuk Kim]

The __f2fs_commit_super is static.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 597b533..75704d9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1195,7 +1195,7 @@ next:
 	return 0;
 }
 
-int __f2fs_commit_super(struct f2fs_sb_info *sbi, int block)
+static int __f2fs_commit_super(struct f2fs_sb_info *sbi, int block)
 {
 	struct f2fs_super_block *super = F2FS_RAW_SUPER(sbi);
 	struct buffer_head *bh;
-- 
2.5.4 (Apple Git-61)

RE: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, December 24, 2015 10:15 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid
> 
> If get_node_page() gets zero nid, we can return early without getting a wrong
> page. For example, get_dnode_of_data() can try to do that.

Good catch!

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/node.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 341de5d..e17128d 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1059,6 +1059,9 @@ struct page *get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
>  {
>  	struct page *page;
>  	int err;
> +
> +	if (!nid)
> +		return ERR_PTR(-ENOENT);

How about expand to check upper and lower boundary:

	if (check_nid_range)
		return ERR_PTR(-ENOENT);

Thanks,

>  repeat:
>  	page = grab_cache_page(NODE_MAPPING(sbi), nid);
>  	if (!page)
> --
> 2.5.4 (Apple Git-61)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Thursday, December 24, 2015 10:15 AM
> To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> 
> If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> order to avoid the checkpoint latency in the write syscall.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 49092da..d53cf4f 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>  	pgoff_t index = page->index;
>  	struct dnode_of_data dn;
>  	struct page *ipage;
> +	bool locked = false;
> +	struct extent_info ei;
>  	int err = 0;
> 
> -	f2fs_lock_op(sbi);
> -
> +	if (f2fs_has_inline_data(inode) ||
> +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> +		f2fs_lock_op(sbi);
> +		locked = true;
> +	}
> +restart:
>  	/* check inline_data */
>  	ipage = get_node_page(sbi, inode->i_ino);
>  	if (IS_ERR(ipage)) {
> @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
>  			read_inline_data(page, ipage);
>  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
>  			sync_inode_page(&dn);
> -			goto done;
>  		} else {
>  			err = f2fs_convert_inline_page(&dn, page);
>  			if (err)
> -				goto err_out;
> +				goto out;
> +			err = f2fs_get_block(&dn, index);

Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
have been set, so f2fs_get_block could be removed here.

> +		}
> +	} else if (locked) {
> +		err = f2fs_get_block(&dn, index);
> +	} else {
> +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> +		} else {
> +			bool restart = false;
> +
> +			/* hole case */
> +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);

Better to handle error case like -ENOMEM or -EIO.

Thanks,

> +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> +				restart = true;
> +			if (restart) {
> +				f2fs_put_dnode(&dn);
> +				f2fs_lock_op(sbi);
> +				locked = true;
> +				goto restart;
> +			}
>  		}
>  	}
> -	err = f2fs_get_block(&dn, index);
> -done:
> +
>  	/* convert_inline_page can make node_changed */
>  	*blk_addr = dn.data_blkaddr;
>  	*node_changed = dn.node_changed;
> -err_out:
> +out:
>  	f2fs_put_dnode(&dn);
>  unlock_out:
> -	f2fs_unlock_op(sbi);
> +	if (locked)
> +		f2fs_unlock_op(sbi);
>  	return err;
>  }
> 
> @@ -1488,7 +1513,7 @@ repeat:
> 
>  	*pagep = page;
> 
> -	err = prepare_write_begin(sbi, page, pos + len,
> +	err = prepare_write_begin(sbi, page, pos,  len,
>  					&blkaddr, &need_balance);
>  	if (err)
>  		goto fail;
> --
> 2.5.4 (Apple Git-61)
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid

[Jaegeuk Kim]

On Thu, Dec 24, 2015 at 01:49:24PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Thursday, December 24, 2015 10:15 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid
> > 
> > If get_node_page() gets zero nid, we can return early without getting a wrong
> > page. For example, get_dnode_of_data() can try to do that.
> 
> Good catch!
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/node.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 341de5d..e17128d 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -1059,6 +1059,9 @@ struct page *get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
> >  {
> >  	struct page *page;
> >  	int err;
> > +
> > +	if (!nid)
> > +		return ERR_PTR(-ENOENT);
> 
> How about expand to check upper and lower boundary:
> 
> 	if (check_nid_range)
> 		return ERR_PTR(-ENOENT);

It'd better to add f2fs_bug_on(check_nid_range()) after this.
Cause check_nid_range() checks its nid as *unlikely*, but this case seems
*likely*.

Thanks,

> 
> Thanks,
> 
> >  repeat:
> >  	page = grab_cache_page(NODE_MAPPING(sbi), nid);
> >  	if (!page)
> > --
> > 2.5.4 (Apple Git-61)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Jaegeuk Kim]

Hi Chao,

On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Thursday, December 24, 2015 10:15 AM
> > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > 
> > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > order to avoid the checkpoint latency in the write syscall.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 34 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 49092da..d53cf4f 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> >  	pgoff_t index = page->index;
> >  	struct dnode_of_data dn;
> >  	struct page *ipage;
> > +	bool locked = false;
> > +	struct extent_info ei;
> >  	int err = 0;
> > 
> > -	f2fs_lock_op(sbi);
> > -
> > +	if (f2fs_has_inline_data(inode) ||
> > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > +		f2fs_lock_op(sbi);
> > +		locked = true;
> > +	}
> > +restart:
> >  	/* check inline_data */
> >  	ipage = get_node_page(sbi, inode->i_ino);
> >  	if (IS_ERR(ipage)) {
> > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> >  			read_inline_data(page, ipage);
> >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> >  			sync_inode_page(&dn);
> > -			goto done;
> >  		} else {
> >  			err = f2fs_convert_inline_page(&dn, page);
> >  			if (err)
> > -				goto err_out;
> > +				goto out;
> > +			err = f2fs_get_block(&dn, index);
> 
> Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> have been set, so f2fs_get_block could be removed here.

I don't think so. The inline_data treats with 0'th index only.
In order to handle non-zero index, we should do f2fs_get_block.
Oh, if you meant zero index case only, we can do that like this.

			if (index)
				err = f2fs_get_block();

> 
> > +		}
> > +	} else if (locked) {
> > +		err = f2fs_get_block(&dn, index);
> > +	} else {
> > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > +		} else {
> > +			bool restart = false;
> > +
> > +			/* hole case */
> > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> 
> Better to handle error case like -ENOMEM or -EIO.

I think we can just give another chance at this moment and let f2fs_get_block()
handle that in the next round.

Thanks,

> 
> Thanks,
> 
> > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > +				restart = true;
> > +			if (restart) {
> > +				f2fs_put_dnode(&dn);
> > +				f2fs_lock_op(sbi);
> > +				locked = true;
> > +				goto restart;
> > +			}
> >  		}
> >  	}
> > -	err = f2fs_get_block(&dn, index);
> > -done:
> > +
> >  	/* convert_inline_page can make node_changed */
> >  	*blk_addr = dn.data_blkaddr;
> >  	*node_changed = dn.node_changed;
> > -err_out:
> > +out:
> >  	f2fs_put_dnode(&dn);
> >  unlock_out:
> > -	f2fs_unlock_op(sbi);
> > +	if (locked)
> > +		f2fs_unlock_op(sbi);
> >  	return err;
> >  }
> > 
> > @@ -1488,7 +1513,7 @@ repeat:
> > 
> >  	*pagep = page;
> > 
> > -	err = prepare_write_begin(sbi, page, pos + len,
> > +	err = prepare_write_begin(sbi, page, pos,  len,
> >  					&blkaddr, &need_balance);
> >  	if (err)
> >  		goto fail;
> > --
> > 2.5.4 (Apple Git-61)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Jaegeuk Kim]

Hi Chao,

On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> > 
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Thursday, December 24, 2015 10:15 AM
> > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > linux-f2fs-devel@lists.sourceforge.net
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > 
> > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > order to avoid the checkpoint latency in the write syscall.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 49092da..d53cf4f 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > >  	pgoff_t index = page->index;
> > >  	struct dnode_of_data dn;
> > >  	struct page *ipage;
> > > +	bool locked = false;
> > > +	struct extent_info ei;
> > >  	int err = 0;
> > > 
> > > -	f2fs_lock_op(sbi);
> > > -
> > > +	if (f2fs_has_inline_data(inode) ||
> > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > +		f2fs_lock_op(sbi);
> > > +		locked = true;
> > > +	}
> > > +restart:
> > >  	/* check inline_data */
> > >  	ipage = get_node_page(sbi, inode->i_ino);
> > >  	if (IS_ERR(ipage)) {
> > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > >  			read_inline_data(page, ipage);
> > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > >  			sync_inode_page(&dn);
> > > -			goto done;
> > >  		} else {
> > >  			err = f2fs_convert_inline_page(&dn, page);
> > >  			if (err)
> > > -				goto err_out;
> > > +				goto out;
> > > +			err = f2fs_get_block(&dn, index);
> > 
> > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > have been set, so f2fs_get_block could be removed here.
> 
> I don't think so. The inline_data treats with 0'th index only.
> In order to handle non-zero index, we should do f2fs_get_block.
> Oh, if you meant zero index case only, we can do that like this.
> 
> 			if (index)
> 				err = f2fs_get_block();

Actually, we need
			if (index || !dn.data_blkaddr)
				err = f2fs_get_block();

Since f2fs_convert_inline_page can return right away, if there is
no data in inline_data space.

Thanks,

> 
> > 
> > > +		}
> > > +	} else if (locked) {
> > > +		err = f2fs_get_block(&dn, index);
> > > +	} else {
> > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > +		} else {
> > > +			bool restart = false;
> > > +
> > > +			/* hole case */
> > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > 
> > Better to handle error case like -ENOMEM or -EIO.
> 
> I think we can just give another chance at this moment and let f2fs_get_block()
> handle that in the next round.
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > +				restart = true;
> > > +			if (restart) {
> > > +				f2fs_put_dnode(&dn);
> > > +				f2fs_lock_op(sbi);
> > > +				locked = true;
> > > +				goto restart;
> > > +			}
> > >  		}
> > >  	}
> > > -	err = f2fs_get_block(&dn, index);
> > > -done:
> > > +
> > >  	/* convert_inline_page can make node_changed */
> > >  	*blk_addr = dn.data_blkaddr;
> > >  	*node_changed = dn.node_changed;
> > > -err_out:
> > > +out:
> > >  	f2fs_put_dnode(&dn);
> > >  unlock_out:
> > > -	f2fs_unlock_op(sbi);
> > > +	if (locked)
> > > +		f2fs_unlock_op(sbi);
> > >  	return err;
> > >  }
> > > 
> > > @@ -1488,7 +1513,7 @@ repeat:
> > > 
> > >  	*pagep = page;
> > > 
> > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > >  					&blkaddr, &need_balance);
> > >  	if (err)
> > >  		goto fail;
> > > --
> > > 2.5.4 (Apple Git-61)
> > > 
> > > 
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

RE: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid

[Chao Yu]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, December 25, 2015 4:16 AM
> To: Chao Yu
> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid
> 
> On Thu, Dec 24, 2015 at 01:49:24PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > Sent: Thursday, December 24, 2015 10:15 AM
> > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > linux-f2fs-devel@lists.sourceforge.net
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 2/4] f2fs: return early when trying to read null nid
> > >
> > > If get_node_page() gets zero nid, we can return early without getting a wrong
> > > page. For example, get_dnode_of_data() can try to do that.
> >
> > Good catch!
> >
> > >
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/node.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > > index 341de5d..e17128d 100644
> > > --- a/fs/f2fs/node.c
> > > +++ b/fs/f2fs/node.c
> > > @@ -1059,6 +1059,9 @@ struct page *get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
> > >  {
> > >  	struct page *page;
> > >  	int err;
> > > +
> > > +	if (!nid)
> > > +		return ERR_PTR(-ENOENT);
> >
> > How about expand to check upper and lower boundary:
> >
> > 	if (check_nid_range)
> > 		return ERR_PTR(-ENOENT);
> 
> It'd better to add f2fs_bug_on(check_nid_range()) after this.
> Cause check_nid_range() checks its nid as *unlikely*, but this case seems
> *likely*.

Agreed.

Thanks,

> 
> Thanks,
> 
> >
> > Thanks,
> >
> > >  repeat:
> > >  	page = grab_cache_page(NODE_MAPPING(sbi), nid);
> > >  	if (!page)
> > > --
> > > 2.5.4 (Apple Git-61)
> > >
> > >
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

RE: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, December 25, 2015 5:52 AM
> To: Chao Yu
> Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> 
> Hi Chao,
> 
> On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> > Hi Chao,
> >
> > On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > > Hi Jaegeuk,
> > >
> > > > -----Original Message-----
> > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > Sent: Thursday, December 24, 2015 10:15 AM
> > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > > linux-f2fs-devel@lists.sourceforge.net
> > > > Cc: Jaegeuk Kim
> > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > >
> > > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > > order to avoid the checkpoint latency in the write syscall.
> > > >
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > ---
> > > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > index 49092da..d53cf4f 100644
> > > > --- a/fs/f2fs/data.c
> > > > +++ b/fs/f2fs/data.c
> > > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > >  	pgoff_t index = page->index;
> > > >  	struct dnode_of_data dn;
> > > >  	struct page *ipage;
> > > > +	bool locked = false;
> > > > +	struct extent_info ei;
> > > >  	int err = 0;
> > > >
> > > > -	f2fs_lock_op(sbi);
> > > > -
> > > > +	if (f2fs_has_inline_data(inode) ||
> > > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > > +		f2fs_lock_op(sbi);
> > > > +		locked = true;
> > > > +	}
> > > > +restart:
> > > >  	/* check inline_data */
> > > >  	ipage = get_node_page(sbi, inode->i_ino);
> > > >  	if (IS_ERR(ipage)) {
> > > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > >  			read_inline_data(page, ipage);
> > > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > > >  			sync_inode_page(&dn);
> > > > -			goto done;
> > > >  		} else {
> > > >  			err = f2fs_convert_inline_page(&dn, page);
> > > >  			if (err)
> > > > -				goto err_out;
> > > > +				goto out;
> > > > +			err = f2fs_get_block(&dn, index);
> > >
> > > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > > have been set, so f2fs_get_block could be removed here.
> >
> > I don't think so. The inline_data treats with 0'th index only.
> > In order to handle non-zero index, we should do f2fs_get_block.

non-zero index page should have been handled before grab_cache_page_write_begin.
We won't encounter such case here.

> > Oh, if you meant zero index case only, we can do that like this.
> >
> > 			if (index)
> > 				err = f2fs_get_block();
> 
> Actually, we need
> 			if (index || !dn.data_blkaddr)
> 				err = f2fs_get_block();
> 
> Since f2fs_convert_inline_page can return right away, if there is
> no data in inline_data space.

Yes, that's the case we should handle. :)

Thanks,

> 
> Thanks,
> 
> >
> > >
> > > > +		}
> > > > +	} else if (locked) {
> > > > +		err = f2fs_get_block(&dn, index);
> > > > +	} else {
> > > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > > +		} else {
> > > > +			bool restart = false;
> > > > +
> > > > +			/* hole case */
> > > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > >
> > > Better to handle error case like -ENOMEM or -EIO.
> >
> > I think we can just give another chance at this moment and let f2fs_get_block()
> > handle that in the next round.
> >
> > Thanks,
> >
> > >
> > > Thanks,
> > >
> > > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > > +				restart = true;
> > > > +			if (restart) {
> > > > +				f2fs_put_dnode(&dn);
> > > > +				f2fs_lock_op(sbi);
> > > > +				locked = true;
> > > > +				goto restart;
> > > > +			}
> > > >  		}
> > > >  	}
> > > > -	err = f2fs_get_block(&dn, index);
> > > > -done:
> > > > +
> > > >  	/* convert_inline_page can make node_changed */
> > > >  	*blk_addr = dn.data_blkaddr;
> > > >  	*node_changed = dn.node_changed;
> > > > -err_out:
> > > > +out:
> > > >  	f2fs_put_dnode(&dn);
> > > >  unlock_out:
> > > > -	f2fs_unlock_op(sbi);
> > > > +	if (locked)
> > > > +		f2fs_unlock_op(sbi);
> > > >  	return err;
> > > >  }
> > > >
> > > > @@ -1488,7 +1513,7 @@ repeat:
> > > >
> > > >  	*pagep = page;
> > > >
> > > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > > >  					&blkaddr, &need_balance);
> > > >  	if (err)
> > > >  		goto fail;
> > > > --
> > > > 2.5.4 (Apple Git-61)
> > > >
> > > >
> > > > ------------------------------------------------------------------------------
> > > > _______________________________________________
> > > > Linux-f2fs-devel mailing list
> > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin

[Jaegeuk Kim]

Hi Chao,

On Fri, Dec 25, 2015 at 09:18:06AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Friday, December 25, 2015 5:52 AM
> > To: Chao Yu
> > Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > 
> > Hi Chao,
> > 
> > On Thu, Dec 24, 2015 at 12:33:50PM -0800, Jaegeuk Kim wrote:
> > > Hi Chao,
> > >
> > > On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > > > > Sent: Thursday, December 24, 2015 10:15 AM
> > > > > To: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org;
> > > > > linux-f2fs-devel@lists.sourceforge.net
> > > > > Cc: Jaegeuk Kim
> > > > > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid f2fs_lock_op in f2fs_write_begin
> > > > >
> > > > > If f2fs_write_begin is to update data, we can bypass calling f2fs_lock_op() in
> > > > > order to avoid the checkpoint latency in the write syscall.
> > > > >
> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > ---
> > > > >  fs/f2fs/data.c | 43 ++++++++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 34 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > > > index 49092da..d53cf4f 100644
> > > > > --- a/fs/f2fs/data.c
> > > > > +++ b/fs/f2fs/data.c
> > > > > @@ -1418,10 +1418,16 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > > >  	pgoff_t index = page->index;
> > > > >  	struct dnode_of_data dn;
> > > > >  	struct page *ipage;
> > > > > +	bool locked = false;
> > > > > +	struct extent_info ei;
> > > > >  	int err = 0;
> > > > >
> > > > > -	f2fs_lock_op(sbi);
> > > > > -
> > > > > +	if (f2fs_has_inline_data(inode) ||
> > > > > +			(pos & PAGE_CACHE_MASK) >= i_size_read(inode)) {
> > > > > +		f2fs_lock_op(sbi);
> > > > > +		locked = true;
> > > > > +	}
> > > > > +restart:
> > > > >  	/* check inline_data */
> > > > >  	ipage = get_node_page(sbi, inode->i_ino);
> > > > >  	if (IS_ERR(ipage)) {
> > > > > @@ -1436,22 +1442,41 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi,
> > > > >  			read_inline_data(page, ipage);
> > > > >  			set_inode_flag(F2FS_I(inode), FI_DATA_EXIST);
> > > > >  			sync_inode_page(&dn);
> > > > > -			goto done;
> > > > >  		} else {
> > > > >  			err = f2fs_convert_inline_page(&dn, page);
> > > > >  			if (err)
> > > > > -				goto err_out;
> > > > > +				goto out;
> > > > > +			err = f2fs_get_block(&dn, index);
> > > >
> > > > Seems after f2fs_convert_inline_page, data_blkaddr and node_changed should
> > > > have been set, so f2fs_get_block could be removed here.
> > >
> > > I don't think so. The inline_data treats with 0'th index only.
> > > In order to handle non-zero index, we should do f2fs_get_block.
> 
> non-zero index page should have been handled before grab_cache_page_write_begin.
> We won't encounter such case here.

Alright.
So, let me go with:
		if (dn.data_blkaddr == NULL_ADDR)
			err = f2fs_get_block();

Thanks,

> 
> > > Oh, if you meant zero index case only, we can do that like this.
> > >
> > > 			if (index)
> > > 				err = f2fs_get_block();
> > 
> > Actually, we need
> > 			if (index || !dn.data_blkaddr)
> > 				err = f2fs_get_block();
> > 
> > Since f2fs_convert_inline_page can return right away, if there is
> > no data in inline_data space.
> 
> Yes, that's the case we should handle. :)
> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> > >
> > > >
> > > > > +		}
> > > > > +	} else if (locked) {
> > > > > +		err = f2fs_get_block(&dn, index);
> > > > > +	} else {
> > > > > +		if (f2fs_lookup_extent_cache(inode, index, &ei)) {
> > > > > +			dn.data_blkaddr = ei.blk + index - ei.fofs;
> > > > > +		} else {
> > > > > +			bool restart = false;
> > > > > +
> > > > > +			/* hole case */
> > > > > +			err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
> > > >
> > > > Better to handle error case like -ENOMEM or -EIO.
> > >
> > > I think we can just give another chance at this moment and let f2fs_get_block()
> > > handle that in the next round.
> > >
> > > Thanks,
> > >
> > > >
> > > > Thanks,
> > > >
> > > > > +			if (err || (!err && dn.data_blkaddr == NULL_ADDR))
> > > > > +				restart = true;
> > > > > +			if (restart) {
> > > > > +				f2fs_put_dnode(&dn);
> > > > > +				f2fs_lock_op(sbi);
> > > > > +				locked = true;
> > > > > +				goto restart;
> > > > > +			}
> > > > >  		}
> > > > >  	}
> > > > > -	err = f2fs_get_block(&dn, index);
> > > > > -done:
> > > > > +
> > > > >  	/* convert_inline_page can make node_changed */
> > > > >  	*blk_addr = dn.data_blkaddr;
> > > > >  	*node_changed = dn.node_changed;
> > > > > -err_out:
> > > > > +out:
> > > > >  	f2fs_put_dnode(&dn);
> > > > >  unlock_out:
> > > > > -	f2fs_unlock_op(sbi);
> > > > > +	if (locked)
> > > > > +		f2fs_unlock_op(sbi);
> > > > >  	return err;
> > > > >  }
> > > > >
> > > > > @@ -1488,7 +1513,7 @@ repeat:
> > > > >
> > > > >  	*pagep = page;
> > > > >
> > > > > -	err = prepare_write_begin(sbi, page, pos + len,
> > > > > +	err = prepare_write_begin(sbi, page, pos,  len,
> > > > >  					&blkaddr, &need_balance);
> > > > >  	if (err)
> > > > >  		goto fail;
> > > > > --
> > > > > 2.5.4 (Apple Git-61)
> > > > >
> > > > >
> > > > > ------------------------------------------------------------------------------
> > > > > _______________________________________________
> > > > > Linux-f2fs-devel mailing list
> > > > > Linux-f2fs-devel@lists.sourceforge.net
> > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@lists.sourceforge.net
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel