| 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: <0351eaf0-4d5d-d424-ea67-a4aabbf69b2b@huawei.com>
Date: Fri, 4 Aug 2017 12:05:49 +0800
From: Yunlong Song <yunlong.song@...wei.com>
To: Jaegeuk Kim <jaegeuk@...nel.org>
CC: Yunlong Song <sylinux@....com>, chao <chao@...nel.org>,
yuchao0 <yuchao0@...wei.com>, miaoxie <miaoxie@...wei.com>,
"bintian.wang" <bintian.wang@...wei.com>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-f2fs-devel <linux-f2fs-devel@...ts.sourceforge.net>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] f2fs: provide f2fs_balance_fs to __write_data_page for
dentry pages
Chao has pointed out that, maybe writebacking dentry page can redirty
dnode page
which may be clean previously, so it is more safe to call
f2fs_balance_fs to make sure
dirty metadatas of filesystem will not exceed the threshold.
On 2017/8/4 11:34, Jaegeuk Kim wrote:
> On 08/04, Yunlong Song wrote:
>> Since __write_data_page will not do f2fs_balance_fs for dir inode, so there
>> is no lock.
>> - f2fs_balance_fs
>> - __write_data_page (dir inode)
>> - f2fs_balance_fs again? <- Can not happen!
>> And if let sync_dirty_inodes flush dentry page of inodeB, then inodeB will
>> sikp the
>> f2fs_balance_fs check due to the same reason above (it is dir inode).
> What I wanted to say was we cannot add f2fs_balance_fs() in my proposed approach
> due to livelock.
>
>> Besides, does any lock will lock the kernel writeback process? If not, when
>> normal
>> writeback of inodeB just happens before sync_dirty_inodes fetch it from the
>> dirty_list,
>> the normal writeback of indoeB will skip the f2fs_balance_fs check.
> Again, I'm asking why this normal writeback should call f2fs_balance_fs().
> We must flush all the dirty dentry pages in inodeB to finalize checkpoint.
> Who is able to make dirty dentry pages at this moment?
>
>> f2fs_trim(,sync)_fs normal dentry page writeback
>> of inodeB
>> -- write_checkpoint --f2fs_write_data_pages
>> --block_operations --f2fs_write_cache_pages
>> --SBI_BLOCK_OPS is set --__write_data_page
>> --sync_dirty_inodes --test SBI_BLOCK_OPS is set and skip
>> f2fs_balance_fs
>> --retry: write to reserved segments
>> inodeA <- list_first_entry(dirty_list)
>> filemap_fdatawrite(inodeA)
>> go to retry
>> --SBI_BLOCK_OPS is clear
>>
>> On 2017/8/4 9:57, Jaegeuk Kim wrote:
>>> On 08/02, Yunlong Song wrote:
>>>> Hi Jay,
>>>> The case is like this:
>>>> write_checkpoint: normal dentry
>>>> page writeback of inodeB：
>>>> -block_operations -f2fs_write_data_pages
>>>> -SBI_BLOCK_OPS is set -f2fs_write_cache_pages
>>>> -sync_dirty_inodes -__write_data_page
>>>> -retry: -test SBI_BLOCK_OPS is set and skip
>>>> f2fs_balance_fs
>>>> inodeA <- list_first_entry(dirty_list)
>>>> filemap_fdatawrite(inodeA)
>>>> goto retry
>>>> -SBI_BLOCK_OPS is clear
>>>>
>>>> write_checkpoint flow call sync_dirty_inodes to traversal the dirty inode
>>>> list and filemap_fdatawrite each inode,
>>>> during this period, if normal dentry_page writeback is processing inodeB,
>>>> and syc_dirty_inodes is processing
>>>> inodeA, then inodeB writeback flow will skip f2fs_balance_fs and may write
>>>> the dentry page to reserved segments.
>>> If there are not enough sections, all the possible system calls were already
>>> blocked by gc_mutex. So, it doesn't have to do f2fs_balance_fs(), and let
>>> sync_dirty_inodes() flush dentry pages of inodeB.
>>>
>>> Oh, does it make livelock?
>>>
>>> - f2fs_balance_fs
>>> - f2fs_gc
>>> - write_checkpoint
>>> - sync_dirty_inodes
>>> - filemap_fdatawrite(inodeB)
>>> - __write_data_page
>>> - f2fs_balance_fs again?
>>>
>>>> On 2017/8/2 2:22, Jaegeuk Kim wrote:
>>>>> On 08/01, Yunlong Song wrote:
>>>>>> Hi Jay,
>>>>>> The SBI_BLOCK_OPS can not cover all the case, once SBI_BLOCK_OPS is set,
>>>>>> all the normal writeback
>>>>>> (before the SBI_BLOCK_OPS is clear) of dentry pages which do not come from
>>>>>> write_checkpoint flow will
>>>>>> totally miss all the f2fs_balance_fs check.
>>>>> Why not? There must be no dirty dentry pages after sync_dirty_inodes() in that
>>>>> period which we grabbed the globla lock.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>> On 2017/7/31 0:05, Yunlong Song wrote:
>>>>>>> Hi Jay,
>>>>>>> Chao has pointed out one reason, besides, I have another reason: we
>>>>>>> should take care of writeback for f2fs_balance_fs carefully, because if
>>>>>>> some bugs cause reserved segments unlikely used, which means
>>>>>>> f2fs_balance_fs does not work or is skipped in some corner case that we
>>>>>>> have not noticed or found out yet, then the reserved segments may be
>>>>>>> continually used and even used up in the writeback process of dentry
>>>>>>> page, since current design believe in the f2fs_balance_fs in system
>>>>>>> call and has no check in denty page writeback. To avoid this, we can put
>>>>>>> a f2fs_balance_fs in the dentry page writeback process to give f2fs more
>>>>>>> robust in free segments reserved. This is worth, because free segments
>>>>>>> reserved are so important, if they are used up, f2fs will enter a
>>>>>>> totally wrong status and make a wrong image.
>>>>>>>
>>>>>>> On 07/30/2017 15:31, Jaegeuk Kim <mailto:jaegeuk@...nel.org> wrote:
>>>>>>>
>>>>>>> On 07/29, Yunlong Song wrote:
>>>>>>> > f2fs_balance_fs of dentry pages is skipped in __write_data_page
>>>>>>> due to deadlock
>>>>>>> > of gc_mutex in write_checkpoint flow. This patch enables
>>>>>>> f2fs_balance_fs for
>>>>>>> > normal dentry page writeback to ensure there are always enough
>>>>>>> free segments.
>>>>>>>
>>>>>>> Sorry, by the way, why do we need to do this? I subtly thought
>>>>>>> that dirty node
>>>>>>> pages can be produce by redirtied inodes from what we've not
>>>>>>> covered through
>>>>>>> filesystem calls. But, in dentry pages, we're controlling the
>>>>>>> number of dirty
>>>>>>> pages, and calling f2fs_balance_fs in each directory operations.
>>>>>>>
>>>>>>> Chao?
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> >
>>>>>>> > Reported-by: Chao Yu <yuchao0@...wei.com>
>>>>>>> > Signed-off-by: Yunlong Song <yunlong.song@...wei.com>
>>>>>>> > ---
>>>>>>> > fs/f2fs/checkpoint.c | 2 +-
>>>>>>> > fs/f2fs/data.c | 67
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++-------
>>>>>>> > fs/f2fs/f2fs.h | 1 +
>>>>>>> > 3 files changed, 61 insertions(+), 9 deletions(-)
>>>>>>> >
>>>>>>> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>>> > index 3c84a25..2882878 100644
>>>>>>> > --- a/fs/f2fs/checkpoint.c
>>>>>>> > +++ b/fs/f2fs/checkpoint.c
>>>>>>> > @@ -904,7 +904,7 @@ int sync_dirty_inodes(struct f2fs_sb_info
>>>>>>> *sbi, enum inode_type type)
>>>>>>> > if (inode) {
>>>>>>> > unsigned long cur_ino = inode->i_ino;
>>>>>>> >
>>>>>>> > - filemap_fdatawrite(inode->i_mapping);
>>>>>>> > + f2fs_filemap_fdatawrite(inode->i_mapping, is_dir);
>>>>>>> > iput(inode);
>>>>>>> > /* We need to give cpu to another writers. */
>>>>>>> > if (ino == cur_ino) {
>>>>>>> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>> > index aefc2a5..ed7efa5 100644
>>>>>>> > --- a/fs/f2fs/data.c
>>>>>>> > +++ b/fs/f2fs/data.c
>>>>>>> > @@ -1475,7 +1475,7 @@ int do_write_data_page(struct f2fs_io_info
>>>>>>> *fio)
>>>>>>> > }
>>>>>>> >
>>>>>>> > static int __write_data_page(struct page *page, bool *submitted,
>>>>>>> > - struct writeback_control *wbc)
>>>>>>> > + struct writeback_control *wbc, bool do_balance)
>>>>>>> > {
>>>>>>> > struct inode *inode = page->mapping->host;
>>>>>>> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>>>> > @@ -1578,7 +1578,7 @@ static int __write_data_page(struct page
>>>>>>> *page, bool *submitted,
>>>>>>> > }
>>>>>>> >
>>>>>>> > unlock_page(page);
>>>>>>> > - if (!S_ISDIR(inode->i_mode))
>>>>>>> > + if (do_balance)
>>>>>>> > f2fs_balance_fs(sbi, need_balance_fs);
>>>>>>> >
>>>>>>> > if (unlikely(f2fs_cp_error(sbi))) {
>>>>>>> > @@ -1602,7 +1602,7 @@ static int __write_data_page(struct page
>>>>>>> *page, bool *submitted,
>>>>>>> > static int f2fs_write_data_page(struct page *page,
>>>>>>> > struct writeback_control *wbc)
>>>>>>> > {
>>>>>>> > - return __write_data_page(page, NULL, wbc);
>>>>>>> > + return __write_data_page(page, NULL, wbc, true);
>>>>>>> > }
>>>>>>> >
>>>>>>> > /*
>>>>>>> > @@ -1611,7 +1611,7 @@ static int f2fs_write_data_page(struct
>>>>>>> page *page,
>>>>>>> > * warm/hot data page.
>>>>>>> > */
>>>>>>> > static int f2fs_write_cache_pages(struct address_space *mapping,
>>>>>>> > - struct writeback_control *wbc)
>>>>>>> > + struct writeback_control *wbc, bool
>>>>>>> do_balance)
>>>>>>> > {
>>>>>>> > int ret = 0;
>>>>>>> > int done = 0;
>>>>>>> > @@ -1701,7 +1701,7 @@ static int f2fs_write_cache_pages(struct
>>>>>>> address_space *mapping,
>>>>>>> > if (!clear_page_dirty_for_io(page))
>>>>>>> > goto continue_unlock;
>>>>>>> >
>>>>>>> > - ret = __write_data_page(page, &submitted, wbc);
>>>>>>> > + ret = __write_data_page(page, &submitted, wbc,
>>>>>>> do_balance);
>>>>>>> > if (unlikely(ret)) {
>>>>>>> > /*
>>>>>>> > * keep nr_to_write, since vfs uses this to
>>>>>>> > @@ -1756,8 +1756,8 @@ static int f2fs_write_cache_pages(struct
>>>>>>> address_space *mapping,
>>>>>>> > return ret;
>>>>>>> > }
>>>>>>> >
>>>>>>> > -static int f2fs_write_data_pages(struct address_space *mapping,
>>>>>>> > - struct writeback_control *wbc)
>>>>>>> > +static int _f2fs_write_data_pages(struct address_space *mapping,
>>>>>>> > + struct writeback_control *wbc, bool do_balance)
>>>>>>> > {
>>>>>>> > struct inode *inode = mapping->host;
>>>>>>> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>>>> > @@ -1794,7 +1794,7 @@ static int f2fs_write_data_pages(struct
>>>>>>> address_space *mapping,
>>>>>>> > goto skip_write;
>>>>>>> >
>>>>>>> > blk_start_plug(&plug);
>>>>>>> > - ret = f2fs_write_cache_pages(mapping, wbc);
>>>>>>> > + ret = f2fs_write_cache_pages(mapping, wbc, do_balance);
>>>>>>> > blk_finish_plug(&plug);
>>>>>>> >
>>>>>>> > if (wbc->sync_mode == WB_SYNC_ALL)
>>>>>>> > @@ -1813,6 +1813,57 @@ static int f2fs_write_data_pages(struct
>>>>>>> address_space *mapping,
>>>>>>> > return 0;
>>>>>>> > }
>>>>>>> >
>>>>>>> > +static int f2fs_write_data_pages(struct address_space *mapping,
>>>>>>> > + struct writeback_control *wbc)
>>>>>>> > +{
>>>>>>> > + return _f2fs_write_data_pages(mapping, wbc, true);
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > +/*
>>>>>>> > + * This function was copied from do_writepages from
>>>>>>> mm/page-writeback.c.
>>>>>>> > + * The major change is changing writepages to
>>>>>>> _f2fs_write_data_pages.
>>>>>>> > + */
>>>>>>> > +static int f2fs_do_writepages(struct address_space *mapping,
>>>>>>> > + struct writeback_control *wbc, bool is_dir)
>>>>>>> > +{
>>>>>>> > + int ret;
>>>>>>> > +
>>>>>>> > + if (wbc->nr_to_write <= 0)
>>>>>>> > + return 0;
>>>>>>> > + while (1) {
>>>>>>> > + ret = _f2fs_write_data_pages(mapping, wbc, !is_dir);
>>>>>>> > + if ((ret != -ENOMEM) || (wbc->sync_mode != WB_SYNC_ALL))
>>>>>>> > + break;
>>>>>>> > + cond_resched();
>>>>>>> > + congestion_wait(BLK_RW_ASYNC, HZ/50);
>>>>>>> > + }
>>>>>>> > + return ret;
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > +/*
>>>>>>> > + * This function was copied from __filemap_fdatawrite_range from
>>>>>>> > + * mm/filemap.c. The major change is changing do_writepages to
>>>>>>> > + * f2fs_do_writepages.
>>>>>>> > + */
>>>>>>> > +int f2fs_filemap_fdatawrite(struct address_space *mapping, bool
>>>>>>> is_dir)
>>>>>>> > +{
>>>>>>> > + int ret;
>>>>>>> > + struct writeback_control wbc = {
>>>>>>> > + .sync_mode = WB_SYNC_ALL,
>>>>>>> > + .nr_to_write = LONG_MAX,
>>>>>>> > + .range_start = 0,
>>>>>>> > + .range_end = LLONG_MAX,
>>>>>>> > + };
>>>>>>> > +
>>>>>>> > + if (!mapping_cap_writeback_dirty(mapping))
>>>>>>> > + return 0;
>>>>>>> > +
>>>>>>> > + wbc_attach_fdatawrite_inode(&wbc, mapping->host);
>>>>>>> > + ret = f2fs_do_writepages(mapping, &wbc, is_dir);
>>>>>>> > + wbc_detach_inode(&wbc);
>>>>>>> > + return ret;
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > static void f2fs_write_failed(struct address_space *mapping,
>>>>>>> loff_t to)
>>>>>>> > {
>>>>>>> > struct inode *inode = mapping->host;
>>>>>>> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>> > index 9280283..ea9bebb 100644
>>>>>>> > --- a/fs/f2fs/f2fs.h
>>>>>>> > +++ b/fs/f2fs/f2fs.h
>>>>>>> > @@ -2572,6 +2572,7 @@ void f2fs_invalidate_page(struct page
>>>>>>> *page, unsigned int offset,
>>>>>>> > int f2fs_migrate_page(struct address_space *mapping, struct
>>>>>>> page *newpage,
>>>>>>> > struct page *page, enum migrate_mode mode);
>>>>>>> > #endif
>>>>>>> > +int f2fs_filemap_fdatawrite(struct address_space *mapping, bool
>>>>>>> is_dir);
>>>>>>> >
>>>>>>> > /*
>>>>>>> > * gc.c
>>>>>>> > --
>>>>>>> > 1.8.5.2
>>>>>>>
>>>>>> --
>>>>>> Thanks,
>>>>>> Yunlong Song
>>>>>>
>>>>> .
>>>>>
>>>> --
>>>> Thanks,
>>>> Yunlong Song
>>>>
>>> .
>>>
>> --
>> Thanks,
>> Yunlong Song
>>
> .
>
--
Thanks,
Yunlong Song
Powered by blists - more mailing lists