Hi Chao, Can you please help review the diff given by Jaegeuk below? If it looks good, I can post a v2.
Thanks, On Thu, May 28, 2020 at 12:18:39PM -0700, Jaegeuk Kim wrote: > On 05/28, Chao Yu wrote: > > On 2020/5/28 10:45, Chao Yu wrote: > > > On 2020/5/27 10:20, Sahitya Tummala wrote: > > >> In case a compressed file is getting overwritten, the current retry > > >> logic doesn't include the current page to be retried now as it sets > > >> the new start index as 0 and new end index as writeback_index - 1. > > >> This causes the corresponding cluster to be uncompressed and written > > >> as normal pages without compression. Fix this by allowing writeback to > > >> be retried for the current page as well (in case of compressed page > > >> getting retried due to index mismatch with cluster index). So that > > >> this cluster can be written compressed in case of overwrite. > > >> > > >> Signed-off-by: Sahitya Tummala <stumm...@codeaurora.org> > > >> --- > > >> fs/f2fs/data.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > >> index 4af5fcd..bfd1df4 100644 > > >> --- a/fs/f2fs/data.c > > >> +++ b/fs/f2fs/data.c > > >> @@ -3024,7 +3024,7 @@ static int f2fs_write_cache_pages(struct > > >> address_space *mapping, > > >> if ((!cycled && !done) || retry) { > > > > > > IMO, we add retry logic in wrong place, you can see that cycled value is > > > zero only if wbc->range_cyclic is true, in that case writeback_index is > > > valid. > > > > > > However if retry is true and wbc->range_cyclic is false, then > > > writeback_index > > > would be uninitialized variable. > > > > > > Thoughts? > > > > > > Thanks, > > > > > >> cycled = 1; > > >> index = 0; > > >> - end = writeback_index - 1; > > > > BTW, I notice that range_cyclic writeback flow was refactored in below > > commit, > > and skeleton of f2fs.writepages was copied from > > mm/page-writeback.c::write_cache_pages(), > > I guess we need follow that change. > > > > 64081362e8ff ("mm/page-writeback.c: fix range_cyclic writeback vs > > writepages deadlock") > > Is that something like this? > > --- > fs/f2fs/data.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 48a622b95b76e..28fcdf0d4dcb9 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2861,7 +2861,6 @@ static int f2fs_write_cache_pages(struct address_space > *mapping, > pgoff_t index; > pgoff_t end; /* Inclusive */ > pgoff_t done_index; > - int cycled; > int range_whole = 0; > xa_mark_t tag; > int nwritten = 0; > @@ -2879,17 +2878,12 @@ static int f2fs_write_cache_pages(struct > address_space *mapping, > if (wbc->range_cyclic) { > writeback_index = mapping->writeback_index; /* prev offset */ > index = writeback_index; > - if (index == 0) > - cycled = 1; > - else > - cycled = 0; > end = -1; > } else { > index = wbc->range_start >> PAGE_SHIFT; > end = wbc->range_end >> PAGE_SHIFT; > if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) > range_whole = 1; > - cycled = 1; /* ignore range_cyclic tests */ > } > if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages) > tag = PAGECACHE_TAG_TOWRITE; > @@ -3054,10 +3048,9 @@ static int f2fs_write_cache_pages(struct address_space > *mapping, > } > } > #endif > - if ((!cycled && !done) || retry) { > - cycled = 1; > + if (retry) { > index = 0; > - end = writeback_index - 1; > + end = -1; > goto retry; > } > if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0)) > -- > 2.27.0.rc0.183.gde8f92d652-goog > -- -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.