Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Matthew Bobrowski <mbobrowski@mbobrowski.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca,
	linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	riteshh@linux.ibm.com
Subject: Re: [PATCH v7 08/11] ext4: move inode extension/truncate code out from ->iomap_end() callback
Date: Wed, 6 Nov 2019 08:10:59 +1100
Message-ID: <20191105211058.GD1739@bobrowski> (raw)
In-Reply-To: <20191105154950.GC15203@magnolia>

On Tue, Nov 05, 2019 at 07:49:50AM -0800, Darrick J. Wong wrote:
> On Tue, Nov 05, 2019 at 11:01:51PM +1100, Matthew Bobrowski wrote:
> > In preparation for implementing the iomap direct I/O modifications,
> > the inode extension/truncate code needs to be moved out from the
> > ext4_iomap_end() callback. For direct I/O, if the current code
> > remained, it would behave incorrrectly. Updating the inode size prior
> > to converting unwritten extents would potentially allow a racing
> > direct I/O read to find unwritten extents before being converted
> > correctly.
> > 
> > The inode extension/truncate code now resides within a new helper
> > ext4_handle_inode_extension(). This function has been designed so that
> > it can accommodate for both DAX and direct I/O extension/truncate
> > operations.
> > 
> > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ---
> >  fs/ext4/file.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  fs/ext4/inode.c | 48 +-------------------------
> >  2 files changed, 89 insertions(+), 48 deletions(-)
> > 
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 440f4c6ba4ee..ec54fec96a81 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -33,6 +33,7 @@
> >  #include "ext4_jbd2.h"
> >  #include "xattr.h"
> >  #include "acl.h"
> > +#include "truncate.h"
> >  
> >  static bool ext4_dio_supported(struct inode *inode)
> >  {
> > @@ -234,12 +235,95 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from)
> >  	return iov_iter_count(from);
> >  }
> >  
> > +static ssize_t ext4_handle_inode_extension(struct inode *inode, loff_t offset,
> > +					   ssize_t written, size_t count)
> > +{
> > +	handle_t *handle;
> > +	bool truncate = false;
> > +	u8 blkbits = inode->i_blkbits;
> > +	ext4_lblk_t written_blk, end_blk;
> > +
> > +	/*
> > +	 * Note that EXT4_I(inode)->i_disksize can get extended up to
> > +	 * inode->i_size while the I/O was running due to writeback of delalloc
> > +	 * blocks. But, the code in ext4_iomap_alloc() is careful to use
> > +	 * zeroed/unwritten extents if this is possible; thus we won't leave
> > +	 * uninitialized blocks in a file even if we didn't succeed in writing
> > +	 * as much as we intended.
> > +	 */
> > +	WARN_ON_ONCE(i_size_read(inode) < EXT4_I(inode)->i_disksize);
> > +	if (offset + count <= EXT4_I(inode)->i_disksize) {
> > +		/*
> > +		 * We need to ensure that the inode is removed from the orphan
> > +		 * list if it has been added prematurely, due to writeback of
> > +		 * delalloc blocks.
> > +		 */
> > +		if (!list_empty(&EXT4_I(inode)->i_orphan) && inode->i_nlink) {
> > +			handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> > +
> > +			if (IS_ERR(handle)) {
> > +				ext4_orphan_del(NULL, inode);
> > +				return PTR_ERR(handle);
> > +			}
> > +
> > +			ext4_orphan_del(handle, inode);
> > +			ext4_journal_stop(handle);
> 
> I keep seeing this chunk (and the ext4_orphan_add chunk) bouncing around
> through this patchset, which causes me to wonder -- would it be useful
> to refactor these into small helpers?  Or is it really just the same two
> orphan_add/del chunks bouncing around multiple places?

No, you're right. This and the other pattern is sprayed throughout the
patchset, but also possibly throughout some of the other chunks of
EXT4 code (I think), which I haven't touched here. So, my thought
process was to actually introduce a small separate cleanup patchset
that does exactly that i.e. moves out these duplicate chunks
orphan_add/orphan_del into small helpers.

/M

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-05 11:58 [PATCH v7 00/11] ext4: port direct I/O to iomap infrastructure Matthew Bobrowski
2019-11-05 11:58 ` [PATCH v7 01/11] ext4: reorder map.m_flags checks within ext4_iomap_begin() Matthew Bobrowski
2019-11-05 11:59 ` [PATCH v7 02/11] ext4: update direct I/O read lock pattern for IOCB_NOWAIT Matthew Bobrowski
2019-11-05 11:59 ` [PATCH v7 03/11] ext4: iomap that extends beyond EOF should be marked dirty Matthew Bobrowski
2019-11-05 11:59 ` [PATCH v7 04/11] ext4: move set iomap routines into a separate helper ext4_set_iomap() Matthew Bobrowski
2019-11-05 12:00 ` [PATCH v7 05/11] ext4: split IOMAP_WRITE branch in ext4_iomap_begin() into helper Matthew Bobrowski
2019-11-05 12:01 ` [PATCH v7 07/11] ext4: introduce direct I/O read using iomap infrastructure Matthew Bobrowski
2019-11-05 12:01 ` [PATCH v7 08/11] ext4: move inode extension/truncate code out from ->iomap_end() callback Matthew Bobrowski
2019-11-05 15:49   ` Darrick J. Wong
2019-11-05 21:10     ` Matthew Bobrowski [this message]
2019-11-05 12:02 ` [PATCH v7 09/11] ext4: move inode extension check out from ext4_iomap_alloc() Matthew Bobrowski
2019-11-05 12:02 ` [PATCH v7 10/11] ext4: update ext4_sync_file() to not use __generic_file_fsync() Matthew Bobrowski
2019-11-05 12:02 ` [PATCH v7 11/11] ext4: introduce direct I/O write using iomap infrastructure Matthew Bobrowski
2019-11-05 13:59   ` Jan Kara
2019-11-05 20:32     ` Matthew Bobrowski
2019-11-05 20:53       ` Theodore Y. Ts'o
2019-11-05 21:00         ` Matthew Bobrowski
2019-11-06  0:59           ` Theodore Y. Ts'o
2019-11-05 16:28   ` Theodore Y. Ts'o
2019-11-05 20:57     ` Matthew Bobrowski
2019-11-05 12:03 ` [PATCH v7 06/11] ext4: introduce new callback for IOMAP_REPORT Matthew Bobrowski
2019-11-05 15:43   ` Darrick J. Wong

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=20191105211058.GD1739@bobrowski \
    --to=mbobrowski@mbobrowski.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=darrick.wong@oracle.com \
    --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