On Tue, Aug 24, 2021 at 07:32:06AM +0800, Qu Wenruo wrote:

On 2021/8/24 上午2:16, Omar Sandoval wrote:

quoted

On Sat, Aug 21, 2021 at 09:11:26AM +0800, Qu Wenruo wrote:

quoted

On 2021/8/21 上午2:11, Omar Sandoval wrote:

quoted

On Fri, Aug 20, 2021 at 05:13:34PM +0800, Qu Wenruo wrote:

quoted

On 2021/8/20 下午4:51, Nikolay Borisov wrote:

quoted

On 18.08.21 г. 0:06, Omar Sandoval wrote:

quoted

From: Omar Sandoval <redacted>

Currently, an inline extent is always created after i_size is extended
from btrfs_dirty_pages(). However, for encoded writes, we only want to
update i_size after we successfully created the inline extent.

To me, the idea of write first then update isize is just going to cause
tons of inline extent related prblems.

The current example is falloc, which only update the isize after the
falloc finishes.

This behavior has already bothered me quite a lot, as it can easily
create mixed inline and regular extents.

Do you have an example of how this would happen? I have the inode and
extent bits locked during an encoded write, and I see that fallocate
does the same.

xfs_io -f -c "pwrite 0 1K" -c "sync" -c "falloc 0 4k" -c "pwrite 4k 4k"

The [0, 1K) will be written as inline without doubt.

Then we go to falloc, it will try to zero the range [1K, 4K), but it
doesn't increase the isize.
Thus the page [0, 4k) will still be written back as inline, since isize
is still 1K.

Later [4K, 8K) will be written back as regular, causing mixed extents.

I'll have to read fallocate more closely to follow what's going on here
and figure out if it applies to encoded writes. Please help me out if
you see how this would be an issue with encoded writes.

This won't cause anything wrong, if the encoded writes follows the
existing inline extents requirement (always at offset 0).

Otherwise, the read path could be affected to handle inlined extent at
non-zero offset.

quoted

quoted

quoted

quoted

Can't we remember the old isize (with proper locking), enlarge isize
(with holes filled), do the write.

If something wrong happened, we truncate the isize back to its old isize.

[...]

quoted

quoted

quoted

Urgh, just some days ago Qu was talking about how awkward it is to have
mixed extents in a file. And now, AFAIU, you are making them more likely
since now they can be created not just at the beginning of the file but
also after i_size write. While this won't be a problem in and of itself
it goes just the opposite way of us trying to shrink the possible cases
when we can have mixed extents.

Tree-checker should reject such inline extent at non-zero offset.

This change does not allow creating inline extents at a non-zero offset.

quoted

quoted

Qu what is your take on that?

My question is, why encoded write needs to bother the inline extents at all?

My intuition of such encoded write is, it should not create inline
extents at all.

Or is there any special use-case involved for encoded write?

We create compressed inline extents with normal writes. We should be
able to send and receive them without converting them into regular
extents.

But my first impression for any encoded write is that, they should work
like DIO, thus everything should be sectorsize aligned.

Then why could they create inline extent? As inline extent can only be
possible when the isize is smaller than sectorsize.

ENCODED_WRITE is not defined as "O_DIRECT, but encoded". It happens to
have some resemblance to O_DIRECT because we have alignment requirements
for new extents and because we bypass the page cache, but there's no
reason to copy arbitrary restrictions from O_DIRECT. If someone is using
ENCODED_WRITE to write compressed data, then they care about space
efficiency, so we should make efficient use of inline extents.

Then as long as the inline extent requirement for 0 offset is still
followed, I'll be fine with that.

But for non-zero offset inline extent? It looks like a much larger
change, and may affect read path.

So I'd prefer we keep the 0 offset requirement for inline extent, and
find a better way to work around.

Ah, okay. I didn't get rid of the 0 offset requirement and I have no
plans to. In fact, this patch kind of does the opposite: it gets rid of
the start parameter to cow_file_range_inline() because it doesn't make
sense for it to ever be anything other than 0 (and we're already
checking that start == 0 in the callers).