Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: Ritesh Harjani <riteshh@linux.ibm.com>
Cc: jack@suse.cz, tytso@mit.edu, linux-ext4@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, mbobrowski@mbobrowski.org
Subject: Re: [RFCv3 4/4] ext4: Move to shared iolock even without dioread_nolock mount opt
Date: Wed, 20 Nov 2019 15:32:57 +0100
Message-ID: <20191120143257.GE9509@quack2.suse.cz> (raw)
In-Reply-To: <20191120050024.11161-5-riteshh@linux.ibm.com>

On Wed 20-11-19 10:30:24, Ritesh Harjani wrote:
> We were using shared locking only in case of dioread_nolock
> mount option in case of DIO overwrites. This mount condition
> is not needed anymore with current code, since:-
> 
> 1. No race between buffered writes & DIO overwrites.
> Since buffIO writes takes exclusive locks & DIO overwrites
> will take share locking. Also DIO path will make sure
> to flush and wait for any dirty page cache data.
> 
> 2. No race between buffered reads & DIO overwrites, since there
> is no block allocation that is possible with DIO overwrites.
> So no stale data exposure should happen. Same is the case
> between DIO reads & DIO overwrites.
> 
> 3. Also other paths like truncate is protected,
> since we wait there for any DIO in flight to be over.
> 
> 4. In case of buffIO writes followed by DIO reads:
> Since here also we take exclusive locks in ext4_write_begin/end().
> There is no risk of exposing any stale data in this case.
> Since after ext4_write_end, iomap_dio_rw() will wait to flush &
> wait for any dirty page cache data.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

There's one more case to consider here as I mentioned in [1]. There can be
mmap write instantiating dirty page and then someone starting writeback
against that page while DIO read is running still theoretically leading to
stale data exposure. Now this patch does not have influence on that race
but:

1) We need to close the race mentioned above. Maybe we could do that by
proactively allocating unwritten blocks for a page being faulted when there
is direct IO running against the file - the one who fills holes through
mmap write while direct IO is running on the file deserves to suffer the
performance penalty...

2) After this patch there's no point in having dioread_nolock at all so we
can just make that mount option no-op and drop all the precautions from the
buffered IO path connected with dioread_nolock.

[1] https://lore.kernel.org/linux-ext4/20190925092339.GB23277@quack2.suse.cz

								Honza

> ---
>  fs/ext4/file.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 18cbf9fa52c6..b97efc89cd63 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -383,6 +383,17 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
>  	.end_io = ext4_dio_write_end_io,
>  };
>  
> +static bool ext4_dio_should_shared_lock(struct inode *inode)
> +{
> +	if (!S_ISREG(inode->i_mode))

This cannot happen for DIO so no point in checking here.

> +		return false;
> +	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))

Why this?

> +		return false;
> +	if (ext4_should_journal_data(inode))

We don't do DIO when journalling data so no point in checking here
(dio_supported() already checked this).

								Honza
> +		return false;
> +	return true;
> +}
> +
>  /*
>   * The intention here is to start with shared lock acquired then see if any
>   * condition requires an exclusive inode lock. If yes, then we restart the
> @@ -394,8 +405,8 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
>   * - For extending writes case we don't take the shared lock, since it requires
>   *   updating inode i_disksize and/or orphan handling with exclusive lock.
>   *
> - * - shared locking will only be true mostly in case of overwrites with
> - *   dioread_nolock mode. Otherwise we will switch to excl. iolock mode.
> + * - shared locking will only be true mostly in case of overwrites.
> + *   Otherwise we will switch to excl. iolock mode.
>   */
>  static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>  				 unsigned int *iolock, bool *unaligned_io,
> @@ -433,15 +444,14 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>  		*extend = true;
>  	/*
>  	 * Determine whether the IO operation will overwrite allocated
> -	 * and initialized blocks. If so, check to see whether it is
> -	 * possible to take the dioread_nolock path.
> +	 * and initialized blocks.
>  	 *
>  	 * We need exclusive i_rwsem for changing security info
>  	 * in file_modified().
>  	 */
>  	if (*iolock == EXT4_IOLOCK_SHARED &&
>  	    (!IS_NOSEC(inode) || *unaligned_io || *extend ||
> -	     !ext4_should_dioread_nolock(inode) ||
> +	     !ext4_dio_should_shared_lock(inode) ||
>  	     !ext4_overwrite_io(inode, offset, count))) {
>  		ext4_iunlock(inode, *iolock);
>  		*iolock = EXT4_IOLOCK_EXCL;
> @@ -485,7 +495,10 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  		iolock = EXT4_IOLOCK_EXCL;
>  	}
>  
> -	if (iolock == EXT4_IOLOCK_SHARED && !ext4_should_dioread_nolock(inode))
> +	/*
> +	 * Check if we should continue with shared iolock
> +	 */
> +	if (iolock == EXT4_IOLOCK_SHARED && !ext4_dio_should_shared_lock(inode))
>  		iolock = EXT4_IOLOCK_EXCL;
>  
>  	if (iocb->ki_flags & IOCB_NOWAIT) {
> -- 
> 2.21.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20  5:00 [RFCv3 0/4] ext4: Introducing ilock wrapper APIs & fixing i_rwsem scalablity prob. in DIO mixed-rw Ritesh Harjani
2019-11-20  5:00 ` [RFCv3 1/4] ext4: fix ext4_dax_read/write inode locking sequence for IOCB_NOWAIT Ritesh Harjani
2019-11-20 12:51   ` Jan Kara
2019-11-22  5:53   ` Matthew Bobrowski
2019-11-20  5:00 ` [RFCv3 2/4] ext4: Add ext4_ilock & ext4_iunlock API Ritesh Harjani
2019-11-20 11:23   ` Matthew Bobrowski
2019-11-20 12:18     ` Ritesh Harjani
2019-11-20 16:35       ` Matthew Wilcox
2019-11-23 11:51         ` Ritesh Harjani
2019-11-20 13:11   ` Jan Kara
2019-11-20 16:06     ` Darrick J. Wong
2019-11-20  5:00 ` [RFCv3 3/4] ext4: start with shared iolock in case of DIO instead of excl. iolock Ritesh Harjani
2019-11-20 13:55   ` Jan Kara
2019-11-23 13:17     ` Ritesh Harjani
2019-11-20  5:00 ` [RFCv3 4/4] ext4: Move to shared iolock even without dioread_nolock mount opt Ritesh Harjani
2019-11-20 14:32   ` Jan Kara [this message]
2019-11-26 10:51     ` Ritesh Harjani
2019-11-26 12:45       ` Ritesh Harjani
2019-11-29 17:23         ` Jan Kara
2019-11-29 17:18       ` Jan Kara
2019-12-03 11:54         ` Ritesh Harjani
2019-12-03 12:39           ` Jan Kara
2019-12-03 13:10             ` Ritesh Harjani
2019-12-03 13:48               ` Jan Kara

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=20191120143257.GE9509@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=mbobrowski@mbobrowski.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