Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758185Ab3J2PdO (ORCPT ); Tue, 29 Oct 2013 11:33:14 -0400 Received: from mail-vb0-f41.google.com ([209.85.212.41]:54391 "EHLO mail-vb0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757559Ab3J2PdK convert rfc822-to-8bit (ORCPT ); Tue, 29 Oct 2013 11:33:10 -0400 MIME-Version: 1.0 In-Reply-To: <1383008787.14041.8.camel@kjgkr> References: <1382716919-23345-1-git-send-email-huajun.li.lee@gmail.com> <1382716919-23345-5-git-send-email-huajun.li.lee@gmail.com> <1382964198.992.121.camel@kjgkr> <1383008787.14041.8.camel@kjgkr> Date: Tue, 29 Oct 2013 23:33:09 +0800 Message-ID: Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data From: Huajun Li To: "jaegeuk.kim" Cc: linux-f2fs-devel , linux-fsdevel , linux-kernel , Huajun Li , Haicheng Li , Weihong Xu Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7335 Lines: 220 Yes, will think over these cases carefully, and send out new version. thanks. On Tue, Oct 29, 2013 at 9:06 AM, Jaegeuk Kim wrote: > Hi, > > 2013-10-29 (화), 01:20 +0800, Huajun Li: >> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim wrote: >> > Hi, >> > >> > 2013-10-26 (토), 00:01 +0800, Huajun Li: >> >> From: Huajun Li >> >> >> >> 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 >> >> Signed-off-by: Haicheng Li >> >> Signed-off-by: Weihong Xu >> >> --- >> >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/