* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES [not found] <20191223040020.109570-1-yuchao0@huawei.com> @ 2019-12-23 8:41 ` Geert Uytterhoeven 2019-12-25 9:58 ` Vyacheslav Dubeyko ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2019-12-23 8:41 UTC (permalink / raw) To: Chao Yu Cc: Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel Hi, CC linux-fsdevel On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: > As Geert Uytterhoeven reported: > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > On some platforms, HZ can be less than 50, then unexpected 0 timeout > jiffies will be set in congestion_wait(). > > This patch introduces a macro DEFAULT_IO_TIMEOUT_JIFFIES to limit > mininum value of timeout jiffies. > > Signed-off-by: Chao Yu <yuchao0@huawei.com> Thanks for your patch! > --- > fs/f2fs/compress.c | 3 ++- > fs/f2fs/data.c | 5 +++-- > fs/f2fs/f2fs.h | 2 ++ > fs/f2fs/gc.c | 3 ++- > fs/f2fs/inode.c | 3 ++- > fs/f2fs/node.c | 3 ++- > fs/f2fs/recovery.c | 6 ++++-- > fs/f2fs/segment.c | 12 ++++++++---- > fs/f2fs/super.c | 6 ++++-- > 9 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > index 1bc86a54ad71..ee4fe8e644aa 100644 > --- a/fs/f2fs/compress.c > +++ b/fs/f2fs/compress.c > @@ -945,7 +945,8 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, > } else if (ret == -EAGAIN) { > ret = 0; > cond_resched(); > - congestion_wait(BLK_RW_ASYNC, HZ/50); > + congestion_wait(BLK_RW_ASYNC, > + DEFAULT_IO_TIMEOUT_JIFFIES); > lock_page(cc->rpages[i]); > clear_page_dirty_for_io(cc->rpages[i]); > goto retry_write; > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index f1f5c701228d..78b5c0b0287e 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2320,7 +2320,8 @@ int f2fs_encrypt_one_page(struct f2fs_io_info *fio) > /* flush pending IOs and wait for a while in the ENOMEM case */ > if (PTR_ERR(fio->encrypted_page) == -ENOMEM) { > f2fs_flush_merged_writes(fio->sbi); > - congestion_wait(BLK_RW_ASYNC, HZ/50); > + congestion_wait(BLK_RW_ASYNC, > + DEFAULT_IO_TIMEOUT_JIFFIES); > gfp_flags |= __GFP_NOFAIL; > goto retry_encrypt; > } > @@ -2900,7 +2901,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping, > if (wbc->sync_mode == WB_SYNC_ALL) { > cond_resched(); > congestion_wait(BLK_RW_ASYNC, > - HZ/50); > + DEFAULT_IO_TIMEOUT_JIFFIES); > goto retry_write; > } > goto next; > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 16edbf4e05e8..4bdc20a94185 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -559,6 +559,8 @@ enum { > > #define DEFAULT_RETRY_IO_COUNT 8 /* maximum retry read IO count */ > > +#define DEFAULT_IO_TIMEOUT_JIFFIES (max_t(long, HZ/50, 1)) > + > /* maximum retry quota flush count */ > #define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8 > Seeing other file systems (ext4, xfs) and even core MM code suffers from the same issue, perhaps it makes sense to move this into congestion_wait(), i.e. increase the timeout to 1 if it's zero in the latter function? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-23 8:41 ` [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES Geert Uytterhoeven @ 2019-12-25 9:58 ` Vyacheslav Dubeyko 2019-12-26 9:38 ` Chao Yu 2019-12-26 10:43 ` Geert Uytterhoeven 2019-12-26 3:42 ` Chao Yu 2019-12-31 12:52 ` Matthew Wilcox 2 siblings, 2 replies; 8+ messages in thread From: Vyacheslav Dubeyko @ 2019-12-25 9:58 UTC (permalink / raw) To: Geert Uytterhoeven, Chao Yu Cc: Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel On Mon, 2019-12-23 at 09:41 +0100, Geert Uytterhoeven wrote: > Hi, > > CC linux-fsdevel > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: > > As Geert Uytterhoeven reported: > > > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > > > On some platforms, HZ can be less than 50, then unexpected 0 > > timeout > > jiffies will be set in congestion_wait(). > > It looks like that HZ could have various value on diferent platforms. So, why does it need to divide HZ on 50? Does it really necessary? Could it be used HZ only without the division operation? Thanks, Viacheslav Dubeyko. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-25 9:58 ` Vyacheslav Dubeyko @ 2019-12-26 9:38 ` Chao Yu 2019-12-26 10:43 ` Geert Uytterhoeven 1 sibling, 0 replies; 8+ messages in thread From: Chao Yu @ 2019-12-26 9:38 UTC (permalink / raw) To: Vyacheslav Dubeyko, Geert Uytterhoeven Cc: Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel On 2019/12/25 17:58, Vyacheslav Dubeyko wrote: > On Mon, 2019-12-23 at 09:41 +0100, Geert Uytterhoeven wrote: >> Hi, >> >> CC linux-fsdevel >> >> On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: >>> As Geert Uytterhoeven reported: >>> >>> for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); >>> >>> On some platforms, HZ can be less than 50, then unexpected 0 >>> timeout >>> jiffies will be set in congestion_wait(). >>> > > > It looks like that HZ could have various value on diferent platforms. > So, why does it need to divide HZ on 50? Does it really necessary? I guess this code was copied from other filesystems, I have no idea why we should use HZ/50 as timeout interval value. > Could it be used HZ only without the division operation? Actually, as Geert pointed out, we can handle that zeroed value parameter inside congestion_wait() to cover all filesystems use cases. Thanks, > > Thanks, > Viacheslav Dubeyko. > > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-25 9:58 ` Vyacheslav Dubeyko 2019-12-26 9:38 ` Chao Yu @ 2019-12-26 10:43 ` Geert Uytterhoeven 2019-12-26 13:08 ` Vyacheslav Dubeyko 1 sibling, 1 reply; 8+ messages in thread From: Geert Uytterhoeven @ 2019-12-26 10:43 UTC (permalink / raw) To: Vyacheslav Dubeyko Cc: Chao Yu, Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel Hi Vyacheslav, On Wed, Dec 25, 2019 at 10:58 AM Vyacheslav Dubeyko <slava@dubeyko.com> wrote: > On Mon, 2019-12-23 at 09:41 +0100, Geert Uytterhoeven wrote: > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: > > > As Geert Uytterhoeven reported: > > > > > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > > > > > On some platforms, HZ can be less than 50, then unexpected 0 > > > timeout > > > jiffies will be set in congestion_wait(). > > It looks like that HZ could have various value on diferent platforms. > So, why does it need to divide HZ on 50? Does it really necessary? > Could it be used HZ only without the division operation? A timeout of HZ means 1 second. HZ/50 means 20 ms, but has the risk of being zero, if HZ < 50. If you want to use a timeout of 20 ms, you best use msecs_to_jiffies(20), as that takes care of the special cases, and never returns 0. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-26 10:43 ` Geert Uytterhoeven @ 2019-12-26 13:08 ` Vyacheslav Dubeyko 2019-12-26 13:16 ` Geert Uytterhoeven 0 siblings, 1 reply; 8+ messages in thread From: Vyacheslav Dubeyko @ 2019-12-26 13:08 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Chao Yu, Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel Hi Geert, On Thu, 2019-12-26 at 11:43 +0100, Geert Uytterhoeven wrote: > Hi Vyacheslav, > > On Wed, Dec 25, 2019 at 10:58 AM Vyacheslav Dubeyko < > slava@dubeyko.com> wrote: > > On Mon, 2019-12-23 at 09:41 +0100, Geert Uytterhoeven wrote: > > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> > > > wrote: > > > > As Geert Uytterhoeven reported: > > > > > > > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > > > > > > > On some platforms, HZ can be less than 50, then unexpected 0 > > > > timeout > > > > jiffies will be set in congestion_wait(). > > > > It looks like that HZ could have various value on diferent > > platforms. > > So, why does it need to divide HZ on 50? Does it really necessary? > > Could it be used HZ only without the division operation? > > A timeout of HZ means 1 second. > HZ/50 means 20 ms, but has the risk of being zero, if HZ < 50. > > If you want to use a timeout of 20 ms, you best use > msecs_to_jiffies(20), > as that takes care of the special cases, and never returns 0. > The msecs_to_jiffies(20) looks much better for my taste. Maybe, we could use this as solution of the issue? Thanks, Viacheslav Dubeyko. > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-26 13:08 ` Vyacheslav Dubeyko @ 2019-12-26 13:16 ` Geert Uytterhoeven 0 siblings, 0 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2019-12-26 13:16 UTC (permalink / raw) To: Vyacheslav Dubeyko Cc: Chao Yu, Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel Hi Vyacheslav, On Thu, Dec 26, 2019 at 2:08 PM Vyacheslav Dubeyko <slava@dubeyko.com> wrote: > On Thu, 2019-12-26 at 11:43 +0100, Geert Uytterhoeven wrote: > > On Wed, Dec 25, 2019 at 10:58 AM Vyacheslav Dubeyko < > > slava@dubeyko.com> wrote: > > > On Mon, 2019-12-23 at 09:41 +0100, Geert Uytterhoeven wrote: > > > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> > > > > wrote: > > > > > As Geert Uytterhoeven reported: > > > > > > > > > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > > > > > > > > > On some platforms, HZ can be less than 50, then unexpected 0 > > > > > timeout > > > > > jiffies will be set in congestion_wait(). > > > > > > It looks like that HZ could have various value on diferent > > > platforms. > > > So, why does it need to divide HZ on 50? Does it really necessary? > > > Could it be used HZ only without the division operation? > > > > A timeout of HZ means 1 second. > > HZ/50 means 20 ms, but has the risk of being zero, if HZ < 50. > > > > If you want to use a timeout of 20 ms, you best use > > msecs_to_jiffies(20), > > as that takes care of the special cases, and never returns 0. > > > > The msecs_to_jiffies(20) looks much better for my taste. Maybe, we > could use this as solution of the issue? Thanks, sounds good to me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-23 8:41 ` [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES Geert Uytterhoeven 2019-12-25 9:58 ` Vyacheslav Dubeyko @ 2019-12-26 3:42 ` Chao Yu 2019-12-31 12:52 ` Matthew Wilcox 2 siblings, 0 replies; 8+ messages in thread From: Chao Yu @ 2019-12-26 3:42 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel On 2019/12/23 16:41, Geert Uytterhoeven wrote: > Hi, > > CC linux-fsdevel > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: >> As Geert Uytterhoeven reported: >> >> for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); >> >> On some platforms, HZ can be less than 50, then unexpected 0 timeout >> jiffies will be set in congestion_wait(). >> >> This patch introduces a macro DEFAULT_IO_TIMEOUT_JIFFIES to limit >> mininum value of timeout jiffies. >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> > > Thanks for your patch! > >> --- >> fs/f2fs/compress.c | 3 ++- >> fs/f2fs/data.c | 5 +++-- >> fs/f2fs/f2fs.h | 2 ++ >> fs/f2fs/gc.c | 3 ++- >> fs/f2fs/inode.c | 3 ++- >> fs/f2fs/node.c | 3 ++- >> fs/f2fs/recovery.c | 6 ++++-- >> fs/f2fs/segment.c | 12 ++++++++---- >> fs/f2fs/super.c | 6 ++++-- >> 9 files changed, 29 insertions(+), 14 deletions(-) >> >> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c >> index 1bc86a54ad71..ee4fe8e644aa 100644 >> --- a/fs/f2fs/compress.c >> +++ b/fs/f2fs/compress.c >> @@ -945,7 +945,8 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc, >> } else if (ret == -EAGAIN) { >> ret = 0; >> cond_resched(); >> - congestion_wait(BLK_RW_ASYNC, HZ/50); >> + congestion_wait(BLK_RW_ASYNC, >> + DEFAULT_IO_TIMEOUT_JIFFIES); >> lock_page(cc->rpages[i]); >> clear_page_dirty_for_io(cc->rpages[i]); >> goto retry_write; >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >> index f1f5c701228d..78b5c0b0287e 100644 >> --- a/fs/f2fs/data.c >> +++ b/fs/f2fs/data.c >> @@ -2320,7 +2320,8 @@ int f2fs_encrypt_one_page(struct f2fs_io_info *fio) >> /* flush pending IOs and wait for a while in the ENOMEM case */ >> if (PTR_ERR(fio->encrypted_page) == -ENOMEM) { >> f2fs_flush_merged_writes(fio->sbi); >> - congestion_wait(BLK_RW_ASYNC, HZ/50); >> + congestion_wait(BLK_RW_ASYNC, >> + DEFAULT_IO_TIMEOUT_JIFFIES); >> gfp_flags |= __GFP_NOFAIL; >> goto retry_encrypt; >> } >> @@ -2900,7 +2901,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping, >> if (wbc->sync_mode == WB_SYNC_ALL) { >> cond_resched(); >> congestion_wait(BLK_RW_ASYNC, >> - HZ/50); >> + DEFAULT_IO_TIMEOUT_JIFFIES); >> goto retry_write; >> } >> goto next; >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 16edbf4e05e8..4bdc20a94185 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -559,6 +559,8 @@ enum { >> >> #define DEFAULT_RETRY_IO_COUNT 8 /* maximum retry read IO count */ >> >> +#define DEFAULT_IO_TIMEOUT_JIFFIES (max_t(long, HZ/50, 1)) >> + >> /* maximum retry quota flush count */ >> #define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8 >> > > Seeing other file systems (ext4, xfs) and even core MM code suffers from > the same issue, perhaps it makes sense to move this into congestion_wait(), > i.e. increase the timeout to 1 if it's zero in the latter function? Yup, maybe I can submit a RFC patch to change congestion_wait(), before that we still need this f2fs change. Thanks, > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES 2019-12-23 8:41 ` [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES Geert Uytterhoeven 2019-12-25 9:58 ` Vyacheslav Dubeyko 2019-12-26 3:42 ` Chao Yu @ 2019-12-31 12:52 ` Matthew Wilcox 2 siblings, 0 replies; 8+ messages in thread From: Matthew Wilcox @ 2019-12-31 12:52 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Chao Yu, Jaegeuk Kim, linux-f2fs-devel, Linux Kernel Mailing List, Chao Yu, Linux FS Devel On Mon, Dec 23, 2019 at 09:41:02AM +0100, Geert Uytterhoeven wrote: > Hi, > > CC linux-fsdevel > > On Mon, Dec 23, 2019 at 5:01 AM Chao Yu <yuchao0@huawei.com> wrote: > > As Geert Uytterhoeven reported: > > > > for parameter HZ/50 in congestion_wait(BLK_RW_ASYNC, HZ/50); > > > > On some platforms, HZ can be less than 50, then unexpected 0 timeout > > jiffies will be set in congestion_wait(). It doesn't matter, congestion is broken and has been for years. https://lore.kernel.org/linux-mm/20190923111900.GH15392@bombadil.infradead.org/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-12-31 12:52 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20191223040020.109570-1-yuchao0@huawei.com>
2019-12-23 8:41 ` [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES Geert Uytterhoeven
2019-12-25 9:58 ` Vyacheslav Dubeyko
2019-12-26 9:38 ` Chao Yu
2019-12-26 10:43 ` Geert Uytterhoeven
2019-12-26 13:08 ` Vyacheslav Dubeyko
2019-12-26 13:16 ` Geert Uytterhoeven
2019-12-26 3:42 ` Chao Yu
2019-12-31 12:52 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).