Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data

[Jaegeuk Kim <jaegeuk.kim@samsung.com>]

Hi,

2013-10-29 (화), 01:20 +0800, Huajun Li:
> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> > Hi,
> >
> > 2013-10-26 (토), 00:01 +0800, Huajun Li:
> >> From: Huajun Li <huajun.li@intel.com>
> >>
> >> Functions to implement inline data read/write, and move inline data to
> >> normal data block when file size exceeds inline data limitation.
> >>
> >> Signed-off-by: Huajun Li <huajun.li@intel.com>
> >> Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
> >> Signed-off-by: Weihong Xu <weihong.xu@intel.com>
> >> ---
> >>  fs/f2fs/Makefile |    2 +-
> >>  fs/f2fs/f2fs.h   |    7 +++
> >>  fs/f2fs/inline.c |  144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 152 insertions(+), 1 deletion(-)
> >>  create mode 100644 fs/f2fs/inline.c
> >>
> >
> > [snip]
> >
> >> +static int __f2fs_convert_inline_data(struct inode *inode, struct page *page)
> >> +{
> >> +     int err;
> >> +     struct page *ipage;
> >> +     struct dnode_of_data dn;
> >> +     void *src_addr, *dst_addr;
> >> +     struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> >> +
> >
> > Here..  f2fs_lock_op(sbi);
> >
> >> +     ipage = get_node_page(sbi, inode->i_ino);
> >> +     if (IS_ERR(ipage))
> >> +             return PTR_ERR(ipage);
> >> +
> >
> > Need to move f2fs_lock_op prior to get_node_page.
> >
> >> +     /* i_addr[0] is not used for inline data,
> >
> > Coding style.
> >          /*
> >           * ...
> >           */
> >
> >> +      * so reserving new block will not destroy inline data */
> >> +     err = f2fs_reserve_block(inode, &dn, 0);
> >> +     if (err) {
> >> +             f2fs_put_page(ipage, 1);
> >> +             f2fs_unlock_op(sbi);
> >> +             return err;
> >> +     }
> >
> > Need to move this too.
> >
> >> +
> >> +     src_addr = inline_data_addr(ipage);
> >> +     dst_addr = page_address(page);
> >> +     zero_user_segment(page, 0, PAGE_CACHE_SIZE);
> >
> > + space for readability.
> >
> >> +     /* Copy the whole inline data block */
> >> +     memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> >> +     zero_user_segment(ipage, INLINE_DATA_OFFSET,
> >> +                              INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> >> +     clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> >> +     set_raw_inline(F2FS_I(inode),
> >> +                     (struct f2fs_inode *)page_address(ipage));
> 
> Thanks for your comments, I will update them according to above comments.
> 
> >
> >    --> sync_inode_page()?
> >
> IMO, we just handle file data, so do we still need call sync_inode_page() ?

I think sync_inode_page() is more suitable, since we need to sync
between i_size, i_blocks and its i_flag, inline_data, at this moment.

> 
> >> +
> >> +     set_page_dirty(ipage);

Need to consider skipping set_page_dirty(ipage).

> >> +     f2fs_put_page(ipage, 1);
> >> +     set_page_dirty(page);
> >
> > Here... f2fs_unlock_op(sbi);
> >
> > Here, we need to consider data consistency.
> > Let's think about the below scenario.
> > 1. inline_data was written.
> > 2. sync_fs is done.
> > 3. additional data were written.
> >   : this triggers f2fs_convert_inline_data(), produces a page, and then
> > unsets the inline flag.
> > 4. checkpoint was done with updated inode page. Note that, checkpoint
> > doesn't guarantee any user data.
> > 5. sudden power-off is occurred.
> >   -> Once power-off-recovery is done, we loose the inline_data which was
> > written successfully at step #2.
> >
> > So, I think we need to do f2fs_sync_file() procedure at this moment.
> > Any idea?
> >
> 
> Yes, need consider this case carefully, thanks for your reminder.
> 
> Considering sudden power-off may happen before f2fs_sync_file() is
> called, so how about following solutions:
>      ...
>      /* Copy the whole inline data block */
>      memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> 
>     do f2fs_sync_file() procedure ( Or  do write_data_page() procedure ? )

Ya, but it may still have some conditions to do this or not.
One of them is checking whether previous inline data should be recovered
or not, for example.

> 
>      zero_user_segment(ipage, INLINE_DATA_OFFSET, INLINE_DATA_OFFSET +
> MAX_INLINE_DATA);
>      clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>      set_raw_inline(F2FS_I(inode), (struct f2fs_inode *)page_address(ipage));
>      ...
> 
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +int f2fs_convert_inline_data(struct inode *inode,
> >> +                          struct page *p, unsigned flags)
> >> +{
> >> +     int err;
> >> +     struct page *page;
> >> +
> >> +     if (p && !p->index) {
> >> +             page = p;
> >> +     } else {
> >> +             page = grab_cache_page_write_begin(inode->i_mapping, 0, flags);
> >> +             if (IS_ERR(page))
> >> +                     return PTR_ERR(page);
> >> +     }
> >> +
> >> +     err = __f2fs_convert_inline_data(inode, page);
> >> +
> >> +     if (p && !p->index)
> >> +             f2fs_put_page(page, 1);
> >> +
> >> +     return err;
> >> +}
> >> +
> >> +int f2fs_write_inline_data(struct inode *inode,
> >> +                        struct page *page, unsigned size)
> >> +{
> >> +     void *src_addr, *dst_addr;
> >> +     struct page *ipage;
> >> +     struct dnode_of_data dn;
> >> +     int err;
> >> +
> >> +     set_new_dnode(&dn, inode, NULL, NULL, 0);
> >> +     err = get_dnode_of_data(&dn, 0, LOOKUP_NODE);
> >> +     if (err)
> >> +             return err;
> >> +     ipage = dn.inode_page;
> >> +
> >> +     src_addr = page_address(page);
> >> +     dst_addr = inline_data_addr(ipage);
> >> +
> >> +     zero_user_segment(ipage, INLINE_DATA_OFFSET,
> >> +                              INLINE_DATA_OFFSET + MAX_INLINE_DATA);
> >> +     memcpy(dst_addr, src_addr, size);
> >> +     if (!f2fs_has_inline_data(inode))
> >> +             set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> >> +     set_raw_inline(F2FS_I(inode),
> >> +                     (struct f2fs_inode *)page_address(ipage));
> >> +     set_page_dirty(ipage);
> >> +
> >> +     if ((F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 2) ||
> >> +             (!F2FS_I(inode)->i_xattr_nid && inode->i_blocks == 1)) {
> >> +             f2fs_put_dnode(&dn);
> >> +             return 0;
> >> +     }
> >> +
> >> +     /* Release the first data block if it is allocated */
> >> +     truncate_data_blocks_range(&dn, 1);
> >> +     f2fs_put_dnode(&dn);
> >> +
> >> +     return 0;
> >> +}
> >
> > --
> > 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

-- 
Jaegeuk Kim
Samsung



------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel