| 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
| ||
|
Message-ID: <20190723012721.GA60134@jaegeuk-macbookpro.roam.corp.google.com>
Date: Mon, 22 Jul 2019 18:27:21 -0700
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Chao Yu <yuchao0@...wei.com>
Cc: linux-kernel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH v2] f2fs: fix to read source block before
invalidating it
On 07/18, Chao Yu wrote:
> On 2019/7/18 12:00, Jaegeuk Kim wrote:
> > On 07/18, Chao Yu wrote:
> >> On 2019/7/18 11:12, Jaegeuk Kim wrote:
> >>> f2fs_allocate_data_block() invalidates old block address and enable new block
> >>> address. Then, if we try to read old block by f2fs_submit_page_bio(), it will
> >>> give WARN due to reading invalid blocks.
> >>>
> >>> Let's make the order sanely back.
> >>
> >> Hmm.. to avoid WARM, we may suffer one more memcpy, I suspect this can reduce
> >> online resize or foreground gc ioctl performance...
> >
> > I worried about performance tho, more concern came to me that there may exist a
> > chance that other thread can allocate and write something in old block address.
>
> Me too, however, previous invalid block address should be reused after a
> checkpoint, and checkpoint should have invalidated meta cache already, so there
> shouldn't be any race here.
I think SSR can reuse that before checkpoint.
>
> /*
> * invalidate intermediate page cache borrowed from meta inode
> * which are used for migration of encrypted inode's blocks.
> */
> if (f2fs_sb_has_encrypt(sbi))
> invalidate_mapping_pages(META_MAPPING(sbi),
> MAIN_BLKADDR(sbi), MAX_BLKADDR(sbi) - 1);
>
> Thanks,
>
> >
> >>
> >> Can we just relief to use DATA_GENERIC_ENHANCE_READ for this case...?
> >
> > We need to keep consistency for this api.
> >
> > Thanks,
> >
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
> >>
> >> Except performance, I'm okay with this change.
> >>
> >> Reviewed-by: Chao Yu <yuchao0@...wei.com>
> >>
> >> Thanks,
> >>
> >>> ---
> >>> v2:
> >>> I was fixing the comments. :)
> >>>
> >>> fs/f2fs/gc.c | 70 +++++++++++++++++++++++++---------------------------
> >>> 1 file changed, 34 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>> index 6691f526fa40..8974672db78f 100644
> >>> --- a/fs/f2fs/gc.c
> >>> +++ b/fs/f2fs/gc.c
> >>> @@ -796,6 +796,29 @@ static int move_data_block(struct inode *inode, block_t bidx,
> >>> if (lfs_mode)
> >>> down_write(&fio.sbi->io_order_lock);
> >>>
> >>> + mpage = f2fs_grab_cache_page(META_MAPPING(fio.sbi),
> >>> + fio.old_blkaddr, false);
> >>> + if (!mpage)
> >>> + goto up_out;
> >>> +
> >>> + fio.encrypted_page = mpage;
> >>> +
> >>> + /* read source block in mpage */
> >>> + if (!PageUptodate(mpage)) {
> >>> + err = f2fs_submit_page_bio(&fio);
> >>> + if (err) {
> >>> + f2fs_put_page(mpage, 1);
> >>> + goto up_out;
> >>> + }
> >>> + lock_page(mpage);
> >>> + if (unlikely(mpage->mapping != META_MAPPING(fio.sbi) ||
> >>> + !PageUptodate(mpage))) {
> >>> + err = -EIO;
> >>> + f2fs_put_page(mpage, 1);
> >>> + goto up_out;
> >>> + }
> >>> + }
> >>> +
> >>> f2fs_allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &newaddr,
> >>> &sum, CURSEG_COLD_DATA, NULL, false);
> >>>
> >>> @@ -803,44 +826,18 @@ static int move_data_block(struct inode *inode, block_t bidx,
> >>> newaddr, FGP_LOCK | FGP_CREAT, GFP_NOFS);
> >>> if (!fio.encrypted_page) {
> >>> err = -ENOMEM;
> >>> - goto recover_block;
> >>> - }
> >>> -
> >>> - mpage = f2fs_pagecache_get_page(META_MAPPING(fio.sbi),
> >>> - fio.old_blkaddr, FGP_LOCK, GFP_NOFS);
> >>> - if (mpage) {
> >>> - bool updated = false;
> >>> -
> >>> - if (PageUptodate(mpage)) {
> >>> - memcpy(page_address(fio.encrypted_page),
> >>> - page_address(mpage), PAGE_SIZE);
> >>> - updated = true;
> >>> - }
> >>> f2fs_put_page(mpage, 1);
> >>> - invalidate_mapping_pages(META_MAPPING(fio.sbi),
> >>> - fio.old_blkaddr, fio.old_blkaddr);
> >>> - if (updated)
> >>> - goto write_page;
> >>> - }
> >>> -
> >>> - err = f2fs_submit_page_bio(&fio);
> >>> - if (err)
> >>> - goto put_page_out;
> >>> -
> >>> - /* write page */
> >>> - lock_page(fio.encrypted_page);
> >>> -
> >>> - if (unlikely(fio.encrypted_page->mapping != META_MAPPING(fio.sbi))) {
> >>> - err = -EIO;
> >>> - goto put_page_out;
> >>> - }
> >>> - if (unlikely(!PageUptodate(fio.encrypted_page))) {
> >>> - err = -EIO;
> >>> - goto put_page_out;
> >>> + goto recover_block;
> >>> }
> >>>
> >>> -write_page:
> >>> + /* write target block */
> >>> f2fs_wait_on_page_writeback(fio.encrypted_page, DATA, true, true);
> >>> + memcpy(page_address(fio.encrypted_page),
> >>> + page_address(mpage), PAGE_SIZE);
> >>> + f2fs_put_page(mpage, 1);
> >>> + invalidate_mapping_pages(META_MAPPING(fio.sbi),
> >>> + fio.old_blkaddr, fio.old_blkaddr);
> >>> +
> >>> set_page_dirty(fio.encrypted_page);
> >>> if (clear_page_dirty_for_io(fio.encrypted_page))
> >>> dec_page_count(fio.sbi, F2FS_DIRTY_META);
> >>> @@ -871,11 +868,12 @@ static int move_data_block(struct inode *inode, block_t bidx,
> >>> put_page_out:
> >>> f2fs_put_page(fio.encrypted_page, 1);
> >>> recover_block:
> >>> - if (lfs_mode)
> >>> - up_write(&fio.sbi->io_order_lock);
> >>> if (err)
> >>> f2fs_do_replace_block(fio.sbi, &sum, newaddr, fio.old_blkaddr,
> >>> true, true);
> >>> +up_out:
> >>> + if (lfs_mode)
> >>> + up_write(&fio.sbi->io_order_lock);
> >>> put_out:
> >>> f2fs_put_dnode(&dn);
> >>> out:
> >>>
> >>
> >>
> >> _______________________________________________
> >> Linux-f2fs-devel mailing list
> >> Linux-f2fs-devel@...ts.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > .
> >
Powered by blists - more mailing lists