Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: RITESH HARJANI <riteshh@linux.ibm.com>
To: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	jack@suse.cz, tytso@mit.edu, aneesh.kumar@linux.ibm.com
Subject: Re: [PATCH 0/5] ext4: direct IO via iomap infrastructure
Date: Wed, 14 Aug 2019 17:28:09 +0530
Message-ID: <20190814115810.78742AE045@d06av26.portsmouth.uk.ibm.com> (raw)
In-Reply-To: <20190814094848.GA23465@poseidon.bobrowski.net>


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


  reply index

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-12 12:52 Matthew Bobrowski
2019-08-12 12:52 ` [PATCH 1/5] ext4: introduce direct IO read code path using " Matthew Bobrowski
2019-08-12 17:18   ` Christoph Hellwig
2019-08-12 20:17     ` Matthew Wilcox
2019-08-13 10:45       ` Matthew Bobrowski
2019-08-12 12:52 ` [PATCH 2/5] ext4: move inode extension/truncate code out from ext4_iomap_end() Matthew Bobrowski
2019-08-12 17:18   ` Christoph Hellwig
2019-08-13 10:46     ` Matthew Bobrowski
2019-08-28 19:59   ` Jan Kara
2019-08-28 21:54     ` Matthew Bobrowski
2019-08-29  8:18       ` Jan Kara
2019-08-12 12:53 ` [PATCH 3/5] iomap: modify ->end_io() calling convention Matthew Bobrowski
2019-08-12 17:18   ` Christoph Hellwig
2019-08-13 10:43     ` Matthew Bobrowski
2019-08-12 12:53 ` [PATCH 4/5] ext4: introduce direct IO write code path using iomap infrastructure Matthew Bobrowski
2019-08-12 17:04   ` RITESH HARJANI
2019-08-13 12:58     ` Matthew Bobrowski
2019-08-13 14:35       ` Darrick J. Wong
2019-08-14  9:51         ` Matthew Bobrowski
2019-08-12 17:34   ` Christoph Hellwig
2019-08-13 10:45     ` Matthew Bobrowski
2019-08-28 20:26   ` Jan Kara
2019-08-28 22:32     ` Dave Chinner
2019-08-29  8:03       ` Jan Kara
2019-08-29 11:47       ` Matthew Bobrowski
2019-08-29 11:45     ` Matthew Bobrowski
2019-08-29 12:38       ` Jan Kara
2019-08-12 12:53 ` [PATCH 5/5] ext4: clean up redundant buffer_head direct IO code Matthew Bobrowski
2019-08-12 17:31 ` [PATCH 0/5] ext4: direct IO via iomap infrastructure RITESH HARJANI
2019-08-13 11:10   ` Matthew Bobrowski
2019-08-13 12:27     ` RITESH HARJANI
2019-08-14  9:48       ` Matthew Bobrowski
2019-08-14 11:58         ` RITESH HARJANI [this message]
2019-08-21 13:14       ` Matthew Bobrowski
2019-08-22 12:00         ` Matthew Bobrowski
2019-08-22 14:11           ` Ritesh Harjani
2019-08-24  3:18             ` Matthew Bobrowski
2019-08-24  3:55               ` Darrick J. Wong
2019-08-24 23:04                 ` Christoph Hellwig
2019-08-27  9:52                   ` Matthew Bobrowski
2019-08-28 12:05                     ` Matthew Bobrowski
2019-08-28 14:27                       ` Theodore Y. Ts'o
2019-08-28 18:02                         ` Jan Kara
2019-08-29  6:36                           ` Christoph Hellwig
2019-08-29 11:20                             ` Matthew Bobrowski
2019-08-29 14:41                               ` Christoph Hellwig
2019-08-23 13:43           ` [RFC 1/1] ext4: PoC implementation of option-1 Ritesh Harjani
2019-08-23 13:49             ` Ritesh Harjani

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190814115810.78742AE045@d06av26.portsmouth.uk.ibm.com \
    --to=riteshh@linux.ibm.com \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=mbobrowski@mbobrowski.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git