Re: [PATCH] f2fs: provide f2fs_balance_fs to __write_data_page for dentry pages

[Yunlong Song <yunlong.song@huawei.com>]

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.

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@kernel.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@huawei.com>
>>>      > Signed-off-by: Yunlong Song <yunlong.song@huawei.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