Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2024-03-07 Thread Chao Yu

On 2020/6/20 6:47, Jaegeuk Kim wrote:

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3268f8dd59bb..1862073b96d2 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1250,6 +1250,7 @@ static int __clone_blkaddrs(struct inode *src_inode, 
struct inode *dst_inode,
f2fs_put_page(psrc, 1);
return PTR_ERR(pdst);
}
+   f2fs_wait_on_page_writeback(pdst, DATA, true, true);


Do you mean pdst can be under writeback?


Jaegeuk,

Look back this again, is it possible that pdst is writebacking
in below race condition? or am I missing something?

Thread AGC Thread
- f2fs_move_file_range
 - filemap_write_and_wait_range(dst)
- gc_data_segment
 - f2fs_down_write(dst)
 - move_data_page
  - set_page_writeback(dst_page)
  - f2fs_submit_page_write
 - f2fs_up_write(dst)
 - f2fs_down_write(dst)
 - __exchange_data_block
  - __clone_blkaddrs
   - f2fs_get_new_data_page
   - memcpy_page

Thanks,


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-27 Thread Chao Yu
On 2020/6/24 23:55, Jaegeuk Kim wrote:
> On 06/22, Chao Yu wrote:
>> On 2020/6/22 0:38, Jaegeuk Kim wrote:
>>> On 06/20, Chao Yu wrote:
 On 2020/6/20 6:47, Jaegeuk Kim wrote:
> On 06/19, Chao Yu wrote:
>> On 2020/6/19 13:49, Jaegeuk Kim wrote:
>>> On 06/19, Chao Yu wrote:
 Hi Jaegeuk,

 On 2020/6/19 7:59, Jaegeuk Kim wrote:
> Hi Chao,
>
> On 06/18, Chao Yu wrote:
>> to make page content stable for special device like raid.
>
> Could you elaborate the problem a bit?

 Some devices like raid5 wants page content to be stable, because
 it will calculate parity info based page content, if page is not
 stable, parity info could be corrupted, result in data inconsistency
 in stripe.
>>>
>>> I don't get the point, since those pages are brand new pages which were 
>>> not
>>> modified before. If it's on writeback, we should not modify them 
>>> regardless
>>> of whatever raid configuration. For example, f2fs_new_node_page() waits 
>>> for
>>> writeback. Am I missing something?
>>
>> I think we should use f2fs_bug_on(, PageWriteback()) rather than
>> f2fs_wait_on_page_writeback() for brand new page which is allocated just 
>> now.
>> For other paths, we can keep rule that waiting for writeback before 
>> updating.
>>
>> How do you think?
>>
>> Thanks,
>>
>>>

 Thanks,

>
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/dir.c  |  2 ++
>>  fs/f2fs/extent_cache.c | 18 +-
>>  fs/f2fs/f2fs.h |  2 +-
>>  fs/f2fs/file.c |  1 +
>>  fs/f2fs/inline.c   |  2 ++
>>  fs/f2fs/inode.c|  3 +--
>>  6 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index d35976785e8c..91e86747a604 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>>  if (IS_ERR(dentry_page))
>>  return PTR_ERR(dentry_page);
>>  
>> +f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
>> +
>>  dentry_blk = page_address(dentry_page);
>>  
>>  make_dentry_ptr_block(NULL, , dentry_blk);
>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>> index e60078460ad1..686c68b98610 100644
>> --- a/fs/f2fs/extent_cache.c
>> +++ b/fs/f2fs/extent_cache.c
>> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
>> extent_tree *et,
>>  }
>>  
>>  /* return true, if inode page is changed */
>> -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
>> f2fs_extent *i_ext)
>> +static void __f2fs_init_extent_tree(struct inode *inode, struct 
>> page *ipage)
>>  {
>>  struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> +struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
>> NULL;
>>  struct extent_tree *et;
>>  struct extent_node *en;
>>  struct extent_info ei;
>> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct 
>> inode *inode, struct f2fs_extent *i_e
>>  if (!f2fs_may_extent_tree(inode)) {
>>  /* drop largest extent */
>>  if (i_ext && i_ext->len) {
>> +f2fs_wait_on_page_writeback(ipage, NODE, true, 
>> true);
>>  i_ext->len = 0;
>> -return true;
>> +set_page_dirty(ipage);
>> +return;
>>  }
>> -return false;
>> +return;
>>  }
>>  
>>  et = __grab_extent_tree(inode);
>>  
>>  if (!i_ext || !i_ext->len)
>> -return false;
>> +return;
>>  
>>  get_extent_info(, i_ext);
>>  
>> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct 
>> inode *inode, struct f2fs_extent *i_e
>>  }
>>  out:
>>  write_unlock(>lock);
>> -return false;
>>  }
>>  
>> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
>> *i_ext)
>> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>>  {
>> -bool ret =  __f2fs_init_extent_tree(inode, i_ext);
>> +__f2fs_init_extent_tree(inode, ipage);
>>  
>>  if (!F2FS_I(inode)->extent_tree)
>>  set_inode_flag(inode, 

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-24 Thread Jaegeuk Kim
On 06/22, Chao Yu wrote:
> On 2020/6/22 0:38, Jaegeuk Kim wrote:
> > On 06/20, Chao Yu wrote:
> >> On 2020/6/20 6:47, Jaegeuk Kim wrote:
> >>> On 06/19, Chao Yu wrote:
>  On 2020/6/19 13:49, Jaegeuk Kim wrote:
> > On 06/19, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2020/6/19 7:59, Jaegeuk Kim wrote:
> >>> Hi Chao,
> >>>
> >>> On 06/18, Chao Yu wrote:
>  to make page content stable for special device like raid.
> >>>
> >>> Could you elaborate the problem a bit?
> >>
> >> Some devices like raid5 wants page content to be stable, because
> >> it will calculate parity info based page content, if page is not
> >> stable, parity info could be corrupted, result in data inconsistency
> >> in stripe.
> >
> > I don't get the point, since those pages are brand new pages which were 
> > not
> > modified before. If it's on writeback, we should not modify them 
> > regardless
> > of whatever raid configuration. For example, f2fs_new_node_page() waits 
> > for
> > writeback. Am I missing something?
> 
>  I think we should use f2fs_bug_on(, PageWriteback()) rather than
>  f2fs_wait_on_page_writeback() for brand new page which is allocated just 
>  now.
>  For other paths, we can keep rule that waiting for writeback before 
>  updating.
> 
>  How do you think?
> 
>  Thanks,
> 
> >
> >>
> >> Thanks,
> >>
> >>>
> 
>  Signed-off-by: Chao Yu 
>  ---
>   fs/f2fs/dir.c  |  2 ++
>   fs/f2fs/extent_cache.c | 18 +-
>   fs/f2fs/f2fs.h |  2 +-
>   fs/f2fs/file.c |  1 +
>   fs/f2fs/inline.c   |  2 ++
>   fs/f2fs/inode.c|  3 +--
>   6 files changed, 16 insertions(+), 12 deletions(-)
> 
>  diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>  index d35976785e8c..91e86747a604 100644
>  --- a/fs/f2fs/dir.c
>  +++ b/fs/f2fs/dir.c
>  @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>   if (IS_ERR(dentry_page))
>   return PTR_ERR(dentry_page);
>   
>  +f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
>  +
>   dentry_blk = page_address(dentry_page);
>   
>   make_dentry_ptr_block(NULL, , dentry_blk);
>  diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>  index e60078460ad1..686c68b98610 100644
>  --- a/fs/f2fs/extent_cache.c
>  +++ b/fs/f2fs/extent_cache.c
>  @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
>  extent_tree *et,
>   }
>   
>   /* return true, if inode page is changed */
>  -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
>  f2fs_extent *i_ext)
>  +static void __f2fs_init_extent_tree(struct inode *inode, struct 
>  page *ipage)
>   {
>   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  +struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
>  NULL;
>   struct extent_tree *et;
>   struct extent_node *en;
>   struct extent_info ei;
>  @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct 
>  inode *inode, struct f2fs_extent *i_e
>   if (!f2fs_may_extent_tree(inode)) {
>   /* drop largest extent */
>   if (i_ext && i_ext->len) {
>  +f2fs_wait_on_page_writeback(ipage, NODE, true, 
>  true);
>   i_ext->len = 0;
>  -return true;
>  +set_page_dirty(ipage);
>  +return;
>   }
>  -return false;
>  +return;
>   }
>   
>   et = __grab_extent_tree(inode);
>   
>   if (!i_ext || !i_ext->len)
>  -return false;
>  +return;
>   
>   get_extent_info(, i_ext);
>   
>  @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct 
>  inode *inode, struct f2fs_extent *i_e
>   }
>   out:
>   write_unlock(>lock);
>  -return false;
>   }
>   
>  -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
>  *i_ext)
>  +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>   {
>  -bool ret =  __f2fs_init_extent_tree(inode, i_ext);
>  +__f2fs_init_extent_tree(inode, ipage);
>   
>   if (!F2FS_I(inode)->extent_tree)
>   set_inode_flag(inode, FI_NO_EXTENT);
>  -
>  -

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-21 Thread Chao Yu
On 2020/6/22 0:38, Jaegeuk Kim wrote:
> On 06/20, Chao Yu wrote:
>> On 2020/6/20 6:47, Jaegeuk Kim wrote:
>>> On 06/19, Chao Yu wrote:
 On 2020/6/19 13:49, Jaegeuk Kim wrote:
> On 06/19, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2020/6/19 7:59, Jaegeuk Kim wrote:
>>> Hi Chao,
>>>
>>> On 06/18, Chao Yu wrote:
 to make page content stable for special device like raid.
>>>
>>> Could you elaborate the problem a bit?
>>
>> Some devices like raid5 wants page content to be stable, because
>> it will calculate parity info based page content, if page is not
>> stable, parity info could be corrupted, result in data inconsistency
>> in stripe.
>
> I don't get the point, since those pages are brand new pages which were 
> not
> modified before. If it's on writeback, we should not modify them 
> regardless
> of whatever raid configuration. For example, f2fs_new_node_page() waits 
> for
> writeback. Am I missing something?

 I think we should use f2fs_bug_on(, PageWriteback()) rather than
 f2fs_wait_on_page_writeback() for brand new page which is allocated just 
 now.
 For other paths, we can keep rule that waiting for writeback before 
 updating.

 How do you think?

 Thanks,

>
>>
>> Thanks,
>>
>>>

 Signed-off-by: Chao Yu 
 ---
  fs/f2fs/dir.c  |  2 ++
  fs/f2fs/extent_cache.c | 18 +-
  fs/f2fs/f2fs.h |  2 +-
  fs/f2fs/file.c |  1 +
  fs/f2fs/inline.c   |  2 ++
  fs/f2fs/inode.c|  3 +--
  6 files changed, 16 insertions(+), 12 deletions(-)

 diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
 index d35976785e8c..91e86747a604 100644
 --- a/fs/f2fs/dir.c
 +++ b/fs/f2fs/dir.c
 @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
if (IS_ERR(dentry_page))
return PTR_ERR(dentry_page);
  
 +  f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
 +
dentry_blk = page_address(dentry_page);
  
make_dentry_ptr_block(NULL, , dentry_blk);
 diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
 index e60078460ad1..686c68b98610 100644
 --- a/fs/f2fs/extent_cache.c
 +++ b/fs/f2fs/extent_cache.c
 @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
 extent_tree *et,
  }
  
  /* return true, if inode page is changed */
 -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
 f2fs_extent *i_ext)
 +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
 *ipage)
  {
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 +  struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
 NULL;
struct extent_tree *et;
struct extent_node *en;
struct extent_info ei;
 @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
 *inode, struct f2fs_extent *i_e
if (!f2fs_may_extent_tree(inode)) {
/* drop largest extent */
if (i_ext && i_ext->len) {
 +  f2fs_wait_on_page_writeback(ipage, NODE, true, 
 true);
i_ext->len = 0;
 -  return true;
 +  set_page_dirty(ipage);
 +  return;
}
 -  return false;
 +  return;
}
  
et = __grab_extent_tree(inode);
  
if (!i_ext || !i_ext->len)
 -  return false;
 +  return;
  
get_extent_info(, i_ext);
  
 @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
 *inode, struct f2fs_extent *i_e
}
  out:
write_unlock(>lock);
 -  return false;
  }
  
 -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
 *i_ext)
 +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
  {
 -  bool ret =  __f2fs_init_extent_tree(inode, i_ext);
 +  __f2fs_init_extent_tree(inode, ipage);
  
if (!F2FS_I(inode)->extent_tree)
set_inode_flag(inode, FI_NO_EXTENT);
 -
 -  return ret;
  }
  
  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t 
 pgofs,
 diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
 index 

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-21 Thread Jaegeuk Kim
On 06/20, Chao Yu wrote:
> On 2020/6/20 6:47, Jaegeuk Kim wrote:
> > On 06/19, Chao Yu wrote:
> >> On 2020/6/19 13:49, Jaegeuk Kim wrote:
> >>> On 06/19, Chao Yu wrote:
>  Hi Jaegeuk,
> 
>  On 2020/6/19 7:59, Jaegeuk Kim wrote:
> > Hi Chao,
> >
> > On 06/18, Chao Yu wrote:
> >> to make page content stable for special device like raid.
> >
> > Could you elaborate the problem a bit?
> 
>  Some devices like raid5 wants page content to be stable, because
>  it will calculate parity info based page content, if page is not
>  stable, parity info could be corrupted, result in data inconsistency
>  in stripe.
> >>>
> >>> I don't get the point, since those pages are brand new pages which were 
> >>> not
> >>> modified before. If it's on writeback, we should not modify them 
> >>> regardless
> >>> of whatever raid configuration. For example, f2fs_new_node_page() waits 
> >>> for
> >>> writeback. Am I missing something?
> >>
> >> I think we should use f2fs_bug_on(, PageWriteback()) rather than
> >> f2fs_wait_on_page_writeback() for brand new page which is allocated just 
> >> now.
> >> For other paths, we can keep rule that waiting for writeback before 
> >> updating.
> >>
> >> How do you think?
> >>
> >> Thanks,
> >>
> >>>
> 
>  Thanks,
> 
> >
> >>
> >> Signed-off-by: Chao Yu 
> >> ---
> >>  fs/f2fs/dir.c  |  2 ++
> >>  fs/f2fs/extent_cache.c | 18 +-
> >>  fs/f2fs/f2fs.h |  2 +-
> >>  fs/f2fs/file.c |  1 +
> >>  fs/f2fs/inline.c   |  2 ++
> >>  fs/f2fs/inode.c|  3 +--
> >>  6 files changed, 16 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> >> index d35976785e8c..91e86747a604 100644
> >> --- a/fs/f2fs/dir.c
> >> +++ b/fs/f2fs/dir.c
> >> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
> >>if (IS_ERR(dentry_page))
> >>return PTR_ERR(dentry_page);
> >>  
> >> +  f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
> >> +
> >>dentry_blk = page_address(dentry_page);
> >>  
> >>make_dentry_ptr_block(NULL, , dentry_blk);
> >> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >> index e60078460ad1..686c68b98610 100644
> >> --- a/fs/f2fs/extent_cache.c
> >> +++ b/fs/f2fs/extent_cache.c
> >> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
> >> extent_tree *et,
> >>  }
> >>  
> >>  /* return true, if inode page is changed */
> >> -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
> >> f2fs_extent *i_ext)
> >> +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
> >> *ipage)
> >>  {
> >>struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >> +  struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
> >> NULL;
> >>struct extent_tree *et;
> >>struct extent_node *en;
> >>struct extent_info ei;
> >> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
> >> *inode, struct f2fs_extent *i_e
> >>if (!f2fs_may_extent_tree(inode)) {
> >>/* drop largest extent */
> >>if (i_ext && i_ext->len) {
> >> +  f2fs_wait_on_page_writeback(ipage, NODE, true, 
> >> true);
> >>i_ext->len = 0;
> >> -  return true;
> >> +  set_page_dirty(ipage);
> >> +  return;
> >>}
> >> -  return false;
> >> +  return;
> >>}
> >>  
> >>et = __grab_extent_tree(inode);
> >>  
> >>if (!i_ext || !i_ext->len)
> >> -  return false;
> >> +  return;
> >>  
> >>get_extent_info(, i_ext);
> >>  
> >> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
> >> *inode, struct f2fs_extent *i_e
> >>}
> >>  out:
> >>write_unlock(>lock);
> >> -  return false;
> >>  }
> >>  
> >> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
> >> *i_ext)
> >> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
> >>  {
> >> -  bool ret =  __f2fs_init_extent_tree(inode, i_ext);
> >> +  __f2fs_init_extent_tree(inode, ipage);
> >>  
> >>if (!F2FS_I(inode)->extent_tree)
> >>set_inode_flag(inode, FI_NO_EXTENT);
> >> -
> >> -  return ret;
> >>  }
> >>  
> >>  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t 
> >> pgofs,
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index b35a50f4953c..326c12fa0da5 100644
> >> --- a/fs/f2fs/f2fs.h

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-19 Thread Chao Yu
On 2020/6/20 6:47, Jaegeuk Kim wrote:
> On 06/19, Chao Yu wrote:
>> On 2020/6/19 13:49, Jaegeuk Kim wrote:
>>> On 06/19, Chao Yu wrote:
 Hi Jaegeuk,

 On 2020/6/19 7:59, Jaegeuk Kim wrote:
> Hi Chao,
>
> On 06/18, Chao Yu wrote:
>> to make page content stable for special device like raid.
>
> Could you elaborate the problem a bit?

 Some devices like raid5 wants page content to be stable, because
 it will calculate parity info based page content, if page is not
 stable, parity info could be corrupted, result in data inconsistency
 in stripe.
>>>
>>> I don't get the point, since those pages are brand new pages which were not
>>> modified before. If it's on writeback, we should not modify them regardless
>>> of whatever raid configuration. For example, f2fs_new_node_page() waits for
>>> writeback. Am I missing something?
>>
>> I think we should use f2fs_bug_on(, PageWriteback()) rather than
>> f2fs_wait_on_page_writeback() for brand new page which is allocated just now.
>> For other paths, we can keep rule that waiting for writeback before updating.
>>
>> How do you think?
>>
>> Thanks,
>>
>>>

 Thanks,

>
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/dir.c  |  2 ++
>>  fs/f2fs/extent_cache.c | 18 +-
>>  fs/f2fs/f2fs.h |  2 +-
>>  fs/f2fs/file.c |  1 +
>>  fs/f2fs/inline.c   |  2 ++
>>  fs/f2fs/inode.c|  3 +--
>>  6 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index d35976785e8c..91e86747a604 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>>  if (IS_ERR(dentry_page))
>>  return PTR_ERR(dentry_page);
>>  
>> +f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
>> +
>>  dentry_blk = page_address(dentry_page);
>>  
>>  make_dentry_ptr_block(NULL, , dentry_blk);
>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>> index e60078460ad1..686c68b98610 100644
>> --- a/fs/f2fs/extent_cache.c
>> +++ b/fs/f2fs/extent_cache.c
>> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
>> extent_tree *et,
>>  }
>>  
>>  /* return true, if inode page is changed */
>> -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
>> f2fs_extent *i_ext)
>> +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
>> *ipage)
>>  {
>>  struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> +struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
>> NULL;
>>  struct extent_tree *et;
>>  struct extent_node *en;
>>  struct extent_info ei;
>> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
>> *inode, struct f2fs_extent *i_e
>>  if (!f2fs_may_extent_tree(inode)) {
>>  /* drop largest extent */
>>  if (i_ext && i_ext->len) {
>> +f2fs_wait_on_page_writeback(ipage, NODE, true, 
>> true);
>>  i_ext->len = 0;
>> -return true;
>> +set_page_dirty(ipage);
>> +return;
>>  }
>> -return false;
>> +return;
>>  }
>>  
>>  et = __grab_extent_tree(inode);
>>  
>>  if (!i_ext || !i_ext->len)
>> -return false;
>> +return;
>>  
>>  get_extent_info(, i_ext);
>>  
>> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
>> *inode, struct f2fs_extent *i_e
>>  }
>>  out:
>>  write_unlock(>lock);
>> -return false;
>>  }
>>  
>> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
>> *i_ext)
>> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>>  {
>> -bool ret =  __f2fs_init_extent_tree(inode, i_ext);
>> +__f2fs_init_extent_tree(inode, ipage);
>>  
>>  if (!F2FS_I(inode)->extent_tree)
>>  set_inode_flag(inode, FI_NO_EXTENT);
>> -
>> -return ret;
>>  }
>>  
>>  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index b35a50f4953c..326c12fa0da5 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
>> rb_root_cached *root,
>>  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
>>   

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-19 Thread Jaegeuk Kim
On 06/19, Chao Yu wrote:
> On 2020/6/19 13:49, Jaegeuk Kim wrote:
> > On 06/19, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2020/6/19 7:59, Jaegeuk Kim wrote:
> >>> Hi Chao,
> >>>
> >>> On 06/18, Chao Yu wrote:
>  to make page content stable for special device like raid.
> >>>
> >>> Could you elaborate the problem a bit?
> >>
> >> Some devices like raid5 wants page content to be stable, because
> >> it will calculate parity info based page content, if page is not
> >> stable, parity info could be corrupted, result in data inconsistency
> >> in stripe.
> > 
> > I don't get the point, since those pages are brand new pages which were not
> > modified before. If it's on writeback, we should not modify them regardless
> > of whatever raid configuration. For example, f2fs_new_node_page() waits for
> > writeback. Am I missing something?
> 
> I think we should use f2fs_bug_on(, PageWriteback()) rather than
> f2fs_wait_on_page_writeback() for brand new page which is allocated just now.
> For other paths, we can keep rule that waiting for writeback before updating.
> 
> How do you think?
> 
> Thanks,
> 
> > 
> >>
> >> Thanks,
> >>
> >>>
> 
>  Signed-off-by: Chao Yu 
>  ---
>   fs/f2fs/dir.c  |  2 ++
>   fs/f2fs/extent_cache.c | 18 +-
>   fs/f2fs/f2fs.h |  2 +-
>   fs/f2fs/file.c |  1 +
>   fs/f2fs/inline.c   |  2 ++
>   fs/f2fs/inode.c|  3 +--
>   6 files changed, 16 insertions(+), 12 deletions(-)
> 
>  diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>  index d35976785e8c..91e86747a604 100644
>  --- a/fs/f2fs/dir.c
>  +++ b/fs/f2fs/dir.c
>  @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>   if (IS_ERR(dentry_page))
>   return PTR_ERR(dentry_page);
>   
>  +f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
>  +
>   dentry_blk = page_address(dentry_page);
>   
>   make_dentry_ptr_block(NULL, , dentry_blk);
>  diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>  index e60078460ad1..686c68b98610 100644
>  --- a/fs/f2fs/extent_cache.c
>  +++ b/fs/f2fs/extent_cache.c
>  @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct 
>  extent_tree *et,
>   }
>   
>   /* return true, if inode page is changed */
>  -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
>  f2fs_extent *i_ext)
>  +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
>  *ipage)
>   {
>   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  +struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : 
>  NULL;
>   struct extent_tree *et;
>   struct extent_node *en;
>   struct extent_info ei;
>  @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
>  *inode, struct f2fs_extent *i_e
>   if (!f2fs_may_extent_tree(inode)) {
>   /* drop largest extent */
>   if (i_ext && i_ext->len) {
>  +f2fs_wait_on_page_writeback(ipage, NODE, true, 
>  true);
>   i_ext->len = 0;
>  -return true;
>  +set_page_dirty(ipage);
>  +return;
>   }
>  -return false;
>  +return;
>   }
>   
>   et = __grab_extent_tree(inode);
>   
>   if (!i_ext || !i_ext->len)
>  -return false;
>  +return;
>   
>   get_extent_info(, i_ext);
>   
>  @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
>  *inode, struct f2fs_extent *i_e
>   }
>   out:
>   write_unlock(>lock);
>  -return false;
>   }
>   
>  -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
>  *i_ext)
>  +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>   {
>  -bool ret =  __f2fs_init_extent_tree(inode, i_ext);
>  +__f2fs_init_extent_tree(inode, ipage);
>   
>   if (!F2FS_I(inode)->extent_tree)
>   set_inode_flag(inode, FI_NO_EXTENT);
>  -
>  -return ret;
>   }
>   
>   static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
>  diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>  index b35a50f4953c..326c12fa0da5 100644
>  --- a/fs/f2fs/f2fs.h
>  +++ b/fs/f2fs/f2fs.h
>  @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
>  rb_root_cached *root,
>   bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
>   struct rb_root_cached 
>  *root);
> 

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-19 Thread Chao Yu
On 2020/6/19 13:49, Jaegeuk Kim wrote:
> On 06/19, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2020/6/19 7:59, Jaegeuk Kim wrote:
>>> Hi Chao,
>>>
>>> On 06/18, Chao Yu wrote:
 to make page content stable for special device like raid.
