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

[Huajun Li]

Yes, will think over these cases carefully, and send out new version. thanks.

On Tue, Oct 29, 2013 at 9:06 AM, Jaegeuk Kim <jaegeuk.kim@xxxxxxxxxxx> wrote:
> Hi,
>
> 2013-10-29 (í), 01:20 +0800, Huajun Li:
>> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <jaegeuk.kim@xxxxxxxxxxx> wrote:
>> > Hi,
>> >
>> > 2013-10-26 (í), 00:01 +0800, Huajun Li:
>> >> From: Huajun Li <huajun.li@xxxxxxxxx>
>> >>
>> >> 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@xxxxxxxxx>
>> >> Signed-off-by: Haicheng Li <haicheng.li@xxxxxxxxxxxxxxx>
>> >> Signed-off-by: Weihong Xu <weihong.xu@xxxxxxxxx>
>> >> ---
>> >>  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@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Jaegeuk Kim
> Samsung
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/