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