| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 29 Oct 2013 23:33:09 +0800
From: Huajun Li <huajun.li.lee@...il.com>
To: "jaegeuk.kim" <jaegeuk.kim@...sung.com>
Cc: linux-f2fs-devel <linux-f2fs-devel@...ts.sourceforge.net>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Huajun Li <huajun.li@...el.com>,
Haicheng Li <haicheng.li@...ux.intel.com>,
Weihong Xu <weihong.xu@...el.com>
Subject: Re: [f2fs-dev 4/5] f2fs: Key functions to handle inline data
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@...sung.com> wrote:
> Hi,
>
> 2013-10-29 (화), 01:20 +0800, Huajun Li:
>> On Mon, Oct 28, 2013 at 8:43 PM, Jaegeuk Kim <jaegeuk.kim@...sung.com> wrote:
>> > Hi,
>> >
>> > 2013-10-26 (토), 00:01 +0800, Huajun Li:
>> >> From: Huajun Li <huajun.li@...el.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@...el.com>
>> >> Signed-off-by: Haicheng Li <haicheng.li@...ux.intel.com>
>> >> Signed-off-by: Weihong Xu <weihong.xu@...el.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@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists