Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH 4/4] hfsplus: Convert kmap() to kmap_local_page() in
 btree.c
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <20220809203105.26183-5-fmdefrancesco@gmail.com>
Date:   Tue, 9 Aug 2022 15:18:37 -0700
Cc:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Ira Weiny <ira.weiny@intel.com>, Jens Axboe <axboe@kernel.dk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Bart Van Assche <bvanassche@acm.org>,
        Kees Cook <keescook@chromium.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4C85926A-2FB5-418B-8F76-3055FA213D6C@dubeyko.com>
References: <20220809203105.26183-1-fmdefrancesco@gmail.com>
 <20220809203105.26183-5-fmdefrancesco@gmail.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Precedence: bulk


> On Aug 9, 2022, at 1:31 PM, Fabio M. De Francesco =
<fmdefrancesco@gmail.com> wrote:
>=20
> kmap() is being deprecated in favor of kmap_local_page().
>=20
> There are two main problems with kmap(): (1) It comes with an overhead =
as
> mapping space is restricted and protected by a global lock for
> synchronization and (2) it also requires global TLB invalidation when =
the
> kmap=E2=80=99s pool wraps and it might block when the mapping space is =
fully
> utilized until a slot becomes available.
>=20
> With kmap_local_page() the mappings are per thread, CPU local, can =
take
> page faults, and can be called from any context (including =
interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, =
the
> kernel virtual addresses are restored and are still valid.
>=20
> Since its use in btree.c is safe everywhere, it should be preferred.
>=20
> Therefore, replace kmap() with kmap_local_page() in btree.c.
>=20
> Tested in a QEMU/KVM x86_32 VM, 6GB RAM, booting a kernel with
> HIGHMEM64GB enabled.
>=20
> Cc: Viacheslav Dubeyko <slava@dubeyko.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---


Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.


> fs/hfsplus/btree.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>=20
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 3a917a9a4edd..9e1732a2b92a 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -163,7 +163,7 @@ struct hfs_btree *hfs_btree_open(struct =
super_block *sb, u32 id)
> 		goto free_inode;
>=20
> 	/* Load the header */
> -	head =3D (struct hfs_btree_header_rec *)(kmap(page) +
> +	head =3D (struct hfs_btree_header_rec *)(kmap_local_page(page) +
> 		sizeof(struct hfs_bnode_desc));
> 	tree->root =3D be32_to_cpu(head->root);
> 	tree->leaf_count =3D be32_to_cpu(head->leaf_count);
> @@ -240,12 +240,12 @@ struct hfs_btree *hfs_btree_open(struct =
super_block *sb, u32 id)
> 		(tree->node_size + PAGE_SIZE - 1) >>
> 		PAGE_SHIFT;
>=20
> -	kunmap(page);
> +	kunmap_local(head);
> 	put_page(page);
> 	return tree;
>=20
>  fail_page:
> -	kunmap(page);
> +	kunmap_local(head);
> 	put_page(page);
>  free_inode:
> 	tree->inode->i_mapping->a_ops =3D &hfsplus_aops;
> @@ -292,7 +292,7 @@ int hfs_btree_write(struct hfs_btree *tree)
> 		return -EIO;
> 	/* Load the header */
> 	page =3D node->page[0];
> -	head =3D (struct hfs_btree_header_rec *)(kmap(page) +
> +	head =3D (struct hfs_btree_header_rec *)(kmap_local_page(page) +
> 		sizeof(struct hfs_bnode_desc));
>=20
> 	head->root =3D cpu_to_be32(tree->root);
> @@ -304,7 +304,7 @@ int hfs_btree_write(struct hfs_btree *tree)
> 	head->attributes =3D cpu_to_be32(tree->attributes);
> 	head->depth =3D cpu_to_be16(tree->depth);
>=20
> -	kunmap(page);
> +	kunmap_local(head);
> 	set_page_dirty(page);
> 	hfs_bnode_put(node);
> 	return 0;
> @@ -395,7 +395,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree =
*tree)
>=20
> 	off +=3D node->page_offset;
> 	pagep =3D node->page + (off >> PAGE_SHIFT);
> -	data =3D kmap(*pagep);
> +	data =3D kmap_local_page(*pagep);
> 	off &=3D ~PAGE_MASK;
> 	idx =3D 0;
>=20
> @@ -408,7 +408,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree =
*tree)
> 						idx +=3D i;
> 						data[off] |=3D m;
> 						set_page_dirty(*pagep);
> -						kunmap(*pagep);
> +						kunmap_local(data);
> 						tree->free_nodes--;
> 						=
mark_inode_dirty(tree->inode);
> 						hfs_bnode_put(node);
> @@ -418,14 +418,14 @@ struct hfs_bnode *hfs_bmap_alloc(struct =
hfs_btree *tree)
> 				}
> 			}
> 			if (++off >=3D PAGE_SIZE) {
> -				kunmap(*pagep);
> -				data =3D kmap(*++pagep);
> +				kunmap_local(data);
> +				data =3D kmap_local_page(*++pagep);
> 				off =3D 0;
> 			}
> 			idx +=3D 8;
> 			len--;
> 		}
> -		kunmap(*pagep);
> +		kunmap_local(data);
> 		nidx =3D node->next;
> 		if (!nidx) {
> 			hfs_dbg(BNODE_MOD, "create new bmap node\n");
> @@ -441,7 +441,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree =
*tree)
> 		off =3D off16;
> 		off +=3D node->page_offset;
> 		pagep =3D node->page + (off >> PAGE_SHIFT);
> -		data =3D kmap(*pagep);
> +		data =3D kmap_local_page(*pagep);
> 		off &=3D ~PAGE_MASK;
> 	}
> }
> @@ -491,7 +491,7 @@ void hfs_bmap_free(struct hfs_bnode *node)
> 	}
> 	off +=3D node->page_offset + nidx / 8;
> 	page =3D node->page[off >> PAGE_SHIFT];
> -	data =3D kmap(page);
> +	data =3D kmap_local_page(page);
> 	off &=3D ~PAGE_MASK;
> 	m =3D 1 << (~nidx & 7);
> 	byte =3D data[off];
> @@ -499,13 +499,13 @@ void hfs_bmap_free(struct hfs_bnode *node)
> 		pr_crit("trying to free free bnode "
> 				"%u(%d)\n",
> 			node->this, node->type);
> -		kunmap(page);
> +		kunmap_local(data);
> 		hfs_bnode_put(node);
> 		return;
> 	}
> 	data[off] =3D byte & ~m;
> 	set_page_dirty(page);
> -	kunmap(page);
> +	kunmap_local(data);
> 	hfs_bnode_put(node);
> 	tree->free_nodes++;
> 	mark_inode_dirty(tree->inode);
> --=20
> 2.37.1
>=20