On 8/14/19 3:18 PM, Matthew Bobrowski wrote:
On Tue, Aug 13, 2019 at 05:57:22PM +0530, RITESH HARJANI wrote:
On 8/13/19 4:40 PM, Matthew Bobrowski wrote:
On Mon, Aug 12, 2019 at 11:01:50PM +0530, RITESH HARJANI wrote:
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.
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?
Ah yes, I see. I believe that what you're saying is correct and I
think we will need to account for this case. But, I haven't thought
about how to do this just yet.
That's not a problem, we can surely discuss the possible approaches.
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;
I cannot explain why, because I myself don't know exactly why in this
particular case the extents cannot be merged. Perhaps `git blame` is
our friend and we can direct that question accordingly, or someone
else on this mailing list knows the answer. :-)
git blame didn't help much. But I think I may know what the above
condition means.
So I think if there is an ongoing IO to an unwritten extent, we may
sometimes first split that extent
to match the exact range of the IO, then write data to it and then
convert that *exact range* to written extent.
So this means while there is an ongoing IO to any inode which has
unwritten extents, we should not allow
any other extent to merge with extents of this inode.
Now one race which I could think of it is, when we are doing AIO DIO and
in parallel doing fallocate to the end of the file.
But since we wait for inode_dio_wait in fallocate, so that should not
race with any DIO paths.
I think, here we can wait to get answers from others to understand, if
there is any race with any of the DIO paths on removing this state
flag(EXT4_STATE_DIO_UNWRITTEN) & not
setting "i_unwritten". Whether it can race with any other path and if
this state flag is necessary(in DIO cases also) for the correct
functionality?
Regards
Ritesh
