On Tue, Aug 11, 2020 at 12:23:31AM +0800, Ming Lei wrote:

On Mon, Aug 10, 2020 at 04:52:17PM +0900, Tetsuo Handa wrote:

...

On 2020/08/10 12:19, Ming Lei wrote:

...

Block layer usually doesn't support or allow zero-length bvec. Since commit 1bdc76aea115 ("iov_iter: use bvec iterator to implement iterate_bvec()"), iterate_bvec() switches to bvec iterator. However, Al mentioned that 'Zero-length segments are not disallowed' in iov_iter.

Fixes for_each_bvec() so that it can move on after seeing one zero length bvec.

Signed-off-by: Ming Lei ming.lei@redhat.com Link: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2262077.html Fixes: 1bdc76aea115 ("iov_iter: use bvec iterator to implement iterate_bvec()")

Is this Fixes: correct? That commit should be in RHEL8's 4.18 kernel but that kernel does not hit this bug.

Yeah, it is correct, see the following link:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h...

Commit 1bdc76aea115 was merged to v4.8, so it is definitely in both RHEL8's 4.18 based kernel and upstream kernel.

...

Moreover, maybe nobody cares, but behavior of splice() differs when there are only zero-length pages. With this fix, splice() returns 0 despite there is still pipe writers.

It is another new issue, which isn't related with Commit 1bdc76aea115, see below.

...

Man page seems to say that splice() returns 0 when there is no pipe writers...

A return value of 0 means end of input.  If fd_in refers to a pipe,
then this means that there was no data to transfer, and it would not
make sense to block because there are no writers connected to the
write end of the pipe.

----- test case ----- #define _GNU_SOURCE #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include <unistd.h> #include <string.h>

int main(int argc, char *argv[]) { static char buffer[4096]; const int fd = open("/tmp/testfile", O_WRONLY | O_CREAT, 0600); int pipe_fd[2] = { EOF, EOF }; pipe(pipe_fd); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'a', sizeof(buffer)); //write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'b', sizeof(buffer)); //write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'c', sizeof(buffer)); //write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'd', sizeof(buffer)); //write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); splice(pipe_fd[0], NULL, fd, NULL, 65536, 0); return 0; }

The above test doesn't trigger the reported lockup issue, so this patch isn't related with the new issue you described.

BTW, for_each_bvec won't be called in the above splice test code.

Thanks, Ming

On Wed, Aug 12, 2020 at 07:03:59PM +0900, Tetsuo Handa wrote:

On 2020/08/12 18:00, Ming Lei wrote:

...

BTW, for_each_bvec won't be called in the above splice test code.

Please uncomment the // lines when testing for_each_bvec() case.

What is the '//' lines?

This is a test case for testing all empty pages.

But the case for testing all empty pages is not related with this patch, is it?

Thanks, Ming

On 2020/08/11 1:23, Ming Lei wrote:

The same behavior can be observed on v4.8 too, both v4.8 and v4.18 includes 1bdc76aea115. If you apply the fix against v4.8, you can observe the same behavior too.

(...snipped...)

I think this new issue may be introduced between v4.18 and v5.8.

Bisection reported that both problems ("infinite busy loop lockup" and "premature splice() return") became visible since commit a194dfe6e6f6f720 ("pipe: Rearrange sequence in pipe_write() to preallocate slot").

Therefore, although the bug might have been existed since commit 1bdc76aea115 ("iov_iter: use bvec iterator to implement iterate_bvec()"), we need to apply your patch to 5.5+ only.

----- test case -----

#define _GNU_SOURCE #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include <unistd.h> #include <string.h>

int main(int argc, char *argv[]) { static char buffer[4096]; const int fd = open("/tmp/testfile", O_WRONLY | O_CREAT, 0600); int pipe_fd[2] = { EOF, EOF }; pipe(pipe_fd); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'a', sizeof(buffer)); if (argc > 1) write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'b', sizeof(buffer)); if (argc > 1) write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'c', sizeof(buffer)); if (argc > 1) write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); memset(buffer, 'd', sizeof(buffer)); if (argc > 1) write(pipe_fd[1], buffer, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); write(pipe_fd[1], NULL, sizeof(buffer)); splice(pipe_fd[0], NULL, fd, NULL, 65536, 0); return 0; }

----- bisect log -----

# bad: [e42617b825f8073569da76dc4510bfa019b1c35a] Linux 5.5-rc1 # good: [219d54332a09e8d8741c1e1982f5eae56099de85] Linux 5.4 # good: [4d856f72c10ecb060868ed10ff1b1453943fc6c8] Linux 5.3 # good: [0ecfebd2b52404ae0c54a878c872bb93363ada36] Linux 5.2 # good: [e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd] Linux 5.1 # good: [1c163f4c7b3f621efff9b28a47abb36f7378d783] Linux 5.0 git bisect start 'v5.5-rc1' 'v5.4' 'v5.3' 'v5.2' 'v5.1' 'v5.0' '--' 'fs/splice.c' 'fs/pipe.c' 'include/linux/splice.h' 'include/linux/pipe_fs_i.h' # bad: [8f868d68d335a17923dffb6858f8e9b656424699] pipe: Fix missing mask update after pipe_wait() git bisect bad 8f868d68d335a17923dffb6858f8e9b656424699 # bad: [a194dfe6e6f6f7205eea850a420f2bc6a1541209] pipe: Rearrange sequence in pipe_write() to preallocate slot git bisect bad a194dfe6e6f6f7205eea850a420f2bc6a1541209 # good: [6718b6f855a0b4962d54bd625be2718cb820cec6] pipe: Allow pipes to have kernel-reserved slots git bisect good 6718b6f855a0b4962d54bd625be2718cb820cec6 # good: [8446487feba988a92e7649c60367510f0b0445a8] pipe: Conditionalise wakeup in pipe_read() git bisect good 8446487feba988a92e7649c60367510f0b0445a8 # first bad commit: [a194dfe6e6f6f7205eea850a420f2bc6a1541209] pipe: Rearrange sequence in pipe_write() to preallocate slot