Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191223040020.109570-1-yuchao0@huawei.com>
In-Reply-To: <20191223040020.109570-1-yuchao0@huawei.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 23 Dec 2019 09:41:02 +0100
Message-ID: <CAMuHMdUDMv_mMw_ZU4BtuRKX1OvMhjLWw2owTcAP-0D4j5XROw@mail.gmail.com>
Subject: Re: [PATCH] f2fs: introduce DEFAULT_IO_TIMEOUT_JIFFIES
To:     Chao Yu <yuchao0@huawei.com>
Cc:     Jaegeuk Kim <jaegeuk@kernel.org>,
        linux-f2fs-devel@lists.sourceforge.net,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Chao Yu <chao@kernel.org>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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