Re: [inline_data] ext4: Stale flags before sync when convert to non-inline

From: Daniel Dawson <danielcdawson@gmail.com>
To: Luis Henriques <lhenriques@suse.de>
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
Date: Sun, 28 Jan 2024 04:06:38 -0800	[thread overview]
Message-ID: <a25e5ee6-70c2-421f-92c2-407b43a7c61e@gmail.com> (raw)
In-Reply-To: <87jznyr6xd.fsf@suse.de>

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 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.

-- 
PGP fingerprint: 5BBD5080FEB0EF7F142F8173D572B791F7B4422A