Date: Mon, 29 Jan 2024 11:17:04 +0000
From: Luis Henriques <lhenriques@...e.de>
To: Daniel Dawson <danielcdawson@...il.com>
Cc: Theodore Ts'o <tytso@....edu>,  Andreas Dilger
 <adilger.kernel@...ger.ca>,  linux-ext4@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [inline_data] ext4: Stale flags before sync when convert to
 non-inline

Daniel Dawson <danielcdawson@...il.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, but
>> 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 around 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=200681

> of the thread, here is the test I gave for triggering the issue:
>
> $ rm -f test-file; dd if=/dev/zero of=test-file bs=64 count=3 status=none;
> lsattr test-file
>
> Instead of writing the file all at once, it splits it into 3 writes, where 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, such 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() call. The
> changes are to address the file not yet being converted from inline, where 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 application to
> function properly under such a condition, but it should at least *not* give
> ENOENT.
>
> After testing what I think are the changes he proposed, I find it doesn't
> work. If I remove the "&& iomap->type == IOMAP_HOLE", lseek() no longer gives an
> error, but instead returns 0, which I'm pretty sure won't work for the affected
> use case. Either way, I'm not sure I interpreted his description of the changes
> 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,
-- 
Luís

Powered by blists - more mailing lists