>>>
>>> Could you elaborate the problem a bit?
>>
>> Some devices like raid5 wants page content to be stable, because
>> it will calculate parity info based page content, if page is not
>> stable, parity info could be corrupted, result in data inconsistency
>> in stripe.
> 
> I don't get the point, since those pages are brand new pages which were not
> modified before. If it's on writeback, we should not modify them regardless
> of whatever raid configuration. For example, f2fs_new_node_page() waits for
> writeback. Am I missing something?

I think we should use f2fs_bug_on(, PageWriteback()) rather than
f2fs_wait_on_page_writeback() for brand new page which is allocated just now.
For other paths, we can keep rule that waiting for writeback before updating.

How do you think?

Thanks,

> 
>>
>> Thanks,
>>
>>>

 Signed-off-by: Chao Yu 
 ---
  fs/f2fs/dir.c  |  2 ++
  fs/f2fs/extent_cache.c | 18 +-
  fs/f2fs/f2fs.h |  2 +-
  fs/f2fs/file.c |  1 +
  fs/f2fs/inline.c   |  2 ++
  fs/f2fs/inode.c|  3 +--
  6 files changed, 16 insertions(+), 12 deletions(-)

 diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
 index d35976785e8c..91e86747a604 100644
 --- a/fs/f2fs/dir.c
 +++ b/fs/f2fs/dir.c
 @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
