Received-SPF: pass (google.com: domain of linux-ext4+bounces-994-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Luis Henriques <lhenriques@suse.de>
To: Daniel Dawson <danielcdawson@gmail.com>
Cc: Theodore Ts'o <tytso@mit.edu>,  Andreas Dilger
 <adilger.kernel@dilger.ca>,  linux-ext4@vger.kernel.org,
  linux-kernel@vger.kernel.org
Subject: Re: [inline_data] ext4: Stale flags before sync when convert to
 non-inline
In-Reply-To: <a25e5ee6-70c2-421f-92c2-407b43a7c61e@gmail.com> (Daniel Dawson's
	message of "Sun, 28 Jan 2024 04:06:38 -0800")
References: <5189fe60-c3e3-4bc6-89d4-1033cf4337c3@gmail.com>
	<358aaf68-0618-41e6-9adf-04e211eb690e@gmail.com>
	<87jznyr6xd.fsf@suse.de>
	<a25e5ee6-70c2-421f-92c2-407b43a7c61e@gmail.com>
Date: Mon, 29 Jan 2024 11:17:04 +0000
Message-ID: <87frygqthr.fsf@suse.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Daniel Dawson <danielcdawson@gmail.com> writes:

> I didn't see your message until now. Sorry.
>
> On 1/24/24 9:13 AM, Luis Henriques wrote:
>> Bellow, I'm inlining a patch that started as debug patch that I've used =
to
>> try to understand what was going on.  It seems to workaround that bug, b=
ut
>> I know it's not a real fix -- I don't yet understand what's going on.
>
> Thanks for this. I'm not sure if you meant to say you think it works arou=
nd the
> present issue. I just tested it, and it does not. In case you missed the =
start

I was referring to the bugzilla bug [1] I've been looking into recently.
My patch was a debug patch for that bug, but it definitely does not fix
the issue you're reporting.  Sorry for mixing up everything and confusing
everyone ;-)

[1] https://bugzilla.kernel.org/show_bug.cgi?id=3D200681

> of the thread, here is the test I gave for triggering the issue:
>
> $ rm -f test-file; dd if=3D/dev/zero of=3Dtest-file bs=3D64 count=3D3 sta=
tus=3Dnone;
> lsattr test-file
>
> Instead of writing the file all at once, it splits it into 3 writes, wher=
e the
> first is small enough to make the file inline, and then it becomes
> non-inline. Ideally, the output should be
>
> --------------e------- test-file
>
> but delayed allocation means it instead shows
>
> ------------------N--- test-file
>
> until sync. I also gave this code for testing SEEK_HOLE:
>
> https://gist.github.com/ddawson/22cfd4cac32916f6f1dcc86f90eed21a
>
>> Regarding your specific usecase, I can reproduce it and, unfortunately, I
>> don't thing Ted's suggestion will fix it as I don't even see
>> ext4_iomap_begin_report() being executed at all.
>
> To be clear, that function is called in a few specific circumstances, suc=
h as
> when lseek() is called with SEEK_HOLE or SEEK_DATA, or with FIEMAP. When I
> traced the kernel myself, I did see it being executed from the lseek() ca=
ll. The
> changes are to address the file not yet being converted from inline, wher=
e the
> contents are still written where the map would otherwise be. If you treat=
 it as
> the map, you get nonsense. Something else needs to be done.
>
> I'm not clear on whether his proposed changes would then allow an applica=
tion to
> function properly under such a condition, but it should at least *not* gi=
ve
> ENOENT.
>
> After testing what I think are the changes he proposed, I find it doesn't
> work. If I remove the "&& iomap->type =3D=3D IOMAP_HOLE", lseek() no long=
er gives an
> error, but instead returns 0, which I'm pretty sure won't work for the af=
fected
> use case. Either way, I'm not sure I interpreted his description of the c=
hanges
> correctly.

Sure, I can understand under which circumstances ext4_iomap_begin_report()
can be executed.  What I meant was that, for your very simple test case
(using 'dd' and 'lsattr'), this function will not be executed and thus the
suggested fix will still show the 'N' in the file attributes.

Anyway, thanks a lot for the extra information your providing here.  I'll
try to find some time in the next few days to keep debugging the issue and
hopefully get some more useful info (and, who knows! a patch).

Cheers,
--=20
Lu=C3=ADs