Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Matthew Bobrowski <mbobrowski@mbobrowski.org>
To: RITESH HARJANI <riteshh@linux.ibm.com>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	jack@suse.cz, tytso@mit.edu
Subject: Re: [PATCH 0/5] ext4: direct IO via iomap infrastructure
Date: Tue, 13 Aug 2019 21:10:06 +1000
Message-ID: <20190813111004.GA12682@poseidon.bobrowski.net> (raw)
In-Reply-To: <20190812173150.AF04F5204F@d06av21.portsmouth.uk.ibm.com>

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.

> I will also do some basic testing of xfstests which I did for my patches and
> will revert back.

Sounds good!

> 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.


  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 [this message]
2019-08-13 12:27     ` RITESH HARJANI
2019-08-14  9:48       ` Matthew Bobrowski
2019-08-14 11:58         ` RITESH HARJANI
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=20190813111004.GA12682@poseidon.bobrowski.net \
    --to=mbobrowski@mbobrowski.org \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=riteshh@linux.ibm.com \
    --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