if (IS_ERR(dentry_page))
return PTR_ERR(dentry_page);
  
 +  f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
 +
dentry_blk = page_address(dentry_page);
  
make_dentry_ptr_block(NULL, , dentry_blk);
 diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
 index e60078460ad1..686c68b98610 100644
 --- a/fs/f2fs/extent_cache.c
 +++ b/fs/f2fs/extent_cache.c
 @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct extent_tree 
 *et,
  }
  
  /* return true, if inode page is changed */
 -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
 f2fs_extent *i_ext)
 +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
 *ipage)
  {
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 +  struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : NULL;
struct extent_tree *et;
struct extent_node *en;
struct extent_info ei;
 @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
 *inode, struct f2fs_extent *i_e
if (!f2fs_may_extent_tree(inode)) {
/* drop largest extent */
if (i_ext && i_ext->len) {
 +  f2fs_wait_on_page_writeback(ipage, NODE, true, true);
i_ext->len = 0;
 -  return true;
 +  set_page_dirty(ipage);
 +  return;
}
 -  return false;
 +  return;
}
  
et = __grab_extent_tree(inode);
  
if (!i_ext || !i_ext->len)
 -  return false;
 +  return;
  
get_extent_info(, i_ext);
  
 @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
 *inode, struct f2fs_extent *i_e
}
  out:
write_unlock(>lock);
 -  return false;
  }
  
 -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext)
 +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
  {
 -  bool ret =  __f2fs_init_extent_tree(inode, i_ext);
 +  __f2fs_init_extent_tree(inode, ipage);
  
if (!F2FS_I(inode)->extent_tree)
set_inode_flag(inode, FI_NO_EXTENT);
 -
 -  return ret;
  }
  
  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
 diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
 index b35a50f4953c..326c12fa0da5 100644
 --- a/fs/f2fs/f2fs.h
 +++ b/fs/f2fs/f2fs.h
 @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
 rb_root_cached *root,
  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
struct rb_root_cached *root);
  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int 
 nr_shrink);
 -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
 *i_ext);
 +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage);
  void f2fs_drop_extent_tree(struct inode *inode);
  unsigned int f2fs_destroy_extent_node(struct inode *inode);
  void f2fs_destroy_extent_tree(struct inode *inode);
 diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
 index 3268f8dd59bb..1862073b96d2 100644
 

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-18 Thread Jaegeuk Kim
On 06/19, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2020/6/19 7:59, Jaegeuk Kim wrote:
> > Hi Chao,
> > 
> > On 06/18, Chao Yu wrote:
> >> to make page content stable for special device like raid.
> > 
> > Could you elaborate the problem a bit?
> 
> Some devices like raid5 wants page content to be stable, because
> it will calculate parity info based page content, if page is not
> stable, parity info could be corrupted, result in data inconsistency
> in stripe.

I don't get the point, since those pages are brand new pages which were not
modified before. If it's on writeback, we should not modify them regardless
of whatever raid configuration. For example, f2fs_new_node_page() waits for
writeback. Am I missing something?

