Re: [f2fs-dev] [PATCH v3] f2fs: don't sleep while grabing nat_tree_lock

Chao Yu
2021-07-23T02:41:26Z

On 2021/7/23 10:35, Jaegeuk Kim wrote:
> This tries to fix priority inversion in the below condition resulting in
> long checkpoint delay.
>
> f2fs_get_node_info()
> - nat_tree_lock
> -> sleep to grab journal_rwsem by contention
>
> checkpoint
> - waiting for nat_tree_lock
>
> In order to let checkpoint go, let's release nat_tree_lock, if there's a
> journal_rwsem contention.
>
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> Change log from v2:
> - don't bother checkpoint
>
> fs/f2fs/node.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 0be9e2d7120e..b26642daa3d2 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -552,7 +552,7 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
> int i;
>
> ni->nid = nid;
> -
> +retry:
> /* Check nat cache */
> down_read(&nm_i->nat_tree_lock);
> e = __lookup_nat_cache(nm_i, nid);
> @@ -564,10 +564,19 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
> return 0;
> }
>
> - memset(&ne, 0, sizeof(struct f2fs_nat_entry));
> + /*
> + * Check current segment summary by trying to grab journal_rwsem first.
> + * This sem is on the critical path on the checkpoint requiring the above
> + * nat_tree_lock. Therefore, we should retry, if we failed to grab here
> + * while not bothering checkpoint.
> + */
> + if (rwsem_is_locked(&sbi->cp_global_sem)) {

You mean: if (!rwsem_is_locked()) ?

IMO, once CP is processing, all readers who needs read nat will be blocked...

Thanks,

> + down_read(&curseg->journal_rwsem);
> + } else if (!down_read_trylock(&curseg->journal_rwsem)) {
> + up_read(&nm_i->nat_tree_lock);
> + goto retry;
> + }
>
> - /* Check current segment summary */
> - down_read(&curseg->journal_rwsem);
> i = f2fs_lookup_journal_in_cursum(journal, NAT_JOURNAL, nid, 0);
> if (i >= 0) {
> ne = nat_in_journal(journal, i);
>