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:     dsterba@suse.cz, "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Ira Weiny <ira.weiny@intel.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Christoph Hellwig <hch@infradead.org>,
        Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH 0/3] btrfs: Replace kmap() with kmap_local_page()
Date:   Thu, 02 Jun 2022 20:01:04 +0200
Message-ID: <4713391.F8r316W7xa@opensuse>
In-Reply-To: <20220601132545.GM20633@twin.jikos.cz>
References: <20220531145335.13954-1-fmdefrancesco@gmail.com> <20220601132545.GM20633@twin.jikos.cz>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On mercoled=C3=AC 1 giugno 2022 15:25:45 CEST David Sterba wrote:
> On Tue, May 31, 2022 at 04:53:32PM +0200, Fabio M. De Francesco wrote:
> > This is the first series of patches aimed towards the conversion of=20
Btrfs
> > filesystem from the use of kmap() to kmap_local_page().
>=20
> We've already had patches converting kmaps and you're changing the last
> ones, so this is could be the last series, with two exceptions.
>=20
> 1) You've changed lzo.c and zlib.

and inode.c.

> but the same kmap/kunmap pattern is
>    used in zstd.c.

I thought that these mappings I had worked on were safe to convert.

Instead I wasn't sure that the others I left untouched in zstd.c could so=20
easily and mechanically be converted without prior code inspection and=20
proper tests.

I did see those in zstd.c, but I decided to postpone those conversions=20
because I'm not yet sure if and how the virtual addresses we get currently=
=20
from kmap() are re-used.

I saw assignments like "workspace->in_buf.src =3D kmap(in_page);". Is=20
"in_buf" re-used across different contexts? (I see that Btrfs uses a dozen=
=20
workqueues).=20

I also see that kunmap() is called in the same functions that assign=20
virtual addresses to "in_buf" and this makes me think that those addresses=
=20
are not handled across contexts, otherwise you already have bugs. But may=20
very well be that somewhere in the calls chain the code flushes workqueues=
=20
before returning to the callers (it would mean that when kunmap() is called=
=20
we can be sure that those workqueues are done with using those addresses).=
=20

=46urthermore, what can you say about the tens of page_address() spread=20
across the whole fs/btrfs?=20

If those page_address() take pages from HIGHMEM which were mapped using=20
kmap_local_page(), the filesystem will oops the kernel...

About this issue, please see a bug fix ("[PATCH v2] fs/ext2: Avoid=20
page_address on pages returned by ext2_get_page") at=20
https://lore.kernel.org/lkml/
20210714185448.8707ac239e9f12b3a7f5b9f9@urjc.es/#r

Do they only use physical memory from ZONE_NORMAL?

Can you please confirm that it is safe to convert those left kmap() to=20
kmap_local_page() and that those page_address() are safe?

If so, I have no problems to convert what I had left for later. Otherwise=20
I'll need to carefully inspect the code.

> 2) kmap_atomic in inode.c, so that's technically not kmap but it's said
>    to be deprecated and also can be replaced by kmap_local_page. The
>    context in check_compressed_csum is atomic (in end io) so the kmap
>    hasn't been used there.

I was not 100% sure about the preemption requirements for those call sites=
=20
so I had not converted them yet. Are you saying that there is no need for=20
preempt_disable() at the following sites?

# git grep kmap_atomic fs/btrfs
fs/btrfs/compression.c:                 kaddr =3D kmap_atomic(page);
fs/btrfs/inode.c:                       kaddr =3D kmap_atomic(cpage);
fs/btrfs/inode.c:               kaddr =3D kmap_atomic(page);
fs/btrfs/inode.c:       kaddr =3D kmap_atomic(page);

> > tweed32:~ # btrfs check -p ~zoek/dev/btrfs.file
>=20
> That won't verify if the kmap conversion is OK, it's a runtime thing
> while 'check' verifies the data on device. Have you run any kind of
> stress test with enabled compression before running the check?

Ah, thanks. I didn't know this thing.

I installed (x)fstests a couple of days ago. I think it helps to test this=
=20
and other conversions to local mappings, but I haven't yet had time to=20
learn how to use it.

Does (x)fstests cover the compression code? Are there any specific tests I=
=20
should target?

> Please send patches converting zstd.c and the remaining kmap_atomic
> usage in inode.c, otherwise the 3 patches are now in misc-next, thanks.

New version is required in any case because LKP reported four uninitialized=
=20
variables in patch 3/3.=20

I'm just reading the reports that both you and Christoph hit. At first=20
sight they seem to be due to page_address() calls (but I may be wrong, just=
=20
had few minutes to reply so late) :(=20

I was wrong in thinking that these call sites could be converted safely.=20
I'll do the tests before posting v2.

Thanks,

=46abio

P.S.: I've just read a message from Ira Weiny about something he saw in the=
=20
unmapping order in zstd_compress_pages(). We must think how to address=20
mapping /unmapping order properly.