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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Gao Xiang <hsiangkao@linux.alibaba.com>
Cc:     linux-erofs@lists.ozlabs.org, Chao Yu <chao@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, ira.weiny@intel.com
Subject: Re: [PATCH v2] erofs: use kmap_local_page() only for erofs_bread()
Date:   Wed, 19 Oct 2022 01:21:27 +0200
Message-ID: <2019477.yKVeVyVuyW@mypc>
In-Reply-To: <Y08asdeoz5yOAefN@B-P7TQMD6M-0146.lan>
References: <20221018105313.4940-1-hsiangkao@linux.alibaba.com> <9108233.CDJkKcVGEf@mypc> <Y08asdeoz5yOAefN@B-P7TQMD6M-0146.lan>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On Tuesday, October 18, 2022 11:29:21 PM CEST Gao Xiang wrote:
> Hi Fabio,
> 
> On Tue, Oct 18, 2022 at 09:18:49PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, October 18, 2022 12:53:13 PM CEST Gao Xiang wrote:
> > > Convert all mapped erofs_bread() users to use kmap_local_page()
> > > instead of kmap() or kmap_atomic().
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > > ---
> > >  fs/erofs/data.c     | 8 ++------
> > >  fs/erofs/internal.h | 3 +--
> > >  fs/erofs/xattr.c    | 8 ++++----
> > >  fs/erofs/zmap.c     | 4 ++--
> > >  4 files changed, 9 insertions(+), 14 deletions(-)
> > > 
> > 
> > I just realized that you know the code of fs/erofs very well. I saw a Gao 
> > Xiang in MAINTAINERS, although having a different email address.
> > 
> > Therefore, I'm sure that everybody can trust that you checked everything 
is 
> > needed to assure the safety of the conversions.
> > 
> > However, an extended commit message would have prevented me to send you 
the 
> > previous email with all those questions / objections.
> 
> Thanks for your suggestion. 
> Yeah, this conversion looks trivial [since most
> paths for erofs_bread() don't have more restriction in principle so we can
> just disable migration.

Not sure about what you mean by "restrictions". Few months ago I updated the 
kmap_local_page() documentantation (highmem.rst). Please take a look at it, so 
that you may check if what you call restrictions are intended the way you 
mean.

The two most important rules are (1) that users cannot hand the virtual kernel 
addresses returned by kmap_local_page() to other contexts (that is why they 
are thread local) and (2) how to nest mappings /unmappings.

> One of what I need to care is nested kmap() usage,
> some unmap/remap order cannot be simply converted to kmap_local()

Correct about nesting. If we map A and then B, we must unmap B and then A.

However, as you seem to convey, not always unmappings in right order (stack 
based) is possible, sometimes because very long functions have many loop's 
breaks and many goto exit labels.

> but I think
> it's not the case for erofs_bread().  Actually EROFS has one of such nested
> kmap() usage, but I don't really care its performance on HIGHMEM platforms,
> so I think kmap() is still somewhat useful compared to kmap_local() from 
this
> point of view],

In Btrfs I solved (thanks to David S.' advice) by mapping only one of two 
pages, only the one coming from the page cache. 

The other page didn't need the use of kmap_local_page() because it was 
allocated in the filesystem with "alloc_page(GFP_NOFS)". GFP_NOFS won't ever 
allocate from ZONE_HIGHMEM, therefore a direct page_address() could avoid the 
mapping and the nesting issues.

Did you check if you may solve the same way? 

A little group of people are working to remove all kmap() and kmap_atomic() we 
meet across the whole kernel. I have not yet encountered conversions which 
cannot be made. Sometimes we may refactor, if what I said above cannot apply.

> but in order to make it all work properly, I will try to do
> stress test with 32-bit platform later. 

I use QEMU/KVM x86_32 VM, 6GB RAM, and a kernel with HIGHMEM64 enabled and an 
openSUSE Tumbleweed 32 distro. I've heard that Debian too provides an x86_32 
distribution. 

> Since it targets for the next cycle
> 6.2, I will do a full stress test in the next following weeks.
> 
> Thanks,
> Gao Xiang
> 

Thanks,

Fabio