On Wed, Oct 14, 2020 at 07:44:16PM -0700, Linus Torvalds wrote:
> On Wed, Oct 14, 2020 at 6:48 PM Qian Cai <cai@xxxxxx> wrote:
> >
> > While on this topic, I just want to bring up a bug report that we are chasing an
> > issue that a process is stuck in the loop of wait_on_page_bit_common() for more
> > than 10 minutes before I gave up.
>
> Judging by call trace, that looks like a deadlock rather than a missed wakeup.
>
> The trace isn't reliable, but I find it suspicious that the call trace
> just before the fault contains that
> "iov_iter_copy_from_user_atomic()".
>
> IOW, I think you're in fuse_fill_write_pages(), which has allocated
> the page, locked it, and then it takes a page fault.
>
> And the page fault waits on a page that is locked.
>
> This is a classic deadlock.
>
> The *intent* is that iov_iter_copy_from_user_atomic() returns zero,
> and you retry without the page lock held.
>
> HOWEVER.
>
> That's not what fuse actually does. Fuse will do multiple pages, and
> it will unlock only the _last_ page. It keeps the other pages locked,
> and puts them in an array:
>
> ap->pages[ap->num_pages] = page;
>
> And after the iov_iter_copy_from_user_atomic() fails, it does that
> "unlock" and repeat.
>
> But while the _last_ page was unlocked, the *previous* pages are still
> locked in that array. Deadlock.
>
> I really don't think this has anything at all to do with page locking,
> and everything to do with fuse_fill_write_pages() having a deadlock if
> the source of data is a mmap of one of the pages it is trying to write
> to (just with an offset, so that it's not the last page).
>
> See a similar code sequence in generic_perform_write(), but notice how
> that code only has *one* page that it locks, and never holds an array
> of pages around over that iov_iter_fault_in_readable() thing.
Indeed. This is a deadlock in fuse. Thanks for the analysis. I can now
trivially reproduce it with following program.
Thanks
Vivek
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/uio.h>
int main(int argc, char *argv[])
{
int fd, ret;
void *map_addr;
size_t map_length = 2 * 4096;
char *buf_out = "Hello World";
struct iovec iov[2];
if (argc != 2 ) {
printf("Usage:%s <file-to-write> \n", argv[0]);
exit(1);
}
fd = open(argv[1], O_RDWR | O_TRUNC);
if (fd == -1) {
fprintf(stderr, "Failed to open file %s:%s, errorno=%d\n", argv[1], strerror(errno), errno);
exit(1);
}
map_addr = mmap(NULL, map_length, PROT_READ | PROT_WRITE, MAP_SHARED,
fd, 0);
if (map_addr == MAP_FAILED) {
fprintf(stderr, "mmap failed %s, errorno=%d\n", strerror(errno),
errno);
exit(1);
}
/* Write first page and second page */
pwrite(fd, buf_out, strlen(buf_out), 0);
pwrite(fd, buf_out, strlen(buf_out), 4096);
/* Copy from first page and then second page */
iov[0].iov_base = map_addr;
iov[0].iov_len = strlen(buf_out) + 1;
iov[1].iov_base = map_addr + 4096;
iov[1].iov_len = strlen(buf_out) + 1;
/*
* Write second page offset (4K - 12), reading from first page and
* then second page. In first iteration we should fault in first
* page and lock second page. And in second iteration we should
* try fault in second page which is locked. Deadlock.
*/
ret = pwritev(fd, iov, 2, 4096 + 4096 - 12);
printf("write() returned=%d\n", ret);
munmap(map_addr, map_length);
close(fd);
}
