Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] ext4: fix direct I/O read error
@ 2020-06-29  3:09 Jiang Ying
  2020-06-29  6:22 ` Christoph Hellwig
  2020-06-29  6:26 ` Markus Elfring
  0 siblings, 2 replies; 3+ messages in thread
From: Jiang Ying @ 2020-06-29  3:09 UTC (permalink / raw)
  To: Markus.Elfring, tytso, adilger.kernel, linux-ext4, linux-kernel
  Cc: wanglong19

Fixes: 9fe55eea7e4b ("Fix race when checking i_size on direct i/o read").
This commit caused ext4 direct I/O read error when the read size is not
alignment with block size.

Then, I will use a test to explain the error.

(1) Make the file that is not alignment with block size:
	$dd if=/dev/zero of=./test.jar bs=1000 count=3

(2) I wrote a test script named "direct_io_read_file.c" as following:

	#include <stdio.h>
	#include <stdlib.h>
	#include <unistd.h>
	#include <sys/file.h>
	#include <sys/types.h>
	#include <sys/stat.h>
	#include <string.h>
	#define BUF_SIZE 1024

	int main()
	{
		int fd;
		int ret;

		unsigned char *buf;
		ret = posix_memalign((void **)&buf, 512, BUF_SIZE);
		if (ret) {
			perror("posix_memalign failed");
			exit(1);
		}
		fd = open("./test.jar", O_RDONLY | O_DIRECT, 0755);
		if (fd < 0){
			perror("open ./test.jar failed");
			exit(1);
		}

		do {
			ret = read(fd, buf, BUF_SIZE);
			printf("ret=%d\n",ret);
			if (ret < 0) {
				perror("write test.jar failed");
			}
		} while (ret > 0);

		free(buf);
		close(fd);
	}

(3) Compiling the script:
	$gcc direct_io_read_file.c -D_GNU_SOURCE

(4) Exec the script:
	$./a.out

	The result is as following:
	ret=1024
	ret=1024
	ret=952
	ret=-1
	write test.jar failed: Invalid argument.

I have tested this script on XFS filesystem, XFS does not have
this problem, because XFS use iomap_dio_rw() to do direct I/O
read. And the comparing between read offset and file size is done
in iomap_dio_rw(), the code is as following:
	if (pos < size) {
		retval = filemap_write_and_wait_range(mapping, pos,
				pos + iov_length(iov, nr_segs) - 1);

		if (!retval) {
			retval = mapping->a_ops->direct_IO(READ, iocb,
						iov, pos, nr_segs);
		}
		...
	}
Only when "pos < size", direct I/O can be done, or 0 will be return.

I have tested my fix patch, it is up to the mustard of EINVAL in
man2(read) as following:
	#include <unistd.h>
	ssize_t read(int fd, void *buf, size_t count);

	EINVAL
		fd is attached to an object which is unsuitable for reading;
		or the file was opened with the O_DIRECT flag, and either the
		address specified in buf, the value specified in count, or the
		current file offset is not suitably aligned.

So I think this patch can be applied to fix ext4 direct I/O error.

However Ext4 introduces direct I/O read using iomap infrastructure
on kernel 5.5, the patch is commit <b1b4705d54ab>
("ext4: introduce direct I/O read using iomap infrastructure"),
then Ext4 will be the same as XFS, they all use iomap_dio_rw() to do direct
I/O read. So this problem does not exist on kernel 5.5 for Ext4.

From above description, we can see this problem exists on all the kernel
versions between kernel 3.14 and kernel 5.4. Please apply this patch
on these kernel versions, or please use the method on kernel 5.5 to fix
this problem. Thanks.

Co-developed-by: Wang Long <wanglong19@meituan.com>
Signed-off-by: Wang Long <wanglong19@meituan.com>
Signed-off-by: Jiang Ying <jiangying8582@126.com>
---
 fs/ext4/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 516faa2..d514ff5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3821,6 +3821,12 @@ static ssize_t ext4_direct_IO_read(struct kiocb *iocb, struct iov_iter *iter)
 	struct inode *inode = mapping->host;
 	size_t count = iov_iter_count(iter);
 	ssize_t ret;
+	loff_t offset = iocb->ki_pos;
+	loff_t size;
+
+	size = i_size_read(inode);
+	if (offset >= size)
+		return 0;
 
 	/*
 	 * Shared inode_lock is enough for us - it protects against concurrent
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ext4: fix direct I/O read error
  2020-06-29  3:09 [PATCH v2] ext4: fix direct I/O read error Jiang Ying
@ 2020-06-29  6:22 ` Christoph Hellwig
  2020-06-29  6:26 ` Markus Elfring
  1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2020-06-29  6:22 UTC (permalink / raw)
  To: Jiang Ying
  Cc: Markus.Elfring, tytso, adilger.kernel, linux-ext4, linux-kernel,
	wanglong19

Any chance you could add your reproducer to xfstests?

Thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ext4: fix direct I/O read error
  2020-06-29  3:09 [PATCH v2] ext4: fix direct I/O read error Jiang Ying
  2020-06-29  6:22 ` Christoph Hellwig
@ 2020-06-29  6:26 ` Markus Elfring
  1 sibling, 0 replies; 3+ messages in thread
From: Markus Elfring @ 2020-06-29  6:26 UTC (permalink / raw)
  To: Jiang Ying, Wang Long, linux-ext4
  Cc: linux-kernel, kernel-janitors, Andreas Dilger, Theodore Ts'o

> Fixes: 9fe55eea7e4b ("Fix race when checking i_size on direct i/o read").

* I suggest to move this tag to the bottom of the commit message.

* This specification is usually preferred without a dot at the line end.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68#n183


> This commit caused ext4 direct I/O read error when the read size is not
> alignment with block size.

Wording alternative:
aligned …


> (1) Make the file that is not alignment with block size:

… a file … aligned …


> (3) Compiling the script:

(3) Compile the source file:


> (4) Exec the script:

(4) Run the test program:


> … Thanks.

I propose to omit this word here.


> ---
>  fs/ext4/inode.c | 6 ++++++

Would you like to add patch version descriptions here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68#n751> @@ -3821,6 +3821,12 @@ static ssize_t ext4_direct_IO_read(struct kiocb *iocb, struct iov_iter *iter)
>  	struct inode *inode = mapping->host;
>  	size_t count = iov_iter_count(iter);
>  	ssize_t ret;
> +	loff_t offset = iocb->ki_pos;
> +	loff_t size;
> +
> +	size = i_size_read(inode);

How do you think about to use the following source code variant?

+	loff_t size = i_size_read(inode);


> +	if (offset >= size)
> +		return 0;

Will any further fine-tuning matter for this function implementation?

Regards,
Markus

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  3:09 [PATCH v2] ext4: fix direct I/O read error Jiang Ying
2020-06-29  6:22 ` Christoph Hellwig
2020-06-29  6:26 ` Markus Elfring

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