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 <[email protected]>
---
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)
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 <[email protected]>
---
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)
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 <[email protected]>
---
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)
The __f2fs_commit_super is static.
Signed-off-by: Jaegeuk Kim <[email protected]>
---
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)
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 10:15 AM
> To: [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, December 24, 2015 10:15 AM
> To: [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On Thu, Dec 24, 2015 at 01:49:24PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Thursday, December 24, 2015 10:15 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hi Chao,
On Thu, Dec 24, 2015 at 01:50:32PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Thursday, December 24, 2015 10:15 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
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:[email protected]]
> > > Sent: Thursday, December 24, 2015 10:15 AM
> > > To: [email protected]; [email protected];
> > > [email protected]
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Friday, December 25, 2015 4:16 AM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> 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:[email protected]]
> > > Sent: Thursday, December 24, 2015 10:15 AM
> > > To: [email protected]; [email protected];
> > > [email protected]
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Friday, December 25, 2015 5:52 AM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> 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:[email protected]]
> > > > Sent: Thursday, December 24, 2015 10:15 AM
> > > > To: [email protected]; [email protected];
> > > > [email protected]
> > > > 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 <[email protected]>
> > > > ---
> > > > 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
> > > > [email protected]
> > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hi Chao,
On Fri, Dec 25, 2015 at 09:18:06AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Friday, December 25, 2015 5:52 AM
> > To: Chao Yu
> > Cc: [email protected]; [email protected];
> > [email protected]
> > 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:[email protected]]
> > > > > Sent: Thursday, December 24, 2015 10:15 AM
> > > > > To: [email protected]; [email protected];
> > > > > [email protected]
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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
> > > > > [email protected]
> > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > >
> > > ------------------------------------------------------------------------------
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > [email protected]
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel