linux-kernel - Re: [f2fs-dev][PATCH V2 4/6] f2fs: Key functions to handle inline data

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Nov 2013 20:51:12 +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][PATCH V2 4/6] f2fs: Key functions to handle inline data

On Fri, Nov 15, 2013 at 3:49 PM, Jaegeuk Kim <jaegeuk.kim@...sung.com> wrote:
> Hi Huajun,
>
> [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;
>> +     block_t old_blk_addr, new_blk_addr;
>> +     struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>> +
>> +     f2fs_lock_op(sbi);
>> +     ipage = get_node_page(sbi, inode->i_ino);
>> +     if (IS_ERR(ipage))
>> +             return PTR_ERR(ipage);
>> +
>> +     /*
>> +      * i_addr[0] is not used for inline data,
>> +      * so reserving new block will not destroy inline data
>> +      */
>> +     set_new_dnode(&dn, inode, ipage, ipage, 0);
>> +     err = f2fs_reserve_block(&dn, 0);
>> +     if (err) {
>> +             f2fs_put_page(ipage, 1);
>> +             f2fs_unlock_op(sbi);
>> +             return err;
>> +     }
>> +
>> +     src_addr = inline_data_addr(ipage);
>> +     dst_addr = page_address(page);
>> +     zero_user_segment(page, 0, PAGE_CACHE_SIZE);
>> +
>> +     /* Copy the whole inline data block */
>> +     memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>> +
>> +     /* write data page to try to make data consistent */
>> +     old_blk_addr = dn.data_blkaddr;
>> +     set_page_writeback(page);
>> +     write_data_page(inode, page, &dn,
>> +                     old_blk_addr, &new_blk_addr);
>> +     update_extent_cache(new_blk_addr, &dn);
>> +     f2fs_wait_on_page_writeback(page, DATA, true);
>> +
>> +     /* clear inline data and flag after data writeback */
>> +     zero_user_segment(ipage, INLINE_DATA_OFFSET,
>> +                              INLINE_DATA_OFFSET + MAX_INLINE_DATA);
>> +     clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
>> +
>> +     sync_inode_page(&dn);
>> +     f2fs_put_page(ipage, 1);
>
> Again, it seems that you missed what I mentioned.
> If we write the inlined data block only, we cannot recover the data
> block after SPO.
> In order to avoid that, we should write its dnode block too by
> triggering sync_node_pages(ino) at this point as similar as fsync
> routine.
>
> Thanks,
>
> --
> Jaegeuk Kim
> Samsung
>

Hi Jaegeuk,
Previously, I refactored f2fs_sync_file() and tried to call it while
converting inline data, but find it is easily dead lock, so just write
data block here.
Then, how about following enhancement to this patch ?  it only copies
some codes to the end of  __f2fs_convert_inline_data(),  and keeps
others same as before.

-----------------------------------------------------------------------------
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index e9b33d7..6ceb2e6 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -10,6 +10,8 @@

 #include <linux/fs.h>
 #include <linux/f2fs_fs.h>
+#include <linux/writeback.h>
+#include <linux/blkdev.h>

 #include "f2fs.h"

@@ -71,6 +73,11 @@ static int __f2fs_convert_inline_data(struct inode
*inode, struct page *page)
     void *src_addr, *dst_addr;
     block_t old_blk_addr, new_blk_addr;
     struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
+    struct writeback_control wbc = {
+        .sync_mode = WB_SYNC_ALL,
+        .nr_to_write = LONG_MAX,
+        .for_reclaim = 0,
+    };

     f2fs_lock_op(sbi);
     ipage = get_node_page(sbi, inode->i_ino);
@@ -108,9 +115,20 @@ static int __f2fs_convert_inline_data(struct
inode *inode, struct page *page)
     zero_user_segment(ipage, INLINE_DATA_OFFSET,
                  INLINE_DATA_OFFSET + MAX_INLINE_DATA);
     clear_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
-
     sync_inode_page(&dn);
     f2fs_put_page(ipage, 1);
+
+    while (!sync_node_pages(sbi, inode->i_ino, &wbc)) {
+        mark_inode_dirty_sync(inode);
+        err = f2fs_write_inode(inode, NULL);
+        if (err)
+            goto out;
+    }
+    err = wait_on_node_pages_writeback(sbi, inode->i_ino);
+    if (err)
+        goto out;
+    err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
+out:
     f2fs_unlock_op(sbi);

     return err;
--
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/