This patch is used to fix ext4 direct I/O read error when
the read size is not aligned with block size.
Then, I will use a test to explain the error.
(1) Make a file that is not aligned with block size:
$dd if=/dev/zero of=./test.jar bs=1000 count=3
(2) I wrote a source file 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) Compile the source file:
$gcc direct_io_read_file.c -D_GNU_SOURCE
(4) Run the test program:
$./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 program 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 the fix patch on Ext4, 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. It will cause the Applications
to fail to read. For example, when the search service downloads a new full
index file, the search engine is loading the previous index file and is
processing the search request, it can not use buffer io that may squeeze
the previous index file in use from pagecache, so the serch service must
use direct I/O read.
Please apply this patch on these kernel versions, or please use the method
on kernel 5.5 to fix this problem.
Fixes: 9fe55eea7e4b ("Fix race when checking i_size on direct i/o read")
Reviewed-by: Jan Kara <[email protected]>
Co-developed-by: Wang Long <[email protected]>
Signed-off-by: Wang Long <[email protected]>
Signed-off-by: Jiang Ying <[email protected]>
Changes since V3:
Add the info: this bug could break some application that use the
stable kernel releases.
Changes since V2:
Optimize the description of the commit message and make a variation for
the patch, e.g. with:
Before:
loff_t size;
size = i_size_read(inode);
After:
loff_t size = i_size_read(inode);
Changes since V1:
Signed-off use real name and add "Fixes:" flag
---
fs/ext4/inode.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 516faa2..a66b0ac 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3821,6 +3821,11 @@ 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 = i_size_read(inode);
+
+ if (offset >= size)
+ return 0;
/*
* Shared inode_lock is enough for us - it protects against concurrent
--
1.8.3.1
On Wed, Aug 05, 2020 at 10:51:07AM +0200, Jan Kara wrote:
> Note to stable tree maintainers (summary from the rather long changelog):
> This is a non-upstream patch. It will not go upstream because the problem
> there has been fixed by converting ext4 to use iomap infrastructure.
> However that change is out of scope for stable kernels and this is a
> minimal fix for the problem that has hit real-world applications so I think
> it would be worth it to include the fix in stable trees. Thanks.
Thanks for the note, I wouldn't have noticed it otherwise :)
greg k-h
Sorry，I will fix this error on 4.4 and 4.9，and then send a patch for 4.4 and 4.9，thanks！
发自我的iPhone
> 在 2020年8月6日，上午9:19，Sasha Levin <[email protected]> 写道：
>
> On Wed, Aug 05, 2020 at 10:51:07AM +0200, Jan Kara wrote:
>> Note to stable tree maintainers (summary from the rather long changelog):
>> This is a non-upstream patch. It will not go upstream because the problem
>> there has been fixed by converting ext4 to use iomap infrastructure.
>> However that change is out of scope for stable kernels and this is a
>> minimal fix for the problem that has hit real-world applications so I think
>> it would be worth it to include the fix in stable trees. Thanks.
>
> How far back should it go? It breaks the build on 4.9 and 4.4 but the
> fix for the breakage is trivial.
>
> It does however suggest that this fix wasn't tested on 4.9 or 4.4, so
> I'd like to clarify it here before fixing it up (or dropping it).
>
> --
> Thanks,
> Sasha