2013-07-30 10:10:19

by Gu Zheng

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

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 <[email protected]>
---
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

2013-07-30 12:30:21

by Jaegeuk Kim

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

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 <[email protected]>
> ---
> 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

2013-07-31 02:02:55

by Gu Zheng

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

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 <[email protected]>
>> ---
>> 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;
>

2013-07-31 10:07:01

by Jaegeuk Kim

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

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 <[email protected]>
> >> ---
> >> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Jaegeuk Kim
Samsung

2013-07-31 10:13:40

by Gu Zheng

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

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 <[email protected]>
>>>> ---
>>>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>