> 
> Thanks,
> 
> > 
> >>
> >> Signed-off-by: Chao Yu 
> >> ---
> >>  fs/f2fs/dir.c  |  2 ++
> >>  fs/f2fs/extent_cache.c | 18 +-
> >>  fs/f2fs/f2fs.h |  2 +-
> >>  fs/f2fs/file.c |  1 +
> >>  fs/f2fs/inline.c   |  2 ++
> >>  fs/f2fs/inode.c|  3 +--
> >>  6 files changed, 16 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> >> index d35976785e8c..91e86747a604 100644
> >> --- a/fs/f2fs/dir.c
> >> +++ b/fs/f2fs/dir.c
> >> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
> >>if (IS_ERR(dentry_page))
> >>return PTR_ERR(dentry_page);
> >>  
> >> +  f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
> >> +
> >>dentry_blk = page_address(dentry_page);
> >>  
> >>make_dentry_ptr_block(NULL, , dentry_blk);
> >> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >> index e60078460ad1..686c68b98610 100644
> >> --- a/fs/f2fs/extent_cache.c
> >> +++ b/fs/f2fs/extent_cache.c
> >> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct extent_tree 
> >> *et,
> >>  }
> >>  
> >>  /* return true, if inode page is changed */
> >> -static bool __f2fs_init_extent_tree(struct inode *inode, struct 
> >> f2fs_extent *i_ext)
> >> +static void __f2fs_init_extent_tree(struct inode *inode, struct page 
> >> *ipage)
> >>  {
> >>struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >> +  struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : NULL;
> >>struct extent_tree *et;
> >>struct extent_node *en;
> >>struct extent_info ei;
> >> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
> >> *inode, struct f2fs_extent *i_e
> >>if (!f2fs_may_extent_tree(inode)) {
> >>/* drop largest extent */
> >>if (i_ext && i_ext->len) {
> >> +  f2fs_wait_on_page_writeback(ipage, NODE, true, true);
> >>i_ext->len = 0;
> >> -  return true;
> >> +  set_page_dirty(ipage);
> >> +  return;
> >>}
> >> -  return false;
> >> +  return;
> >>}
> >>  
> >>et = __grab_extent_tree(inode);
> >>  
> >>if (!i_ext || !i_ext->len)
> >> -  return false;
> >> +  return;
> >>  
> >>get_extent_info(, i_ext);
> >>  
> >> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
> >> *inode, struct f2fs_extent *i_e
> >>}
> >>  out:
> >>write_unlock(>lock);
> >> -  return false;
> >>  }
> >>  
> >> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext)
> >> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
> >>  {
> >> -  bool ret =  __f2fs_init_extent_tree(inode, i_ext);
> >> +  __f2fs_init_extent_tree(inode, ipage);
> >>  
> >>if (!F2FS_I(inode)->extent_tree)
> >>set_inode_flag(inode, FI_NO_EXTENT);
> >> -
> >> -  return ret;
> >>  }
> >>  
> >>  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index b35a50f4953c..326c12fa0da5 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
> >> rb_root_cached *root,
> >>  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
> >>struct rb_root_cached *root);
> >>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int 
> >> nr_shrink);
> >> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
> >> *i_ext);
> >> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage);
> >>  void f2fs_drop_extent_tree(struct inode *inode);
> >>  unsigned int f2fs_destroy_extent_node(struct inode *inode);
> >>  void f2fs_destroy_extent_tree(struct inode *inode);
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index 3268f8dd59bb..1862073b96d2 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -1250,6 +1250,7 @@ static int __clone_blkaddrs(struct inode *src_inode, 
> >> struct inode *dst_inode,
> >>f2fs_put_page(psrc, 1);
> >>return PTR_ERR(pdst);
> >>}
> >> +   

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-18 Thread Chao Yu
Hi Jaegeuk,

On 2020/6/19 7:59, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 06/18, Chao Yu wrote:
>> to make page content stable for special device like raid.
> 
> Could you elaborate the problem a bit?

Some devices like raid5 wants page content to be stable, because
it will calculate parity info based page content, if page is not
stable, parity info could be corrupted, result in data inconsistency
in stripe.

Thanks,

