[PATCH 1/2] f2fs: read with READ_SYNC when getting dnode page

[PATCH 1/2] f2fs: read with READ_SYNC when getting dnode page

[Jaegeuk Kim]

It must be set READ_SYNC not READA.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index e275218..185454f 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -930,7 +930,7 @@ repeat:
 	if (!page)
 		return ERR_PTR(-ENOMEM);
 
-	err = read_node_page(page, READA);
+	err = read_node_page(page, READ_SYNC);
 	if (err) {
 		f2fs_put_page(page, 1);
 		return ERR_PTR(err);
-- 
1.8.1.3.566.gaa39828

[PATCH 2/2] f2fs: introduce readahead mode of node pages

[Jaegeuk Kim]

Previously, f2fs reads several node pages ahead when get_dnode_of_data is called
with RDONLY_NODE flag.
And, this flag is set by the following functions.
- get_data_block_ro
- get_lock_data_page
- do_write_data_page
- truncate_blocks
- truncate_hole

However, this readahead mechanism is initially introduced for the use of
get_data_block_ro to enhance the sequential read performance.

So, let's clarify all the cases with the additional modes as follows.

enum {
	ALLOC_NODE,	/* allocate a new node page if needed */
	LOOKUP_NODE,	/* look up a node without readahead */
	LOOKUP_NODE_RA,	/*
			 * look up a node with readahead called
			 * by get_datablock_ro.
			 */
}

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/data.c     | 12 ++++++------
 fs/f2fs/f2fs.h     | 12 ++++++++----
 fs/f2fs/file.c     |  8 ++++----
 fs/f2fs/node.c     |  6 +++---
 fs/f2fs/recovery.c |  2 +-
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7bd22a2..277966a 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -183,7 +183,7 @@ struct page *find_data_page(struct inode *inode, pgoff_t index)
 	f2fs_put_page(page, 0);
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, index, RDONLY_NODE);
+	err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
 	if (err)
 		return ERR_PTR(err);
 	f2fs_put_dnode(&dn);
@@ -222,7 +222,7 @@ struct page *get_lock_data_page(struct inode *inode, pgoff_t index)
 	int err;
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, index, RDONLY_NODE);
+	err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
 	if (err)
 		return ERR_PTR(err);
 	f2fs_put_dnode(&dn);
@@ -262,7 +262,7 @@ struct page *get_new_data_page(struct inode *inode, pgoff_t index,
 	int err;
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, index, 0);
+	err = get_dnode_of_data(&dn, index, ALLOC_NODE);
 	if (err)
 		return ERR_PTR(err);
 
@@ -392,7 +392,7 @@ static int get_data_block_ro(struct inode *inode, sector_t iblock,
 
 	/* When reading holes, we need its node page */
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, pgofs, RDONLY_NODE);
+	err = get_dnode_of_data(&dn, pgofs, LOOKUP_NODE_RA);
 	if (err)
 		return (err == -ENOENT) ? 0 : err;
 
@@ -443,7 +443,7 @@ int do_write_data_page(struct page *page)
 	int err = 0;
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, page->index, RDONLY_NODE);
+	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
 		return err;
 
