Re: [PATCH 0/5] ext4: direct IO via iomap infrastructure

[RITESH HARJANI <riteshh@linux.ibm.com>]

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