[PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC

[PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC

[Gu Zheng]

When we submit bio with READ_SYNC or WRITE_SYNC, we need to wait a
moment for the io completion, current codes only find_data_page() follows the
rule, other places missing this step, so add it.

Further more, moving the PageUptodate check into f2fs_readpage() to clean up
the codes.

Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 fs/f2fs/checkpoint.c |    1 -
 fs/f2fs/data.c       |   39 +++++++++++++++++----------------------
 fs/f2fs/node.c       |    1 -
 fs/f2fs/recovery.c   |    2 --
 fs/f2fs/segment.c    |    2 +-
 5 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index fe91773..e376a42 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -64,7 +64,6 @@ repeat:
 	if (f2fs_readpage(sbi, page, index, READ_SYNC))
 		goto repeat;
 
-	lock_page(page);
 	if (page->mapping != mapping) {
 		f2fs_put_page(page, 1);
 		goto repeat;
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 19cd7c6..b048936 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -216,13 +216,11 @@ struct page *find_data_page(struct inode *inode, pgoff_t index, bool sync)
 
 	err = f2fs_readpage(sbi, page, dn.data_blkaddr,
 					sync ? READ_SYNC : READA);
-	if (sync) {
-		wait_on_page_locked(page);
-		if (!PageUptodate(page)) {
-			f2fs_put_page(page, 0);
-			return ERR_PTR(-EIO);
-		}
-	}
+	if (err)
+		return ERR_PTR(err);
+
+	if (sync)
+		unlock_page(page);
 	return page;
 }
 
@@ -267,11 +265,6 @@ repeat:
 	if (err)
 		return ERR_PTR(err);
 
-	lock_page(page);
-	if (!PageUptodate(page)) {
-		f2fs_put_page(page, 1);
-		return ERR_PTR(-EIO);
-	}
 	if (page->mapping != mapping) {
 		f2fs_put_page(page, 1);
 		goto repeat;
@@ -325,11 +318,7 @@ repeat:
 		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
 		if (err)
 			return ERR_PTR(err);
-		lock_page(page);
-		if (!PageUptodate(page)) {
-			f2fs_put_page(page, 1);
-			return ERR_PTR(-EIO);
-		}
+
 		if (page->mapping != mapping) {
 			f2fs_put_page(page, 1);
 			goto repeat;
@@ -399,6 +388,16 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page,
 
 	submit_bio(type, bio);
 	up_read(&sbi->bio_sem);
+
+	if (type == READ_SYNC) {
+		wait_on_page_locked(page);
+		lock_page(page);
+		if (!PageUptodate(page)) {
+			f2fs_put_page(page, 1);
+			return -EIO;
+		}
+	}
+
 	return 0;
 }
 
@@ -679,11 +678,7 @@ repeat:
 		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
 		if (err)
 			return err;
-		lock_page(page);
-		if (!PageUptodate(page)) {
-			f2fs_put_page(page, 1);
-			return -EIO;
-		}
+
 		if (page->mapping != mapping) {
 			f2fs_put_page(page, 1);
 			goto repeat;
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index f5172e2..f061554 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1534,7 +1534,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
 		if (f2fs_readpage(sbi, page, addr, READ_SYNC))
 			goto out;
 
-		lock_page(page);
 		rn = F2FS_NODE(page);
 		sum_entry->nid = rn->footer.nid;
 		sum_entry->version = 0;
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 639eb34..ec68183 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -140,8 +140,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
 		if (err)
 			goto out;
 
-		lock_page(page);
-
 		if (cp_ver != cpver_of_node(page))
 			break;
 
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9b74ae2..bcd19db 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -639,7 +639,7 @@ static void do_submit_bio(struct f2fs_sb_info *sbi,
 
 		trace_f2fs_do_submit_bio(sbi->sb, btype, sync, sbi->bio[btype]);
 
-		if (type == META_FLUSH) {
+		if ((type == META_FLUSH) || (rw & WRITE_SYNC)) {
 			DECLARE_COMPLETION_ONSTACK(wait);
 			p->is_sync = true;
 			p->wait = &wait;
-- 
1.7.7

Re: [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC

[Jaegeuk Kim]

Hi Gu,

The original read flow was to avoid redandunt lock/unlock_page() calls.
And we should not wait for WRITE_SYNC, since it is just for write
priority, not for synchronization of the file system.
Thanks,

2013-07-30 (화), 18:06 +0800, Gu Zheng:
> When we submit bio with READ_SYNC or WRITE_SYNC, we need to wait a
> moment for the io completion, current codes only find_data_page() follows the
> rule, other places missing this step, so add it.
> 
> Further more, moving the PageUptodate check into f2fs_readpage() to clean up
> the codes.
> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  fs/f2fs/checkpoint.c |    1 -
>  fs/f2fs/data.c       |   39 +++++++++++++++++----------------------
>  fs/f2fs/node.c       |    1 -
>  fs/f2fs/recovery.c   |    2 --
>  fs/f2fs/segment.c    |    2 +-
>  5 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index fe91773..e376a42 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -64,7 +64,6 @@ repeat:
>  	if (f2fs_readpage(sbi, page, index, READ_SYNC))
>  		goto repeat;
>  
> -	lock_page(page);
>  	if (page->mapping != mapping) {
>  		f2fs_put_page(page, 1);
>  		goto repeat;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 19cd7c6..b048936 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -216,13 +216,11 @@ struct page *find_data_page(struct inode *inode, pgoff_t index, bool sync)
>  
>  	err = f2fs_readpage(sbi, page, dn.data_blkaddr,
>  					sync ? READ_SYNC : READA);
> -	if (sync) {
> -		wait_on_page_locked(page);
> -		if (!PageUptodate(page)) {
> -			f2fs_put_page(page, 0);
> -			return ERR_PTR(-EIO);
> -		}
> -	}
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	if (sync)
> +		unlock_page(page);
>  	return page;
>  }
>  
> @@ -267,11 +265,6 @@ repeat:
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	lock_page(page);
> -	if (!PageUptodate(page)) {
> -		f2fs_put_page(page, 1);
> -		return ERR_PTR(-EIO);
> -	}
>  	if (page->mapping != mapping) {
>  		f2fs_put_page(page, 1);
>  		goto repeat;
> @@ -325,11 +318,7 @@ repeat:
>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>  		if (err)
>  			return ERR_PTR(err);
> -		lock_page(page);
> -		if (!PageUptodate(page)) {
> -			f2fs_put_page(page, 1);
> -			return ERR_PTR(-EIO);
> -		}
> +
>  		if (page->mapping != mapping) {
>  			f2fs_put_page(page, 1);
>  			goto repeat;
> @@ -399,6 +388,16 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page,
>  
>  	submit_bio(type, bio);
>  	up_read(&sbi->bio_sem);
> +
> +	if (type == READ_SYNC) {
> +		wait_on_page_locked(page);
> +		lock_page(page);
> +		if (!PageUptodate(page)) {
> +			f2fs_put_page(page, 1);
> +			return -EIO;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -679,11 +678,7 @@ repeat:
>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>  		if (err)
>  			return err;
> -		lock_page(page);
> -		if (!PageUptodate(page)) {
> -			f2fs_put_page(page, 1);
> -			return -EIO;
> -		}
> +
>  		if (page->mapping != mapping) {
>  			f2fs_put_page(page, 1);
>  			goto repeat;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index f5172e2..f061554 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1534,7 +1534,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>  		if (f2fs_readpage(sbi, page, addr, READ_SYNC))
>  			goto out;
>  
> -		lock_page(page);
>  		rn = F2FS_NODE(page);
>  		sum_entry->nid = rn->footer.nid;
>  		sum_entry->version = 0;
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 639eb34..ec68183 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -140,8 +140,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
>  		if (err)
>  			goto out;
>  
> -		lock_page(page);
> -
>  		if (cp_ver != cpver_of_node(page))
>  			break;
>  
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 9b74ae2..bcd19db 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -639,7 +639,7 @@ static void do_submit_bio(struct f2fs_sb_info *sbi,
>  
>  		trace_f2fs_do_submit_bio(sbi->sb, btype, sync, sbi->bio[btype]);
>  
> -		if (type == META_FLUSH) {
> +		if ((type == META_FLUSH) || (rw & WRITE_SYNC)) {
>  			DECLARE_COMPLETION_ONSTACK(wait);
>  			p->is_sync = true;
>  			p->wait = &wait;

-- 
Jaegeuk Kim
Samsung

--
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] f2fs: add a wait step when submit bio with {READ, WRITE}_SYNC

[Gu Zheng]

Hi Kim,

On 07/30/2013 08:29 PM, Jaegeuk Kim wrote:

> Hi Gu,
> 
> The original read flow was to avoid redandunt lock/unlock_page() calls.

Right, this can gain better read performance. But is the wait step after 
submitting bio with READ_SYNC needless too?

> And we should not wait for WRITE_SYNC, since it is just for write
> priority, not for synchronization of the file system.

Got it, thanks for your explanation.:) 

Best regards,
Gu

> Thanks,
> 
> 2013-07-30 (화), 18:06 +0800, Gu Zheng:
>> When we submit bio with READ_SYNC or WRITE_SYNC, we need to wait a
>> moment for the io completion, current codes only find_data_page() follows the
>> rule, other places missing this step, so add it.
>>
>> Further more, moving the PageUptodate check into f2fs_readpage() to clean up
>> the codes.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  fs/f2fs/checkpoint.c |    1 -
>>  fs/f2fs/data.c       |   39 +++++++++++++++++----------------------
>>  fs/f2fs/node.c       |    1 -
>>  fs/f2fs/recovery.c   |    2 --
>>  fs/f2fs/segment.c    |    2 +-
>>  5 files changed, 18 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index fe91773..e376a42 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -64,7 +64,6 @@ repeat:
>>  	if (f2fs_readpage(sbi, page, index, READ_SYNC))
>>  		goto repeat;
>>  
>> -	lock_page(page);
>>  	if (page->mapping != mapping) {
>>  		f2fs_put_page(page, 1);
>>  		goto repeat;
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 19cd7c6..b048936 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -216,13 +216,11 @@ struct page *find_data_page(struct inode *inode, pgoff_t index, bool sync)
>>  
>>  	err = f2fs_readpage(sbi, page, dn.data_blkaddr,
>>  					sync ? READ_SYNC : READA);
>> -	if (sync) {
>> -		wait_on_page_locked(page);
>> -		if (!PageUptodate(page)) {
>> -			f2fs_put_page(page, 0);
>> -			return ERR_PTR(-EIO);
>> -		}
>> -	}
>> +	if (err)
>> +		return ERR_PTR(err);
>> +
>> +	if (sync)
>> +		unlock_page(page);
>>  	return page;
>>  }
>>  
>> @@ -267,11 +265,6 @@ repeat:
>>  	if (err)
>>  		return ERR_PTR(err);
>>  
>> -	lock_page(page);
>> -	if (!PageUptodate(page)) {
>> -		f2fs_put_page(page, 1);
>> -		return ERR_PTR(-EIO);
>> -	}
>>  	if (page->mapping != mapping) {
>>  		f2fs_put_page(page, 1);
>>  		goto repeat;
>> @@ -325,11 +318,7 @@ repeat:
>>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>>  		if (err)
>>  			return ERR_PTR(err);
>> -		lock_page(page);
>> -		if (!PageUptodate(page)) {
>> -			f2fs_put_page(page, 1);
>> -			return ERR_PTR(-EIO);
>> -		}
>> +
>>  		if (page->mapping != mapping) {
>>  			f2fs_put_page(page, 1);
>>  			goto repeat;
>> @@ -399,6 +388,16 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page,
>>  
>>  	submit_bio(type, bio);
>>  	up_read(&sbi->bio_sem);
>> +
>> +	if (type == READ_SYNC) {
>> +		wait_on_page_locked(page);
>> +		lock_page(page);
>> +		if (!PageUptodate(page)) {
>> +			f2fs_put_page(page, 1);
>> +			return -EIO;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -679,11 +678,7 @@ repeat:
>>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>>  		if (err)
>>  			return err;
>> -		lock_page(page);
>> -		if (!PageUptodate(page)) {
>> -			f2fs_put_page(page, 1);
>> -			return -EIO;
>> -		}
>> +
>>  		if (page->mapping != mapping) {
>>  			f2fs_put_page(page, 1);
>>  			goto repeat;
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index f5172e2..f061554 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1534,7 +1534,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>>  		if (f2fs_readpage(sbi, page, addr, READ_SYNC))
>>  			goto out;
>>  
>> -		lock_page(page);
>>  		rn = F2FS_NODE(page);
>>  		sum_entry->nid = rn->footer.nid;
>>  		sum_entry->version = 0;
>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>> index 639eb34..ec68183 100644
>> --- a/fs/f2fs/recovery.c
>> +++ b/fs/f2fs/recovery.c
>> @@ -140,8 +140,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
>>  		if (err)
>>  			goto out;
>>  
>> -		lock_page(page);
>> -
>>  		if (cp_ver != cpver_of_node(page))
>>  			break;
>>  
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 9b74ae2..bcd19db 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -639,7 +639,7 @@ static void do_submit_bio(struct f2fs_sb_info *sbi,
>>  
>>  		trace_f2fs_do_submit_bio(sbi->sb, btype, sync, sbi->bio[btype]);
>>  
>> -		if (type == META_FLUSH) {
>> +		if ((type == META_FLUSH) || (rw & WRITE_SYNC)) {
>>  			DECLARE_COMPLETION_ONSTACK(wait);
>>  			p->is_sync = true;
>>  			p->wait = &wait;
> 



------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC

[Jaegeuk Kim]

2013-07-31 (수), 09:59 +0800, Gu Zheng:
> Hi Kim,
> 
> On 07/30/2013 08:29 PM, Jaegeuk Kim wrote:
> 
> > Hi Gu,
> > 
> > The original read flow was to avoid redandunt lock/unlock_page() calls.
> 
> Right, this can gain better read performance. But is the wait step after 
> submitting bio with READ_SYNC needless too?

Correct, the READ_SYNC is also used for IO priority.
The basic read policy here is that the caller should lock the page only
when it wants to manipulate there-in data.
Otherwise, we don't need to unnecessary lock and unlocks.
Thanks,

> 
> > And we should not wait for WRITE_SYNC, since it is just for write
> > priority, not for synchronization of the file system.
> 
> Got it, thanks for your explanation.:) 
> 
> Best regards,
> Gu
> 
> > Thanks,
> > 
> > 2013-07-30 (화), 18:06 +0800, Gu Zheng:
> >> When we submit bio with READ_SYNC or WRITE_SYNC, we need to wait a
> >> moment for the io completion, current codes only find_data_page() follows the
> >> rule, other places missing this step, so add it.
> >>
> >> Further more, moving the PageUptodate check into f2fs_readpage() to clean up
> >> the codes.
> >>
> >> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> >> ---
> >>  fs/f2fs/checkpoint.c |    1 -
> >>  fs/f2fs/data.c       |   39 +++++++++++++++++----------------------
> >>  fs/f2fs/node.c       |    1 -
> >>  fs/f2fs/recovery.c   |    2 --
> >>  fs/f2fs/segment.c    |    2 +-
> >>  5 files changed, 18 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >> index fe91773..e376a42 100644
> >> --- a/fs/f2fs/checkpoint.c
> >> +++ b/fs/f2fs/checkpoint.c
> >> @@ -64,7 +64,6 @@ repeat:
> >>  	if (f2fs_readpage(sbi, page, index, READ_SYNC))
> >>  		goto repeat;
> >>  
> >> -	lock_page(page);
> >>  	if (page->mapping != mapping) {
> >>  		f2fs_put_page(page, 1);
> >>  		goto repeat;
> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> index 19cd7c6..b048936 100644
> >> --- a/fs/f2fs/data.c
> >> +++ b/fs/f2fs/data.c
> >> @@ -216,13 +216,11 @@ struct page *find_data_page(struct inode *inode, pgoff_t index, bool sync)
> >>  
> >>  	err = f2fs_readpage(sbi, page, dn.data_blkaddr,
> >>  					sync ? READ_SYNC : READA);
> >> -	if (sync) {
> >> -		wait_on_page_locked(page);
> >> -		if (!PageUptodate(page)) {
> >> -			f2fs_put_page(page, 0);
> >> -			return ERR_PTR(-EIO);
> >> -		}
> >> -	}
> >> +	if (err)
> >> +		return ERR_PTR(err);
> >> +
> >> +	if (sync)
> >> +		unlock_page(page);
> >>  	return page;
> >>  }
> >>  
> >> @@ -267,11 +265,6 @@ repeat:
> >>  	if (err)
> >>  		return ERR_PTR(err);
> >>  
> >> -	lock_page(page);
> >> -	if (!PageUptodate(page)) {
> >> -		f2fs_put_page(page, 1);
> >> -		return ERR_PTR(-EIO);
> >> -	}
> >>  	if (page->mapping != mapping) {
> >>  		f2fs_put_page(page, 1);
> >>  		goto repeat;
> >> @@ -325,11 +318,7 @@ repeat:
> >>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
> >>  		if (err)
> >>  			return ERR_PTR(err);
> >> -		lock_page(page);
> >> -		if (!PageUptodate(page)) {
> >> -			f2fs_put_page(page, 1);
> >> -			return ERR_PTR(-EIO);
> >> -		}
> >> +
> >>  		if (page->mapping != mapping) {
> >>  			f2fs_put_page(page, 1);
> >>  			goto repeat;
> >> @@ -399,6 +388,16 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page,
> >>  
> >>  	submit_bio(type, bio);
> >>  	up_read(&sbi->bio_sem);
> >> +
> >> +	if (type == READ_SYNC) {
> >> +		wait_on_page_locked(page);
> >> +		lock_page(page);
> >> +		if (!PageUptodate(page)) {
> >> +			f2fs_put_page(page, 1);
> >> +			return -EIO;
> >> +		}
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -679,11 +678,7 @@ repeat:
> >>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
> >>  		if (err)
> >>  			return err;
> >> -		lock_page(page);
> >> -		if (!PageUptodate(page)) {
> >> -			f2fs_put_page(page, 1);
> >> -			return -EIO;
> >> -		}
> >> +
> >>  		if (page->mapping != mapping) {
> >>  			f2fs_put_page(page, 1);
> >>  			goto repeat;
> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >> index f5172e2..f061554 100644
> >> --- a/fs/f2fs/node.c
> >> +++ b/fs/f2fs/node.c
> >> @@ -1534,7 +1534,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
> >>  		if (f2fs_readpage(sbi, page, addr, READ_SYNC))
> >>  			goto out;
> >>  
> >> -		lock_page(page);
> >>  		rn = F2FS_NODE(page);
> >>  		sum_entry->nid = rn->footer.nid;
> >>  		sum_entry->version = 0;
> >> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >> index 639eb34..ec68183 100644
> >> --- a/fs/f2fs/recovery.c
> >> +++ b/fs/f2fs/recovery.c
> >> @@ -140,8 +140,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
> >>  		if (err)
> >>  			goto out;
> >>  
> >> -		lock_page(page);
> >> -
> >>  		if (cp_ver != cpver_of_node(page))
> >>  			break;
> >>  
> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >> index 9b74ae2..bcd19db 100644
> >> --- a/fs/f2fs/segment.c
> >> +++ b/fs/f2fs/segment.c
> >> @@ -639,7 +639,7 @@ static void do_submit_bio(struct f2fs_sb_info *sbi,
> >>  
> >>  		trace_f2fs_do_submit_bio(sbi->sb, btype, sync, sbi->bio[btype]);
> >>  
> >> -		if (type == META_FLUSH) {
> >> +		if ((type == META_FLUSH) || (rw & WRITE_SYNC)) {
> >>  			DECLARE_COMPLETION_ONSTACK(wait);
> >>  			p->is_sync = true;
> >>  			p->wait = &wait;
> > 
> 
> 
> --
> 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

--
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] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC

[Gu Zheng]

On 07/31/2013 06:06 PM, Jaegeuk Kim wrote:

> 2013-07-31 (수), 09:59 +0800, Gu Zheng:
>> Hi Kim,
>>
>> On 07/30/2013 08:29 PM, Jaegeuk Kim wrote:
>>
>>> Hi Gu,
>>>
>>> The original read flow was to avoid redandunt lock/unlock_page() calls.
>>
>> Right, this can gain better read performance. But is the wait step after 
>> submitting bio with READ_SYNC needless too?
> 
> Correct, the READ_SYNC is also used for IO priority.
> The basic read policy here is that the caller should lock the page only
> when it wants to manipulate there-in data.

> Otherwise, we don't need to unnecessary lock and unlocks.

Got it, it seems that I had some miss reading originally, it's
clear now, thanks very much for your explanation.:)

Regards,
Gu

> Thanks,

> 
>>
>>> And we should not wait for WRITE_SYNC, since it is just for write
>>> priority, not for synchronization of the file system.
>>
>> Got it, thanks for your explanation.:) 
>>
>> Best regards,
>> Gu
>>
>>> Thanks,
>>>
>>> 2013-07-30 (화), 18:06 +0800, Gu Zheng:
>>>> When we submit bio with READ_SYNC or WRITE_SYNC, we need to wait a
>>>> moment for the io completion, current codes only find_data_page() follows the
>>>> rule, other places missing this step, so add it.
>>>>
>>>> Further more, moving the PageUptodate check into f2fs_readpage() to clean up
>>>> the codes.
>>>>
>>>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>>> ---
>>>>  fs/f2fs/checkpoint.c |    1 -
>>>>  fs/f2fs/data.c       |   39 +++++++++++++++++----------------------
>>>>  fs/f2fs/node.c       |    1 -
>>>>  fs/f2fs/recovery.c   |    2 --
>>>>  fs/f2fs/segment.c    |    2 +-
>>>>  5 files changed, 18 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>> index fe91773..e376a42 100644
>>>> --- a/fs/f2fs/checkpoint.c
>>>> +++ b/fs/f2fs/checkpoint.c
>>>> @@ -64,7 +64,6 @@ repeat:
>>>>  	if (f2fs_readpage(sbi, page, index, READ_SYNC))
>>>>  		goto repeat;
>>>>  
>>>> -	lock_page(page);
>>>>  	if (page->mapping != mapping) {
>>>>  		f2fs_put_page(page, 1);
>>>>  		goto repeat;
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 19cd7c6..b048936 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -216,13 +216,11 @@ struct page *find_data_page(struct inode *inode, pgoff_t index, bool sync)
>>>>  
>>>>  	err = f2fs_readpage(sbi, page, dn.data_blkaddr,
>>>>  					sync ? READ_SYNC : READA);
>>>> -	if (sync) {
>>>> -		wait_on_page_locked(page);
>>>> -		if (!PageUptodate(page)) {
>>>> -			f2fs_put_page(page, 0);
>>>> -			return ERR_PTR(-EIO);
>>>> -		}
>>>> -	}
>>>> +	if (err)
>>>> +		return ERR_PTR(err);
>>>> +
>>>> +	if (sync)
>>>> +		unlock_page(page);
>>>>  	return page;
>>>>  }
>>>>  
>>>> @@ -267,11 +265,6 @@ repeat:
>>>>  	if (err)
>>>>  		return ERR_PTR(err);
>>>>  
>>>> -	lock_page(page);
>>>> -	if (!PageUptodate(page)) {
>>>> -		f2fs_put_page(page, 1);
>>>> -		return ERR_PTR(-EIO);
>>>> -	}
>>>>  	if (page->mapping != mapping) {
>>>>  		f2fs_put_page(page, 1);
>>>>  		goto repeat;
>>>> @@ -325,11 +318,7 @@ repeat:
>>>>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>>>>  		if (err)
>>>>  			return ERR_PTR(err);
>>>> -		lock_page(page);
>>>> -		if (!PageUptodate(page)) {
>>>> -			f2fs_put_page(page, 1);
>>>> -			return ERR_PTR(-EIO);
>>>> -		}
>>>> +
>>>>  		if (page->mapping != mapping) {
>>>>  			f2fs_put_page(page, 1);
>>>>  			goto repeat;
>>>> @@ -399,6 +388,16 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page,
>>>>  
>>>>  	submit_bio(type, bio);
>>>>  	up_read(&sbi->bio_sem);
>>>> +
>>>> +	if (type == READ_SYNC) {
>>>> +		wait_on_page_locked(page);
>>>> +		lock_page(page);
>>>> +		if (!PageUptodate(page)) {
>>>> +			f2fs_put_page(page, 1);
>>>> +			return -EIO;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -679,11 +678,7 @@ repeat:
>>>>  		err = f2fs_readpage(sbi, page, dn.data_blkaddr, READ_SYNC);
>>>>  		if (err)
>>>>  			return err;
>>>> -		lock_page(page);
>>>> -		if (!PageUptodate(page)) {
>>>> -			f2fs_put_page(page, 1);
>>>> -			return -EIO;
>>>> -		}
>>>> +
>>>>  		if (page->mapping != mapping) {
>>>>  			f2fs_put_page(page, 1);
>>>>  			goto repeat;
>>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>>> index f5172e2..f061554 100644
>>>> --- a/fs/f2fs/node.c
>>>> +++ b/fs/f2fs/node.c
>>>> @@ -1534,7 +1534,6 @@ int restore_node_summary(struct f2fs_sb_info *sbi,
>>>>  		if (f2fs_readpage(sbi, page, addr, READ_SYNC))
>>>>  			goto out;
>>>>  
>>>> -		lock_page(page);
>>>>  		rn = F2FS_NODE(page);
>>>>  		sum_entry->nid = rn->footer.nid;
>>>>  		sum_entry->version = 0;
>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>> index 639eb34..ec68183 100644
>>>> --- a/fs/f2fs/recovery.c
>>>> +++ b/fs/f2fs/recovery.c
>>>> @@ -140,8 +140,6 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head)
>>>>  		if (err)
>>>>  			goto out;
>>>>  
>>>> -		lock_page(page);
>>>> -
>>>>  		if (cp_ver != cpver_of_node(page))
>>>>  			break;
>>>>  
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 9b74ae2..bcd19db 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -639,7 +639,7 @@ static void do_submit_bio(struct f2fs_sb_info *sbi,
>>>>  
>>>>  		trace_f2fs_do_submit_bio(sbi->sb, btype, sync, sbi->bio[btype]);
>>>>  
>>>> -		if (type == META_FLUSH) {
>>>> +		if ((type == META_FLUSH) || (rw & WRITE_SYNC)) {
>>>>  			DECLARE_COMPLETION_ONSTACK(wait);
>>>>  			p->is_sync = true;
>>>>  			p->wait = &wait;
>>>
>>
>>
>> --
>> 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
>