@@ -607,7 +607,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 	mutex_lock_op(sbi, DATA_NEW);
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, index, 0);
+	err = get_dnode_of_data(&dn, index, ALLOC_NODE);
 	if (err) {
 		mutex_unlock_op(sbi, DATA_NEW);
 		f2fs_put_page(page, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cc2213a..be7ae70 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -125,11 +125,15 @@ static inline int update_sits_in_cursum(struct f2fs_summary_block *rs, int i)
 					 * file keeping -1 as its node offset to
 					 * distinguish from index node blocks.
 					 */
-#define RDONLY_NODE		1	/*
-					 * specify a read-only mode when getting
-					 * a node block. 0 is read-write mode.
-					 * used by get_dnode_of_data().
+enum {
+	ALLOC_NODE,			/* allocate a new node page if needed */
+	LOOKUP_NODE,			/* look up a node without readahead */
+	LOOKUP_NODE_RA,			/*
+					 * look up a node with readahead called
+					 * by get_datablock_ro.
 					 */
+};
+
 #define F2FS_LINK_MAX		32000	/* maximum link count per file */
 
 /* for in-memory extent cache entry */
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index b7a053d..d4e29a5 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -43,7 +43,7 @@ static int f2fs_vm_page_mkwrite(struct vm_area_struct *vma,
 
 	/* block allocation */
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, page->index, 0);
+	err = get_dnode_of_data(&dn, page->index, ALLOC_NODE);
 	if (err) {
 		mutex_unlock_op(sbi, DATA_NEW);
 		goto out;
@@ -258,7 +258,7 @@ static int truncate_blocks(struct inode *inode, u64 from)
 	mutex_lock_op(sbi, DATA_TRUNC);
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	err = get_dnode_of_data(&dn, free_from, RDONLY_NODE);
+	err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE);
 	if (err) {
 		if (err == -ENOENT)
 			goto free_next;
@@ -420,7 +420,7 @@ int truncate_hole(struct inode *inode, pgoff_t pg_start, pgoff_t pg_end)
 
 		mutex_lock_op(sbi, DATA_TRUNC);
 		set_new_dnode(&dn, inode, NULL, NULL, 0);
-		err = get_dnode_of_data(&dn, index, RDONLY_NODE);
+		err = get_dnode_of_data(&dn, index, LOOKUP_NODE);
 		if (err) {
 			mutex_unlock_op(sbi, DATA_TRUNC);
 			if (err == -ENOENT)
@@ -504,7 +504,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
 		mutex_lock_op(sbi, DATA_NEW);
 
 		set_new_dnode(&dn, inode, NULL, NULL, 0);
-		ret = get_dnode_of_data(&dn, index, 0);
+		ret = get_dnode_of_data(&dn, index, ALLOC_NODE);
 		if (ret) {
 			mutex_unlock_op(sbi, DATA_NEW);
 			break;
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 185454f..e88bf2b 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -384,7 +384,7 @@ got:
 /*
  * Caller should call f2fs_put_dnode(dn).
  */
-int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int ro)
+int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int mode)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(dn->inode->i_sb);
 	struct page *npage[4];
@@ -411,7 +411,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int ro)
 	for (i = 1; i <= level; i++) {
 		bool done = false;
 
-		if (!nids[i] && !ro) {
+		if (!nids[i] && mode == ALLOC_NODE) {
 			mutex_lock_op(sbi, NODE_NEW);
 
 			/* alloc new node */
@@ -434,7 +434,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t index, int ro)
 			alloc_nid_done(sbi, nids[i]);
 			mutex_unlock_op(sbi, NODE_NEW);
 			done = true;
-		} else if (ro && i == level && level > 1) {
+		} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
 			npage[i] = get_node_page_ra(parent, offset[i - 1]);
 			if (IS_ERR(npage[i])) {
 				err = PTR_ERR(npage[i]);
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index b235215..6b82e20 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -247,7 +247,7 @@ static void do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 		end = start + ADDRS_PER_BLOCK;
 
 	set_new_dnode(&dn, inode, NULL, NULL, 0);
-	if (get_dnode_of_data(&dn, start, 0))
+	if (get_dnode_of_data(&dn, start, ALLOC_NODE))
 		return;
 
 	wait_on_page_writeback(dn.node_page);
-- 
1.8.1.3.566.gaa39828

Re: [PATCH 2/2] f2fs: introduce readahead mode of node pages

[Namjae Jeon]

> @@ -434,7 +434,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t
> index, int ro)
>  			alloc_nid_done(sbi, nids[i]);
>  			mutex_unlock_op(sbi, NODE_NEW);
>  			done = true;
> -		} else if (ro && i == level && level > 1) {
> +		} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
>  			npage[i] = get_node_page_ra(parent, offset[i - 1]);
>  			if (IS_ERR(npage[i])) {
>  				err = PTR_ERR(npage[i]);

Hi Jaegeuk.
There is no LOOKUP_NODE usage in this patch.
I think that we can use LOOKUP_NODE flag instead of done(bool)  like this.
 if (mode == LOOKUP_NODE)

Thanks.
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index b235215..6b82e20 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -247,7 +247,7 @@ static void do_recover_data(struct f2fs_sb_info *sbi,
> struct inode *inode,
>  		end = start + ADDRS_PER_BLOCK;
>
>  	set_new_dnode(&dn, inode, NULL, NULL, 0);
> -	if (get_dnode_of_data(&dn, start, 0))
> +	if (get_dnode_of_data(&dn, start, ALLOC_NODE))
>  		return;
>
>  	wait_on_page_writeback(dn.node_page);
> --
> 1.8.1.3.566.gaa39828
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/2] f2fs: introduce readahead mode of node pages

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

2013-02-26 (화), 20:52 +0900, Namjae Jeon:
> > @@ -434,7 +434,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t
> > index, int ro)
> >  			alloc_nid_done(sbi, nids[i]);
> >  			mutex_unlock_op(sbi, NODE_NEW);
> >  			done = true;
> > -		} else if (ro && i == level && level > 1) {
> > +		} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
> >  			npage[i] = get_node_page_ra(parent, offset[i - 1]);
> >  			if (IS_ERR(npage[i])) {
> >  				err = PTR_ERR(npage[i]);
> 
> Hi Jaegeuk.
> There is no LOOKUP_NODE usage in this patch.
> I think that we can use LOOKUP_NODE flag instead of done(bool)  like this.
>  if (mode == LOOKUP_NODE)

Hi.
In order to do that, we should check additional conditions like i and
level together with mode == LOOKUP_NODE.
So, I'm not sure how much it makes clearer by using LOOKUP_NODE
explicitly.
It seems fine to me, since we can just use LOOKUP_NODE to distinguish it
from the other modes.
Any thought?

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] f2fs: introduce readahead mode of node pages

[Namjae Jeon]

2013/2/27, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 2013-02-26 (화), 20:52 +0900, Namjae Jeon:
>> > @@ -434,7 +434,7 @@ int get_dnode_of_data(struct dnode_of_data *dn,
>> > pgoff_t
>> > index, int ro)
>> >  			alloc_nid_done(sbi, nids[i]);
>> >  			mutex_unlock_op(sbi, NODE_NEW);
>> >  			done = true;
>> > -		} else if (ro && i == level && level > 1) {
>> > +		} else if (mode == LOOKUP_NODE_RA && i == level && level > 1) {
>> >  			npage[i] = get_node_page_ra(parent, offset[i - 1]);
>> >  			if (IS_ERR(npage[i])) {
>> >  				err = PTR_ERR(npage[i]);
>>
>> Hi Jaegeuk.
>> There is no LOOKUP_NODE usage in this patch.
>> I think that we can use LOOKUP_NODE flag instead of done(bool)  like
>> this.
>>  if (mode == LOOKUP_NODE)
>
> Hi.
> In order to do that, we should check additional conditions like i and
> level together with mode == LOOKUP_NODE.
> So, I'm not sure how much it makes clearer by using LOOKUP_NODE
> explicitly.
> It seems fine to me, since we can just use LOOKUP_NODE to distinguish it
> from the other modes.
> Any thought?
I agree. And It does really need additional condition.
So Rather, It can make more complicated If using LOOKUP_NODE.
Looks reasonable to me on current change.

Reviewed-by: Namjae Jeon <namjae.jeon@samsung.com>

Thanks!
>
> --
> Jaegeuk Kim
> Samsung
>

Re: [PATCH 1/2] f2fs: read with READ_SYNC when getting dnode page

[Namjae Jeon]

2013/2/26, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> It must be set READ_SYNC not READA.
Hi Jaegeuk.
Could you please elaborate more?
Why we need to change READA to READ_SYNC over here, when the purpose
was to read the node page in READ ahead mode.

Thanks.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> ---
>  fs/f2fs/node.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index e275218..185454f 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -930,7 +930,7 @@ repeat:
>  	if (!page)
>  		return ERR_PTR(-ENOMEM);
>
> -	err = read_node_page(page, READA);
> +	err = read_node_page(page, READ_SYNC);
>  	if (err) {
>  		f2fs_put_page(page, 1);
>  		return ERR_PTR(err);
> --
> 1.8.1.3.566.gaa39828
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] f2fs: read with READ_SYNC when getting dnode page

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

Hi,

2013-02-26 (화), 18:35 +0900, Namjae Jeon:
> 2013/2/26, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> > It must be set READ_SYNC not READA.
> Hi Jaegeuk.
> Could you please elaborate more?
> Why we need to change READA to READ_SYNC over here, when the purpose
> was to read the node page in READ ahead mode.

That point was a reason for me to make this bug before.
This is get_node_page_ra, not ra_node_page, which means that
ra_node_page does actual readahead for another node pages.

In summary, get_node_page_ra tries to:
1. grab or read a target node page for the given nid,
2. then, call ra_node_page to read other adjacent node pages in advance.

So, when we try to read a target node page by #1, we should submit bio
with READ_SYNC instead of READA.

I'll add this to the description.
Thanks,

> 
> Thanks.
> >
> > Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> > ---
> >  fs/f2fs/node.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index e275218..185454f 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -930,7 +930,7 @@ repeat:
> >  	if (!page)
> >  		return ERR_PTR(-ENOMEM);
> >
> > -	err = read_node_page(page, READA);
> > +	err = read_node_page(page, READ_SYNC);
> >  	if (err) {
> >  		f2fs_put_page(page, 1);
> >  		return ERR_PTR(err);
> > --
> > 1.8.1.3.566.gaa39828
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] f2fs: read with READ_SYNC when getting dnode page

[Namjae Jeon]

2013/2/27, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> Hi,
>
> 2013-02-26 (화), 18:35 +0900, Namjae Jeon:
>> 2013/2/26, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
>> > It must be set READ_SYNC not READA.
>> Hi Jaegeuk.
>> Could you please elaborate more?
>> Why we need to change READA to READ_SYNC over here, when the purpose
>> was to read the node page in READ ahead mode.
>
> That point was a reason for me to make this bug before.
> This is get_node_page_ra, not ra_node_page, which means that
> ra_node_page does actual readahead for another node pages.
>
> In summary, get_node_page_ra tries to:
> 1. grab or read a target node page for the given nid,
> 2. then, call ra_node_page to read other adjacent node pages in advance.
>
> So, when we try to read a target node page by #1, we should submit bio
> with READ_SYNC instead of READA.
>
> I'll add this to the description.
> Thanks,
okay, Clear :)
Reviewed-by: Namjae Jeon <namjae.jeon@samsung.com>
Thanks for explanation!
>
>>
>> Thanks.
>> >
>> > Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
>> > ---
>> >  fs/f2fs/node.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> > index e275218..185454f 100644
>> > --- a/fs/f2fs/node.c
>> > +++ b/fs/f2fs/node.c
>> > @@ -930,7 +930,7 @@ repeat:
>> >  	if (!page)
>> >  		return ERR_PTR(-ENOMEM);
>> >
>> > -	err = read_node_page(page, READA);
>> > +	err = read_node_page(page, READ_SYNC);
>> >  	if (err) {
>> >  		f2fs_put_page(page, 1);
>> >  		return ERR_PTR(err);
>> > --
>> > 1.8.1.3.566.gaa39828
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel"
>> > in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>
> --
> Jaegeuk Kim
> Samsung
>