> 
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/dir.c  |  2 ++
>>  fs/f2fs/extent_cache.c | 18 +-
>>  fs/f2fs/f2fs.h |  2 +-
>>  fs/f2fs/file.c |  1 +
>>  fs/f2fs/inline.c   |  2 ++
>>  fs/f2fs/inode.c|  3 +--
>>  6 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index d35976785e8c..91e86747a604 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>>  if (IS_ERR(dentry_page))
>>  return PTR_ERR(dentry_page);
>>  
>> +f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
>> +
>>  dentry_blk = page_address(dentry_page);
>>  
>>  make_dentry_ptr_block(NULL, , dentry_blk);
>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>> index e60078460ad1..686c68b98610 100644
>> --- a/fs/f2fs/extent_cache.c
>> +++ b/fs/f2fs/extent_cache.c
>> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct extent_tree 
>> *et,
>>  }
>>  
>>  /* return true, if inode page is changed */
>> -static bool __f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
>> *i_ext)
>> +static void __f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>>  {
>>  struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> +struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : NULL;
>>  struct extent_tree *et;
>>  struct extent_node *en;
>>  struct extent_info ei;
>> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
>> *inode, struct f2fs_extent *i_e
>>  if (!f2fs_may_extent_tree(inode)) {
>>  /* drop largest extent */
>>  if (i_ext && i_ext->len) {
>> +f2fs_wait_on_page_writeback(ipage, NODE, true, true);
>>  i_ext->len = 0;
>> -return true;
>> +set_page_dirty(ipage);
>> +return;
>>  }
>> -return false;
>> +return;
>>  }
>>  
>>  et = __grab_extent_tree(inode);
>>  
>>  if (!i_ext || !i_ext->len)
>> -return false;
>> +return;
>>  
>>  get_extent_info(, i_ext);
>>  
>> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
>> *inode, struct f2fs_extent *i_e
>>  }
>>  out:
>>  write_unlock(>lock);
>> -return false;
>>  }
>>  
>> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext)
>> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>>  {
>> -bool ret =  __f2fs_init_extent_tree(inode, i_ext);
>> +__f2fs_init_extent_tree(inode, ipage);
>>  
>>  if (!F2FS_I(inode)->extent_tree)
>>  set_inode_flag(inode, FI_NO_EXTENT);
>> -
>> -return ret;
>>  }
>>  
>>  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index b35a50f4953c..326c12fa0da5 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
>> rb_root_cached *root,
>>  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
>>  struct rb_root_cached *root);
>>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int 
>> nr_shrink);
>> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext);
>> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage);
>>  void f2fs_drop_extent_tree(struct inode *inode);
>>  unsigned int f2fs_destroy_extent_node(struct inode *inode);
>>  void f2fs_destroy_extent_tree(struct inode *inode);
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 3268f8dd59bb..1862073b96d2 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1250,6 +1250,7 @@ static int __clone_blkaddrs(struct inode *src_inode, 
>> struct inode *dst_inode,
>>  f2fs_put_page(psrc, 1);
>>  return PTR_ERR(pdst);
>>  }
>> +f2fs_wait_on_page_writeback(pdst, DATA, true, true);
>>  f2fs_copy_page(psrc, pdst);
>>  set_page_dirty(pdst);
>>  f2fs_put_page(pdst, 1);
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index dbade310dc79..4bcbc486c9e2 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -340,6 +340,8 @@ int f2fs_make_empty_inline_dir(struct inode *inode, 
>> struct inode *parent,
>>  struct 

Re: [f2fs-dev] [PATCH 1/5] f2fs: fix to wait page writeback before update

2020-06-18 Thread Jaegeuk Kim
Hi Chao,

On 06/18, Chao Yu wrote:
> to make page content stable for special device like raid.

Could you elaborate the problem a bit?

> 
> Signed-off-by: Chao Yu 
> ---
>  fs/f2fs/dir.c  |  2 ++
>  fs/f2fs/extent_cache.c | 18 +-
>  fs/f2fs/f2fs.h |  2 +-
>  fs/f2fs/file.c |  1 +
>  fs/f2fs/inline.c   |  2 ++
>  fs/f2fs/inode.c|  3 +--
>  6 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index d35976785e8c..91e86747a604 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -495,6 +495,8 @@ static int make_empty_dir(struct inode *inode,
>   if (IS_ERR(dentry_page))
>   return PTR_ERR(dentry_page);
>  
> + f2fs_bug_on(F2FS_I_SB(inode), PageWriteback(dentry_page));
> +
>   dentry_blk = page_address(dentry_page);
>  
>   make_dentry_ptr_block(NULL, , dentry_blk);
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index e60078460ad1..686c68b98610 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -325,9 +325,10 @@ static void __drop_largest_extent(struct extent_tree *et,
>  }
>  
>  /* return true, if inode page is changed */
> -static bool __f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent 
> *i_ext)
> +static void __f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>  {
>   struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> + struct f2fs_extent *i_ext = ipage ? _INODE(ipage)->i_ext : NULL;
>   struct extent_tree *et;
>   struct extent_node *en;
>   struct extent_info ei;
> @@ -335,16 +336,18 @@ static bool __f2fs_init_extent_tree(struct inode 
> *inode, struct f2fs_extent *i_e
>   if (!f2fs_may_extent_tree(inode)) {
>   /* drop largest extent */
>   if (i_ext && i_ext->len) {
> + f2fs_wait_on_page_writeback(ipage, NODE, true, true);
>   i_ext->len = 0;
> - return true;
> + set_page_dirty(ipage);
> + return;
>   }
> - return false;
> + return;
>   }
>  
>   et = __grab_extent_tree(inode);
>  
>   if (!i_ext || !i_ext->len)
> - return false;
> + return;
>  
>   get_extent_info(, i_ext);
>  
> @@ -360,17 +363,14 @@ static bool __f2fs_init_extent_tree(struct inode 
> *inode, struct f2fs_extent *i_e
>   }
>  out:
>   write_unlock(>lock);
> - return false;
>  }
>  
> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext)
> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage)
>  {
> - bool ret =  __f2fs_init_extent_tree(inode, i_ext);
> + __f2fs_init_extent_tree(inode, ipage);
>  
>   if (!F2FS_I(inode)->extent_tree)
>   set_inode_flag(inode, FI_NO_EXTENT);
> -
> - return ret;
>  }
>  
>  static bool f2fs_lookup_extent_tree(struct inode *inode, pgoff_t pgofs,
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b35a50f4953c..326c12fa0da5 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3795,7 +3795,7 @@ struct rb_entry *f2fs_lookup_rb_tree_ret(struct 
> rb_root_cached *root,
>  bool f2fs_check_rb_tree_consistence(struct f2fs_sb_info *sbi,
>   struct rb_root_cached *root);
>  unsigned int f2fs_shrink_extent_tree(struct f2fs_sb_info *sbi, int 
> nr_shrink);
> -bool f2fs_init_extent_tree(struct inode *inode, struct f2fs_extent *i_ext);
> +void f2fs_init_extent_tree(struct inode *inode, struct page *ipage);
>  void f2fs_drop_extent_tree(struct inode *inode);
>  unsigned int f2fs_destroy_extent_node(struct inode *inode);
>  void f2fs_destroy_extent_tree(struct inode *inode);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3268f8dd59bb..1862073b96d2 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1250,6 +1250,7 @@ static int __clone_blkaddrs(struct inode *src_inode, 
> struct inode *dst_inode,
>   f2fs_put_page(psrc, 1);
>   return PTR_ERR(pdst);
>   }
> + f2fs_wait_on_page_writeback(pdst, DATA, true, true);
>   f2fs_copy_page(psrc, pdst);
>   set_page_dirty(pdst);
>   f2fs_put_page(pdst, 1);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index dbade310dc79..4bcbc486c9e2 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -340,6 +340,8 @@ int f2fs_make_empty_inline_dir(struct inode *inode, 
> struct inode *parent,
>   struct f2fs_dentry_ptr d;
>   void *inline_dentry;
>  
> + f2fs_wait_on_page_writeback(ipage, NODE, true, true);
> +
>   inline_dentry = inline_data_addr(inode, ipage);
>  
>   make_dentry_ptr_inline(inode, , inline_dentry);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 44582a4db513..7c156eb26dd7 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c