* [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC
@ 2013-07-30 10:06 Gu Zheng
2013-07-30 12:29 ` Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Gu Zheng @ 2013-07-30 10:06 UTC (permalink / raw)
To: Kim; +Cc: f2fs, fsdevel, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC
2013-07-30 10:06 [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC Gu Zheng
@ 2013-07-30 12:29 ` Jaegeuk Kim
2013-07-31 1:59 ` [PATCH] f2fs: add a wait step when submit bio with {READ, WRITE}_SYNC Gu Zheng
0 siblings, 1 reply; 5+ messages in thread
From: Jaegeuk Kim @ 2013-07-30 12:29 UTC (permalink / raw)
To: Gu Zheng; +Cc: f2fs, fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: add a wait step when submit bio with {READ, WRITE}_SYNC
2013-07-30 12:29 ` Jaegeuk Kim
@ 2013-07-31 1:59 ` Gu Zheng
2013-07-31 10:06 ` [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Gu Zheng @ 2013-07-31 1:59 UTC (permalink / raw)
To: jaegeuk.kim; +Cc: fsdevel, linux-kernel, f2fs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC
2013-07-31 1:59 ` [PATCH] f2fs: add a wait step when submit bio with {READ, WRITE}_SYNC Gu Zheng
@ 2013-07-31 10:06 ` Jaegeuk Kim
2013-07-31 10:09 ` Gu Zheng
0 siblings, 1 reply; 5+ messages in thread
From: Jaegeuk Kim @ 2013-07-31 10:06 UTC (permalink / raw)
To: Gu Zheng; +Cc: f2fs, fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC
2013-07-31 10:06 ` [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC Jaegeuk Kim
@ 2013-07-31 10:09 ` Gu Zheng
0 siblings, 0 replies; 5+ messages in thread
From: Gu Zheng @ 2013-07-31 10:09 UTC (permalink / raw)
To: jaegeuk.kim; +Cc: f2fs, fsdevel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-31 10:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-30 10:06 [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC Gu Zheng
2013-07-30 12:29 ` Jaegeuk Kim
2013-07-31 1:59 ` [PATCH] f2fs: add a wait step when submit bio with {READ, WRITE}_SYNC Gu Zheng
2013-07-31 10:06 ` [PATCH] f2fs: add a wait step when submit bio with {READ,WRITE}_SYNC Jaegeuk Kim
2013-07-31 10:09 ` Gu Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).