On 12/17/19 5:49 PM, Dave Chinner wrote:
> On Tue, Dec 17, 2019 at 07:31:51AM -0700, Jens Axboe wrote:
>> On 12/15/19 9:17 PM, Dave Chinner wrote:
>>> On Thu, Dec 12, 2019 at 05:57:57PM -0700, Jens Axboe wrote:
>>>> On 12/12/19 5:54 PM, Jens Axboe wrote:
>>>>> On 12/12/19 3:34 PM, Dave Chinner wrote:
>>>>>> Just a thought on further optimisation for this for XFS.
>>>>>> IOMAP_UNCACHED is being passed into the filesystem ->iomap_begin
>>>>>> methods by iomap_apply(). Hence the filesystems know that it is
>>>>>> an uncached IO that is being done, and we can tailor allocation
>>>>>> strategies to suit the fact that the data is going to be written
>>>>>> immediately.
>>>>>>
>>>>>> In this case, XFS needs to treat it the same way it treats direct
>>>>>> IO. That is, we do immediate unwritten extent allocation rather than
>>>>>> delayed allocation. This will reduce the allocation overhead and
>>>>>> will optimise for immediate IO locality rather than optimise for
>>>>>> delayed allocation.
>>>>>>
>>>>>> This should just be a relatively simple change to
>>>>>> xfs_file_iomap_begin() along the lines of:
>>>>>>
>>>>>> - if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
>>>>>> - !IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
>>>>>> + if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) &&
>>>>>> + !(flags & (IOMAP_DIRECT | IOMAP_UNCACHED)) &&
>>>>>> + !IS_DAX(inode) && !xfs_get_extsz_hint(ip)) {
>>>>>> /* Reserve delalloc blocks for regular writeback. */
>>>>>> return xfs_file_iomap_begin_delay(inode, offset, length, flags,
>>>>>> iomap);
>>>>>> }
>>>>>>
>>>>>> so that it avoids delayed allocation for uncached IO...
>>>>>
>>>>> That's very handy! Thanks, I'll add that to the next version. Just out
>>>>> of curiosity, would you prefer this as a separate patch, or just bundle
>>>>> it with the iomap buffered RWF_UNCACHED patch? I'm assuming the latter,
>>>>> and I'll just mention it in the changelog.
>>>>
>>>> OK, since it's in XFS, it'd be a separate patch.
>>>
>>> *nod*
>>>
>>>> The code you quote seems
>>>> to be something out-of-tree?
>>>
>>> Ah, I quoted the code in the 5.4 release branch, not the 5.5-rc1
>>> tree. I'd forgotten that the xfs_file_iomap_begin() got massively
>>> refactored in the 5.5 merge and I hadn't updated my cscope trees. SO
>>> I'm guessing you want to go looking for the
>>> xfs_buffered_write_iomap_begin() and add another case to this
>>> initial branch:
>>>
>>> /* we can't use delayed allocations when using extent size hints */
>>> if (xfs_get_extsz_hint(ip))
>>> return xfs_direct_write_iomap_begin(inode, offset, count,
>>> flags, iomap, srcmap);
>>>
>>> To make the buffered write IO go down the direct IO allocation path...
>>
>> Makes it even simpler! Something like this:
>>
>>
>> commit 1783722cd4b7088a3c004462c7ae610b8e42b720
>> Author: Jens Axboe <axboe@xxxxxxxxx>
>> Date: Tue Dec 17 07:30:04 2019 -0700
>>
>> xfs: don't do delayed allocations for uncached buffered writes
>>
>> This data is going to be written immediately, so don't bother trying
>> to do delayed allocation for it.
>>
>> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 28e2d1f37267..d0cd4a05d59f 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -847,8 +847,11 @@ xfs_buffered_write_iomap_begin(
>> int allocfork = XFS_DATA_FORK;
>> int error = 0;
>>
>> - /* we can't use delayed allocations when using extent size hints */
>> - if (xfs_get_extsz_hint(ip))
>> + /*
>> + * Don't do delayed allocations when using extent size hints, or
>> + * if we were asked to do uncached buffered writes.
>> + */
>> + if (xfs_get_extsz_hint(ip) || (flags & IOMAP_UNCACHED))
>> return xfs_direct_write_iomap_begin(inode, offset, count,
>> flags, iomap, srcmap);
>>
>
> Yup, that's pretty much what I was thinking. :)
Perfect, thanks for checking!
--
Jens Axboe
