* [PATCH] block: allow for_each_bvec to support zero len bvec
@ 2020-08-10 3:19 Ming Lei
2020-08-10 3:33 ` Matthew Wilcox
2020-08-10 7:52 ` Tetsuo Handa
0 siblings, 2 replies; 10+ messages in thread
From: Ming Lei @ 2020-08-10 3:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Ming Lei, Tetsuo Handa, Al Viro, Matthew Wilcox, stable
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()")
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: <stable@vger.kernel.org>
---
include/linux/bvec.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index ac0c7299d5b8..9c4fab5f22a7 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -117,11 +117,18 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
return true;
}
+static inline void bvec_iter_skip_zero_bvec(struct bvec_iter *iter)
+{
+ iter->bi_bvec_done = 0;
+ iter->bi_idx++;
+}
+
#define for_each_bvec(bvl, bio_vec, iter, start) \
for (iter = (start); \
(iter).bi_size && \
((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \
- bvec_iter_advance((bio_vec), &(iter), (bvl).bv_len))
+ (bvl).bv_len ? bvec_iter_advance((bio_vec), &(iter), \
+ (bvl).bv_len) : bvec_iter_skip_zero_bvec(&(iter)))
/* for iterating one bio from start to end */
#define BVEC_ITER_ALL_INIT (struct bvec_iter) \
--
2.25.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 3:19 [PATCH] block: allow for_each_bvec to support zero len bvec Ming Lei
@ 2020-08-10 3:33 ` Matthew Wilcox
2020-08-10 4:02 ` Ming Lei
2020-08-10 7:52 ` Tetsuo Handa
1 sibling, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2020-08-10 3:33 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Tetsuo Handa, Al Viro, stable
On Mon, Aug 10, 2020 at 11:19:15AM +0800, Ming Lei wrote:
> +++ b/include/linux/bvec.h
> @@ -117,11 +117,18 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
> return true;
> }
>
> +static inline void bvec_iter_skip_zero_bvec(struct bvec_iter *iter)
> +{
> + iter->bi_bvec_done = 0;
> + iter->bi_idx++;
> +}
> +
> #define for_each_bvec(bvl, bio_vec, iter, start) \
> for (iter = (start); \
> (iter).bi_size && \
> ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \
> - bvec_iter_advance((bio_vec), &(iter), (bvl).bv_len))
> + (bvl).bv_len ? bvec_iter_advance((bio_vec), &(iter), \
> + (bvl).bv_len) : bvec_iter_skip_zero_bvec(&(iter)))
>
What if you have two zero-length bvecs in a row? Won't this just skip
the first one?
It would seem better to me to put the bv_len test in bvec_iter_advance()
instead of making the macro more complicated.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 3:33 ` Matthew Wilcox
@ 2020-08-10 4:02 ` Ming Lei
0 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2020-08-10 4:02 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jens Axboe, linux-block, Tetsuo Handa, Al Viro, stable
On Mon, Aug 10, 2020 at 04:33:09AM +0100, Matthew Wilcox wrote:
> On Mon, Aug 10, 2020 at 11:19:15AM +0800, Ming Lei wrote:
> > +++ b/include/linux/bvec.h
> > @@ -117,11 +117,18 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
> > return true;
> > }
> >
> > +static inline void bvec_iter_skip_zero_bvec(struct bvec_iter *iter)
> > +{
> > + iter->bi_bvec_done = 0;
> > + iter->bi_idx++;
> > +}
> > +
> > #define for_each_bvec(bvl, bio_vec, iter, start) \
> > for (iter = (start); \
> > (iter).bi_size && \
> > ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \
> > - bvec_iter_advance((bio_vec), &(iter), (bvl).bv_len))
> > + (bvl).bv_len ? bvec_iter_advance((bio_vec), &(iter), \
> > + (bvl).bv_len) : bvec_iter_skip_zero_bvec(&(iter)))
> >
>
> What if you have two zero-length bvecs in a row? Won't this just skip
> the first one?
The 2nd one will be skipped too when it is observed in next loop.
>
> It would seem better to me to put the bv_len test in bvec_iter_advance()
> instead of making the macro more complicated.
The reason is that block layer won't support zero length bvec, and I'd
not bother bvec_iter_advance() for adding this check.
Thanks,
Ming
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 3:19 [PATCH] block: allow for_each_bvec to support zero len bvec Ming Lei
2020-08-10 3:33 ` Matthew Wilcox
@ 2020-08-10 7:52 ` Tetsuo Handa
2020-08-10 16:23 ` Ming Lei
1 sibling, 1 reply; 10+ messages in thread
From: Tetsuo Handa @ 2020-08-10 7:52 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable
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.
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.
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;
}
----- 4.18.0-193.14.2.el8_2.x86_64 -----
openat(AT_FDCWD, "/tmp/testfile", O_WRONLY|O_CREAT, 0600) = 3
pipe([4, 5]) = 0
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
splice(4, NULL, 3, NULL, 65536, 0
^C) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
strace: Process 1486 detached
----- linux.git + this fix -----
open("/tmp/testfile", O_WRONLY|O_CREAT, 0600) = 3
pipe([4, 5]) = 0
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
write(5, NULL, 4096) = -1 EFAULT (Bad address)
splice(4, NULL, 3, NULL, 65536, 0) = 0
exit_group(0) = ?
+++ exited with 0 +++
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
I just forwarded syzbot's report. Thus, credit goes to
Reported-by: syzbot <syzbot+61acc40a49a3e46e25ea@syzkaller.appspotmail.com>
> Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: <stable@vger.kernel.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 7:52 ` Tetsuo Handa
@ 2020-08-10 16:23 ` Ming Lei
2020-08-12 9:00 ` Ming Lei
2020-08-13 1:13 ` Tetsuo Handa
0 siblings, 2 replies; 10+ messages in thread
From: Ming Lei @ 2020-08-10 16:23 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable
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=v5.8&id=1bdc76aea1159a750846c2fc98e404403eb7d51c
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.
>
> ----- 4.18.0-193.14.2.el8_2.x86_64 -----
> openat(AT_FDCWD, "/tmp/testfile", O_WRONLY|O_CREAT, 0600) = 3
> pipe([4, 5]) = 0
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> splice(4, NULL, 3, NULL, 65536, 0
>
> ^C) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
> strace: Process 1486 detached
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.
>
> ----- linux.git + this fix -----
It should have been linux.git, :-)
I think this new issue may be introduced between v4.18 and v5.8.
> open("/tmp/testfile", O_WRONLY|O_CREAT, 0600) = 3
> pipe([4, 5]) = 0
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> write(5, NULL, 4096) = -1 EFAULT (Bad address)
> splice(4, NULL, 3, NULL, 65536, 0) = 0
> exit_group(0) = ?
> +++ exited with 0 +++
>
> > Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
>
> I just forwarded syzbot's report. Thus, credit goes to
>
> Reported-by: syzbot <syzbot+61acc40a49a3e46e25ea@syzkaller.appspotmail.com>
OK.
Thanks,
Ming
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 16:23 ` Ming Lei
@ 2020-08-12 9:00 ` Ming Lei
2020-08-12 10:03 ` Tetsuo Handa
2020-08-13 1:13 ` Tetsuo Handa
1 sibling, 1 reply; 10+ messages in thread
From: Ming Lei @ 2020-08-12 9:00 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable
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=v5.8&id=1bdc76aea1159a750846c2fc98e404403eb7d51c
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-12 9:00 ` Ming Lei
@ 2020-08-12 10:03 ` Tetsuo Handa
2020-08-12 12:47 ` Ming Lei
0 siblings, 1 reply; 10+ messages in thread
From: Tetsuo Handa @ 2020-08-12 10:03 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable
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.
This is a test case for testing all empty pages.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-12 10:03 ` Tetsuo Handa
@ 2020-08-12 12:47 ` Ming Lei
2020-08-12 12:51 ` Matthew Wilcox
0 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2020-08-12 12:47 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-12 12:47 ` Ming Lei
@ 2020-08-12 12:51 ` Matthew Wilcox
0 siblings, 0 replies; 10+ messages in thread
From: Matthew Wilcox @ 2020-08-12 12:51 UTC (permalink / raw)
To: Ming Lei; +Cc: Tetsuo Handa, Jens Axboe, linux-block, Al Viro, stable
On Wed, Aug 12, 2020 at 08:47:12PM +0800, Ming Lei wrote:
> 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?
The lines in the test-case which begin with the sequence '//'.
> > 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] block: allow for_each_bvec to support zero len bvec
2020-08-10 16:23 ` Ming Lei
2020-08-12 9:00 ` Ming Lei
@ 2020-08-13 1:13 ` Tetsuo Handa
1 sibling, 0 replies; 10+ messages in thread
From: Tetsuo Handa @ 2020-08-13 1:13 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Al Viro, Matthew Wilcox, stable, David Howells
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-13 1:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10 3:19 [PATCH] block: allow for_each_bvec to support zero len bvec Ming Lei
2020-08-10 3:33 ` Matthew Wilcox
2020-08-10 4:02 ` Ming Lei
2020-08-10 7:52 ` Tetsuo Handa
2020-08-10 16:23 ` Ming Lei
2020-08-12 9:00 ` Ming Lei
2020-08-12 10:03 ` Tetsuo Handa
2020-08-12 12:47 ` Ming Lei
2020-08-12 12:51 ` Matthew Wilcox
2020-08-13 1:13 ` Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).