Re: [PATCH 4/4] xfs: report the correct read/write dio alignment for reflinked inodes

John Garry
2025-01-07T09:00:36Z

On 07/01/2025 07:03, Darrick J. Wong wrote:
> On Tue, Jan 07, 2025 at 07:10:12AM +0100, Christoph Hellwig wrote:
>> On Mon, Jan 06, 2025 at 06:37:06PM +0000, John Garry wrote:
>>>> + /*
>>>> + * On COW inodes we are forced to always rewrite an entire file system
>>>> + * block or RT extent.
>>>> + *
>>>> + * Because applications assume they can do sector sized direct writes
>>>> + * on XFS we fall back to buffered I/O for sub-block direct I/O in that
>>>> + * case. Because that needs to copy the entire block into the buffer
>>>> + * cache it is highly inefficient and can easily lead to page cache
>>>> + * invalidation races.
>>>> + *
>>>> + * Tell applications to avoid this case by reporting the natively
>>>> + * supported direct I/O read alignment.
>>>
>>> Maybe I mis-read the complete comment, but did you really mean "natively
>>> supported direct I/O write alignment"? You have been talking about writes
>>> only, but then finally mention read alignment.
>>
>> No, this is indeed intended to talk about the different (smaller) read
>> alignment we are now reporting. But I guess the wording is confusing
>> enough that I should improve it?

I know what you are saying, but I found the last paragraph odd. All
along we talk about writes, but then conclude by mentioning reads (and
not writes at all).

>
> How about:
>
> /*
> * For COW inodes, we can only perform out of place writes of entire
> * file allocation units (clusters). For a sub-cluster directio write,
> * we must fall back to buffered I/O to perform the RMW. At best this
> * is highly inefficient; at worst it leads to page cache invalidation
> * races. Tell applications to avoid this by reporting separately the
> * read and (larger) write alignments.

yeah, that ending is better, but maybe Christoph wants to keep the more
wordy original previous paragraphs.

> */
>
> --D
>