[PATCH v2 1/3] fs: remove redundant cache invalidation after async direct-io write

[PATCH v2 1/3] fs: remove redundant cache invalidation after async direct-io write

[Konstantin Khlebnikov]

Function generic_file_direct_write() invalidates cache at entry. Second
time this should be done when request completes. But this function calls
second invalidation at exit unconditionally even for async requests.

This patch skips second invalidation for async requests (-EIOCBQUEUED).

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 mm/filemap.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 85b7d087eb45..288e38199068 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3218,9 +3218,11 @@ generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	 * Most of the time we do not need this since dio_complete() will do
 	 * the invalidation for us. However there are some file systems that
 	 * do not end up with dio_complete() being called, so let's not break
-	 * them by removing it completely
+	 * them by removing it completely.
+	 *
+	 * Skip invalidation for async writes or if mapping has no pages.
 	 */
-	if (mapping->nrpages)
+	if (written > 0 && mapping->nrpages)
 		invalidate_inode_pages2_range(mapping,
 					pos >> PAGE_SHIFT, end);
 

[PATCH v2 2/3] fs: keep dio_warn_stale_pagecache() when CONFIG_BLOCK=n

[Konstantin Khlebnikov]

This helper prints warning if direct I/O write failed to invalidate cache,
and set EIO at inode to warn usersapce about possible data corruption.
See also commit 5a9d929d6e13 ("iomap: report collisions between directio and
buffered writes to userspace").

Direct I/O is supported by non-disk filesystems, for example NFS.
Thus generic code needs this even in kernel without CONFIG_BLOCK.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 fs/direct-io.c     |   21 ---------------------
 include/linux/fs.h |    6 +++++-
 mm/filemap.c       |   21 +++++++++++++++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 9329ced91f1d..0ec4f270139f 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -220,27 +220,6 @@ static inline struct page *dio_get_page(struct dio *dio,
 	return dio->pages[sdio->head];
 }
 
-/*
- * Warn about a page cache invalidation failure during a direct io write.
- */
-void dio_warn_stale_pagecache(struct file *filp)
-{
-	static DEFINE_RATELIMIT_STATE(_rs, 86400 * HZ, DEFAULT_RATELIMIT_BURST);
-	char pathname[128];
-	struct inode *inode = file_inode(filp);
-	char *path;
-
-	errseq_set(&inode->i_mapping->wb_err, -EIO);
-	if (__ratelimit(&_rs)) {
-		path = file_path(filp, pathname, sizeof(pathname));
-		if (IS_ERR(path))
-			path = "(unknown)";
-		pr_crit("Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!\n");
-		pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid,
-			current->comm);
-	}
-}
-
 /*
  * dio_complete() - called when all DIO BIO I/O has been completed
  *
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..b4e4560d1c38 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3153,7 +3153,6 @@ enum {
 };
 
 void dio_end_io(struct bio *bio);
-void dio_warn_stale_pagecache(struct file *filp);
 
 ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
 			     struct block_device *bdev, struct iov_iter *iter,
@@ -3198,6 +3197,11 @@ static inline void inode_dio_end(struct inode *inode)
 		wake_up_bit(&inode->i_state, __I_DIO_WAKEUP);
 }
 
+/*
+ * Warn about a page cache invalidation failure diring a direct I/O write.
+ */
+void dio_warn_stale_pagecache(struct file *filp);
+
 extern void inode_set_flags(struct inode *inode, unsigned int flags,
 			    unsigned int mask);
 
diff --git a/mm/filemap.c b/mm/filemap.c
index 288e38199068..189b8f318da2 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3161,6 +3161,27 @@ int pagecache_write_end(struct file *file, struct address_space *mapping,
 }
 EXPORT_SYMBOL(pagecache_write_end);
 
+/*
+ * Warn about a page cache invalidation failure during a direct I/O write.
+ */
+void dio_warn_stale_pagecache(struct file *filp)
+{
+	static DEFINE_RATELIMIT_STATE(_rs, 86400 * HZ, DEFAULT_RATELIMIT_BURST);
+	char pathname[128];
+	struct inode *inode = file_inode(filp);
+	char *path;
+
+	errseq_set(&inode->i_mapping->wb_err, -EIO);
+	if (__ratelimit(&_rs)) {
+		path = file_path(filp, pathname, sizeof(pathname));
+		if (IS_ERR(path))
+			path = "(unknown)";
+		pr_crit("Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!\n");
+		pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid,
+			current->comm);
+	}
+}
+
 ssize_t
 generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
 {

Re: [PATCH v2 2/3] fs: keep dio_warn_stale_pagecache() when CONFIG_BLOCK=n

[Jan Kara]

On Sat 02-11-19 16:13:00, Konstantin Khlebnikov wrote:
> This helper prints warning if direct I/O write failed to invalidate cache,
> and set EIO at inode to warn usersapce about possible data corruption.
> See also commit 5a9d929d6e13 ("iomap: report collisions between directio and
> buffered writes to userspace").
> 
> Direct I/O is supported by non-disk filesystems, for example NFS.
> Thus generic code needs this even in kernel without CONFIG_BLOCK.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/direct-io.c     |   21 ---------------------
>  include/linux/fs.h |    6 +++++-
>  mm/filemap.c       |   21 +++++++++++++++++++++
>  3 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 9329ced91f1d..0ec4f270139f 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -220,27 +220,6 @@ static inline struct page *dio_get_page(struct dio *dio,
>  	return dio->pages[sdio->head];
>  }
>  
> -/*
> - * Warn about a page cache invalidation failure during a direct io write.
> - */
> -void dio_warn_stale_pagecache(struct file *filp)
> -{
> -	static DEFINE_RATELIMIT_STATE(_rs, 86400 * HZ, DEFAULT_RATELIMIT_BURST);
> -	char pathname[128];
> -	struct inode *inode = file_inode(filp);
> -	char *path;
> -
> -	errseq_set(&inode->i_mapping->wb_err, -EIO);
> -	if (__ratelimit(&_rs)) {
> -		path = file_path(filp, pathname, sizeof(pathname));
> -		if (IS_ERR(path))
> -			path = "(unknown)";
> -		pr_crit("Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!\n");
> -		pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid,
> -			current->comm);
> -	}
> -}
> -
>  /*
>   * dio_complete() - called when all DIO BIO I/O has been completed
>   *
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e0d909d35763..b4e4560d1c38 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3153,7 +3153,6 @@ enum {
>  };
>  
>  void dio_end_io(struct bio *bio);
> -void dio_warn_stale_pagecache(struct file *filp);
>  
>  ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
>  			     struct block_device *bdev, struct iov_iter *iter,
> @@ -3198,6 +3197,11 @@ static inline void inode_dio_end(struct inode *inode)
>  		wake_up_bit(&inode->i_state, __I_DIO_WAKEUP);
>  }
>  
> +/*
> + * Warn about a page cache invalidation failure diring a direct I/O write.
> + */
> +void dio_warn_stale_pagecache(struct file *filp);
> +
>  extern void inode_set_flags(struct inode *inode, unsigned int flags,
>  			    unsigned int mask);
>  
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 288e38199068..189b8f318da2 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3161,6 +3161,27 @@ int pagecache_write_end(struct file *file, struct address_space *mapping,
>  }
>  EXPORT_SYMBOL(pagecache_write_end);
>  
> +/*
> + * Warn about a page cache invalidation failure during a direct I/O write.
> + */
> +void dio_warn_stale_pagecache(struct file *filp)
> +{
> +	static DEFINE_RATELIMIT_STATE(_rs, 86400 * HZ, DEFAULT_RATELIMIT_BURST);
> +	char pathname[128];
> +	struct inode *inode = file_inode(filp);
> +	char *path;
> +
> +	errseq_set(&inode->i_mapping->wb_err, -EIO);
> +	if (__ratelimit(&_rs)) {
> +		path = file_path(filp, pathname, sizeof(pathname));
> +		if (IS_ERR(path))
> +			path = "(unknown)";
> +		pr_crit("Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!\n");
> +		pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid,
> +			current->comm);
> +	}
> +}
> +
>  ssize_t
>  generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
>  {
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 3/3] fs: warn if stale pagecache is left after direct write

[Konstantin Khlebnikov]

Function generic_file_direct_write() tries to invalidate pagecache after
O_DIRECT write. Unlike to similar code in dio_complete() this silently
ignores error returned from invalidate_inode_pages2_range().

According to comment this code here because not all filesystems call
dio_complete() to do proper invalidation after O_DIRECT write.
Noticeable example is a blkdev_direct_IO().

This patch calls dio_warn_stale_pagecache() if invalidation fails.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 mm/filemap.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 189b8f318da2..dc3b78db079b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3241,11 +3241,13 @@ generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	 * do not end up with dio_complete() being called, so let's not break
 	 * them by removing it completely.
 	 *
+	 * Noticeable example is a blkdev_direct_IO().
+	 *
 	 * Skip invalidation for async writes or if mapping has no pages.
 	 */
-	if (written > 0 && mapping->nrpages)
-		invalidate_inode_pages2_range(mapping,
-					pos >> PAGE_SHIFT, end);
+	if (written > 0 && mapping->nrpages &&
+	    invalidate_inode_pages2_range(mapping, pos >> PAGE_SHIFT, end))
+		dio_warn_stale_pagecache(file);
 
 	if (written > 0) {
 		pos += written;

Re: [PATCH v2 3/3] fs: warn if stale pagecache is left after direct write

[Jan Kara]

On Sat 02-11-19 16:13:03, Konstantin Khlebnikov wrote:
> Function generic_file_direct_write() tries to invalidate pagecache after
> O_DIRECT write. Unlike to similar code in dio_complete() this silently
> ignores error returned from invalidate_inode_pages2_range().
> 
> According to comment this code here because not all filesystems call
> dio_complete() to do proper invalidation after O_DIRECT write.
> Noticeable example is a blkdev_direct_IO().
> 
> This patch calls dio_warn_stale_pagecache() if invalidation fails.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/filemap.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 189b8f318da2..dc3b78db079b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3241,11 +3241,13 @@ generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
>  	 * do not end up with dio_complete() being called, so let's not break
>  	 * them by removing it completely.
>  	 *
> +	 * Noticeable example is a blkdev_direct_IO().
> +	 *
>  	 * Skip invalidation for async writes or if mapping has no pages.
>  	 */
> -	if (written > 0 && mapping->nrpages)
> -		invalidate_inode_pages2_range(mapping,
> -					pos >> PAGE_SHIFT, end);
> +	if (written > 0 && mapping->nrpages &&
> +	    invalidate_inode_pages2_range(mapping, pos >> PAGE_SHIFT, end))
> +		dio_warn_stale_pagecache(file);
>  
>  	if (written > 0) {
>  		pos += written;
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/3] fs: remove redundant cache invalidation after async direct-io write

[Jan Kara]

On Sat 02-11-19 16:12:58, Konstantin Khlebnikov wrote:
> Function generic_file_direct_write() invalidates cache at entry. Second
> time this should be done when request completes. But this function calls
> second invalidation at exit unconditionally even for async requests.
> 
> This patch skips second invalidation for async requests (-EIOCBQUEUED).
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/filemap.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 85b7d087eb45..288e38199068 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3218,9 +3218,11 @@ generic_file_direct_write(struct kiocb *iocb, struct iov_iter *from)
>  	 * Most of the time we do not need this since dio_complete() will do
>  	 * the invalidation for us. However there are some file systems that
>  	 * do not end up with dio_complete() being called, so let's not break
> -	 * them by removing it completely
> +	 * them by removing it completely.
> +	 *
> +	 * Skip invalidation for async writes or if mapping has no pages.
>  	 */
> -	if (mapping->nrpages)
> +	if (written > 0 && mapping->nrpages)
>  		invalidate_inode_pages2_range(mapping,
>  					pos >> PAGE_SHIFT, end);
>  
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR