* [f2fs-dev] [PATCH] f2fs: compress: fix potential deadlock of compress file [not found] <CGME20211210043017epcas1p38cc53389a50e33752e97618498b18f33@epcas1p3.samsung.com> @ 2021-12-10 4:30 ` Hyeong-Jun Kim 0 siblings, 0 replies; 6+ messages in thread From: Hyeong-Jun Kim @ 2021-12-10 4:30 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, Sungjong Seo There is a potential deadlock between writeback process and a process performing write_begin() or write_cache_pages() while trying to write same compress file, but not compressable, as below: [Process A] - doing checkpoint [Process B] [Process C] f2fs_write_cache_pages() - lock_page() [all pages in cluster, 0-31] - f2fs_write_multi_pages() - f2fs_write_raw_pages() - f2fs_write_single_data_page() - f2fs_do_write_data_page() - return -EAGAIN [f2fs_trylock_op() failed] - unlock_page(page) [e.g., page 0] - generic_perform_write() - f2fs_write_begin() - f2fs_prepare_compress_overwrite() - prepare_compress_overwrite() - lock_page() [e.g., page 0] - lock_page() [e.g., page 1] - lock_page(page) [e.g., page 0] Since there is no compress process, it is no longer necessary to hold locks on every pages in cluster within f2fs_write_raw_pages(). This patch changes f2fs_write_raw_pages() to release all locks first and then perform write same as the non-compress file in f2fs_write_cache_pages(). Fixes: 4c8ff7095bef ("f2fs: support data compression") Signed-off-by: Hyeong-Jun Kim <hj514.kim@samsung.com> Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com> Signed-off-by: Youngjin Gil <youngjin.gil@samsung.com> --- fs/f2fs/compress.c | 50 ++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index 7588e4e817b8..5aae63b5eb5c 100644 --- a/fs/f2fs/compress.c +++ b/fs/f2fs/compress.c @@ -1467,25 +1467,38 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, enum iostat_type io_type) { struct address_space *mapping = cc->inode->i_mapping; - int _submitted, compr_blocks, ret; - int i = -1, err = 0; + int _submitted, compr_blocks, ret, i; compr_blocks = f2fs_compressed_blocks(cc); - if (compr_blocks < 0) { - err = compr_blocks; - goto out_err; + + for (i = 0; i < cc->cluster_size; i++) { + if (!cc->rpages[i]) + continue; + + redirty_page_for_writepage(wbc, cc->rpages[i]); + unlock_page(cc->rpages[i]); } + if (compr_blocks < 0) + return compr_blocks; + for (i = 0; i < cc->cluster_size; i++) { if (!cc->rpages[i]) continue; retry_write: + lock_page(cc->rpages[i]); + if (cc->rpages[i]->mapping != mapping) { +continue_unlock: unlock_page(cc->rpages[i]); continue; } - BUG_ON(!PageLocked(cc->rpages[i])); + if (!PageDirty(cc->rpages[i])) + goto continue_unlock; + + if (!clear_page_dirty_for_io(cc->rpages[i])) + goto continue_unlock; ret = f2fs_write_single_data_page(cc->rpages[i], &_submitted, NULL, NULL, wbc, io_type, @@ -1500,26 +1513,15 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, * avoid deadlock caused by cluster update race * from foreground operation. */ - if (IS_NOQUOTA(cc->inode)) { - err = 0; - goto out_err; - } + if (IS_NOQUOTA(cc->inode)) + return 0; ret = 0; cond_resched(); congestion_wait(BLK_RW_ASYNC, DEFAULT_IO_TIMEOUT); - lock_page(cc->rpages[i]); - - if (!PageDirty(cc->rpages[i])) { - unlock_page(cc->rpages[i]); - continue; - } - - clear_page_dirty_for_io(cc->rpages[i]); goto retry_write; } - err = ret; - goto out_err; + return ret; } *submitted += _submitted; @@ -1528,14 +1530,6 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, f2fs_balance_fs(F2FS_M_SB(mapping), true); return 0; -out_err: - for (++i; i < cc->cluster_size; i++) { - if (!cc->rpages[i]) - continue; - redirty_page_for_writepage(wbc, cc->rpages[i]); - unlock_page(cc->rpages[i]); - } - return err; } int f2fs_write_multi_pages(struct compress_ctx *cc, -- 2.25.1 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] f2fs: compress: fix potential deadlock of compress file @ 2021-12-10 4:30 ` Hyeong-Jun Kim 0 siblings, 0 replies; 6+ messages in thread From: Hyeong-Jun Kim @ 2021-12-10 4:30 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu Cc: Hyeong-Jun Kim, Sungjong Seo, Youngjin Gil, linux-f2fs-devel, linux-kernel There is a potential deadlock between writeback process and a process performing write_begin() or write_cache_pages() while trying to write same compress file, but not compressable, as below: [Process A] - doing checkpoint [Process B] [Process C] f2fs_write_cache_pages() - lock_page() [all pages in cluster, 0-31] - f2fs_write_multi_pages() - f2fs_write_raw_pages() - f2fs_write_single_data_page() - f2fs_do_write_data_page() - return -EAGAIN [f2fs_trylock_op() failed] - unlock_page(page) [e.g., page 0] - generic_perform_write() - f2fs_write_begin() - f2fs_prepare_compress_overwrite() - prepare_compress_overwrite() - lock_page() [e.g., page 0] - lock_page() [e.g., page 1] - lock_page(page) [e.g., page 0] Since there is no compress process, it is no longer necessary to hold locks on every pages in cluster within f2fs_write_raw_pages(). This patch changes f2fs_write_raw_pages() to release all locks first and then perform write same as the non-compress file in f2fs_write_cache_pages(). Fixes: 4c8ff7095bef ("f2fs: support data compression") Signed-off-by: Hyeong-Jun Kim <hj514.kim@samsung.com> Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com> Signed-off-by: Youngjin Gil <youngjin.gil@samsung.com> --- fs/f2fs/compress.c | 50 ++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c index 7588e4e817b8..5aae63b5eb5c 100644 --- a/fs/f2fs/compress.c +++ b/fs/f2fs/compress.c @@ -1467,25 +1467,38 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, enum iostat_type io_type) { struct address_space *mapping = cc->inode->i_mapping; - int _submitted, compr_blocks, ret; - int i = -1, err = 0; + int _submitted, compr_blocks, ret, i; compr_blocks = f2fs_compressed_blocks(cc); - if (compr_blocks < 0) { - err = compr_blocks; - goto out_err; + + for (i = 0; i < cc->cluster_size; i++) { + if (!cc->rpages[i]) + continue; + + redirty_page_for_writepage(wbc, cc->rpages[i]); + unlock_page(cc->rpages[i]); } + if (compr_blocks < 0) + return compr_blocks; + for (i = 0; i < cc->cluster_size; i++) { if (!cc->rpages[i]) continue; retry_write: + lock_page(cc->rpages[i]); + if (cc->rpages[i]->mapping != mapping) { +continue_unlock: unlock_page(cc->rpages[i]); continue; } - BUG_ON(!PageLocked(cc->rpages[i])); + if (!PageDirty(cc->rpages[i])) + goto continue_unlock; + + if (!clear_page_dirty_for_io(cc->rpages[i])) + goto continue_unlock; ret = f2fs_write_single_data_page(cc->rpages[i], &_submitted, NULL, NULL, wbc, io_type, @@ -1500,26 +1513,15 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, * avoid deadlock caused by cluster update race * from foreground operation. */ - if (IS_NOQUOTA(cc->inode)) { - err = 0; - goto out_err; - } + if (IS_NOQUOTA(cc->inode)) + return 0; ret = 0; cond_resched(); congestion_wait(BLK_RW_ASYNC, DEFAULT_IO_TIMEOUT); - lock_page(cc->rpages[i]); - - if (!PageDirty(cc->rpages[i])) { - unlock_page(cc->rpages[i]); - continue; - } - - clear_page_dirty_for_io(cc->rpages[i]); goto retry_write; } - err = ret; - goto out_err; + return ret; } *submitted += _submitted; @@ -1528,14 +1530,6 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, f2fs_balance_fs(F2FS_M_SB(mapping), true); return 0; -out_err: - for (++i; i < cc->cluster_size; i++) { - if (!cc->rpages[i]) - continue; - redirty_page_for_writepage(wbc, cc->rpages[i]); - unlock_page(cc->rpages[i]); - } - return err; } int f2fs_write_multi_pages(struct compress_ctx *cc, -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: compress: fix potential deadlock of compress file 2021-12-10 4:30 ` Hyeong-Jun Kim @ 2021-12-10 6:32 ` fengnan chang -1 siblings, 0 replies; 6+ messages in thread From: fengnan chang @ 2021-12-10 6:32 UTC (permalink / raw) To: Hyeong-Jun Kim; +Cc: Jaegeuk Kim, linux-kernel, Sungjong Seo, linux-f2fs-devel Great work，it fix my problem. _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: compress: fix potential deadlock of compress file @ 2021-12-10 6:32 ` fengnan chang 0 siblings, 0 replies; 6+ messages in thread From: fengnan chang @ 2021-12-10 6:32 UTC (permalink / raw) To: Hyeong-Jun Kim Cc: Jaegeuk Kim, Chao Yu, linux-kernel, linux-f2fs-devel, Sungjong Seo Great work，it fix my problem. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: compress: fix potential deadlock of compress file 2021-12-10 4:30 ` Hyeong-Jun Kim @ 2021-12-10 14:59 ` Chao Yu -1 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2021-12-10 14:59 UTC (permalink / raw) To: Hyeong-Jun Kim, Fengnan Chang, Jaegeuk Kim Cc: Sungjong Seo, linux-kernel, linux-f2fs-devel On 2021/12/10 12:30, Hyeong-Jun Kim wrote: > There is a potential deadlock between writeback process and a process > performing write_begin() or write_cache_pages() while trying to write > same compress file, but not compressable, as below: > > [Process A] - doing checkpoint > [Process B] [Process C] > f2fs_write_cache_pages() > - lock_page() [all pages in cluster, 0-31] > - f2fs_write_multi_pages() > - f2fs_write_raw_pages() > - f2fs_write_single_data_page() > - f2fs_do_write_data_page() > - return -EAGAIN [f2fs_trylock_op() failed] > - unlock_page(page) [e.g., page 0] > - generic_perform_write() > - f2fs_write_begin() > - f2fs_prepare_compress_overwrite() > - prepare_compress_overwrite() > - lock_page() [e.g., page 0] > - lock_page() [e.g., page 1] > - lock_page(page) [e.g., page 0] > > Since there is no compress process, it is no longer necessary to hold > locks on every pages in cluster within f2fs_write_raw_pages(). > > This patch changes f2fs_write_raw_pages() to release all locks first > and then perform write same as the non-compress file in > f2fs_write_cache_pages(). > > Fixes: 4c8ff7095bef ("f2fs: support data compression") > Signed-off-by: Hyeong-Jun Kim <hj514.kim@samsung.com> > Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com> > Signed-off-by: Youngjin Gil <youngjin.gil@samsung.com> Looks good to me, thanks for Fengnan and Hyeong-Jun's report and Hyeong-Jun's fixing. :) Reviewed-by: Chao Yu <chao@kernel.org> Thanks, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] f2fs: compress: fix potential deadlock of compress file @ 2021-12-10 14:59 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2021-12-10 14:59 UTC (permalink / raw) To: Hyeong-Jun Kim, Fengnan Chang, Jaegeuk Kim Cc: Sungjong Seo, Youngjin Gil, linux-f2fs-devel, linux-kernel On 2021/12/10 12:30, Hyeong-Jun Kim wrote: > There is a potential deadlock between writeback process and a process > performing write_begin() or write_cache_pages() while trying to write > same compress file, but not compressable, as below: > > [Process A] - doing checkpoint > [Process B] [Process C] > f2fs_write_cache_pages() > - lock_page() [all pages in cluster, 0-31] > - f2fs_write_multi_pages() > - f2fs_write_raw_pages() > - f2fs_write_single_data_page() > - f2fs_do_write_data_page() > - return -EAGAIN [f2fs_trylock_op() failed] > - unlock_page(page) [e.g., page 0] > - generic_perform_write() > - f2fs_write_begin() > - f2fs_prepare_compress_overwrite() > - prepare_compress_overwrite() > - lock_page() [e.g., page 0] > - lock_page() [e.g., page 1] > - lock_page(page) [e.g., page 0] > > Since there is no compress process, it is no longer necessary to hold > locks on every pages in cluster within f2fs_write_raw_pages(). > > This patch changes f2fs_write_raw_pages() to release all locks first > and then perform write same as the non-compress file in > f2fs_write_cache_pages(). > > Fixes: 4c8ff7095bef ("f2fs: support data compression") > Signed-off-by: Hyeong-Jun Kim <hj514.kim@samsung.com> > Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com> > Signed-off-by: Youngjin Gil <youngjin.gil@samsung.com> Looks good to me, thanks for Fengnan and Hyeong-Jun's report and Hyeong-Jun's fixing. :) Reviewed-by: Chao Yu <chao@kernel.org> Thanks, ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-10 15:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20211210043017epcas1p38cc53389a50e33752e97618498b18f33@epcas1p3.samsung.com>
2021-12-10 4:30 ` [f2fs-dev] [PATCH] f2fs: compress: fix potential deadlock of compress file Hyeong-Jun Kim
2021-12-10 4:30 ` Hyeong-Jun Kim
2021-12-10 6:32 ` [f2fs-dev] " fengnan chang
2021-12-10 6:32 ` fengnan chang
2021-12-10 14:59 ` Chao Yu
2021-12-10 14:59 ` Chao Yu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.