On 8/13/19 4:40 PM, Matthew Bobrowski wrote:
On Mon, Aug 12, 2019 at 11:01:50PM +0530, RITESH HARJANI wrote:
This patch series converts the ext4 direct IO code paths to make use of the
iomap infrastructure and removes the old buffer_head direct-io based
implementation. The result is that ext4 is converted to the newer framework
and that it may _possibly_ gain a performance boost for O_SYNC | O_DIRECT IO.
These changes have been tested using xfstests in both DAX and non-DAX modes
using various configurations i.e. 4k, dioread_nolock, dax.
I had some minor review comments posted on Patch-4.
But the rest of the patch series looks good to me.
Thanks for the review, much appreciated! Also, apologies about any
delayed response to your queries, I predominantly do all this work in
my personal time.
Np at all.
I will also do some basic testing of xfstests which I did for my patches and
will revert back.
Sounds good!
I did not find any failure new failures in xfstests with 4K block size.
Neither in basic fio DIO/AIO testing. So my basic testing looks good
(these are mostly the tests which I was using for my debug/dev too)
One query, could you please help answering below for my understanding :-
I was under the assumption that we need to maintain
ext4_test_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN) or
atomic_read(&EXT4_I(inode)->i_unwritten))
in case of non-AIO directIO or AIO directIO case as well (when we may
allocate unwritten extents),
to protect with some kind of race with other parts of code(maybe
truncate/bufferedIO/fallocate not sure?) which may call for
ext4_can_extents_be_merged()
to check if extents can be merged or not.
Is it not the case?
Now that directIO code has no way of specifying that this inode has
unwritten extent, will it not race with any other path, where this info was
necessary (like
in above func ext4_can_extents_be_merged())?
Ah yes, I was under the same assumption when reviewing the code
initially and one of my first solutions was to also use this dynamic
'state' flag in the ->end_io() handler. But, I fell flat on my face as
that deemed to be problematic... This is because there can be multiple
direct IOs to unwritten extents against the same inode, so you cannot
possibly get away with tracking them using this single inode flag. So,
hence the reason why we drop using EXT4_STATE_DIO_UNWRITTEN and use
IOMAP_DIO_UNWRITTEN instead in the ->end_io() handler, which tracks
whether _this_ particular IO has an underlying unwritten extent.
Thanks for taking time to explain this.
Yes, I do understand that part - i.e. while preparing block mapping in
->iomap_begin
we get to know(from ext4_map_blocks) whether this is an unwritten extent
and we add the flag
IOMAP_DIO_UNWRITTEN to iomap. This is needed so that we can convert
unwritten extents in ->end_io
before we update the inode size and mark the inode dirty - to avoid any
race with other AIO DIO or bufferedIO.
But what I meant was this (I may be wrong here since I haven't really
looked into it),
but for my understanding I would like to discuss this -
So earlier with this flag(EXT4_STATE_DIO_UNWRITTEN) we were determining
on whether a newextent can be merged with ex1 in function
ext4_extents_can_be_merged. But now since we have removed that flag we
have no way of knowing that whether
this inode has any unwritten extents or not from any DIO path.
What I meant is isn't this removal of setting/unsetting of
flag(EXT4_STATE_DIO_UNWRITTEN)
changing the behavior of this func - ext4_extents_can_be_merged?
Also - could you please explain why this check returns 0 in the first
place (line 1762 - 1766 below)?
1733 int
1734 ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent
*ex1,
1735 struct ext4_extent *ex2)
<...>
1762 if (ext4_ext_is_unwritten(ex1) &&
1763 (ext4_test_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN) ||
1764 atomic_read(&EXT4_I(inode)->i_unwritten) ||
1765 (ext1_ee_len + ext2_ee_len > EXT_UNWRITTEN_MAX_LEN)))
1766 return 0;
<...>
Regards
Ritesh
