Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sandeep Patil <sspatil@android.com>
To:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Sandeep Patil <sspatil@android.com>, torvalds@linux-foundation.org,
        dhowells@redhat.com, gregkh@linuxfoundation.org,
        stable@vger.kernel.org, kernel-team@android.com
Subject: [PATCH 0/1] Revert change in pipe reader wakeup behavior
Date:   Thu, 29 Jul 2021 22:26:34 +0000
Message-Id: <20210729222635.2937453-1-sspatil@android.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

The commit <1b6b26ae7053>("pipe: fix and clarify pipe write wakeup
logic") changed pipe write logic to wakeup readers only if the pipe
was empty at the time of write. However, there are libraries that relied
upon the older behavior for notification scheme similar to what's
described in [1]

One such library 'realm-core'[2] is used by numerous Android applications.
The library uses a similar notification mechanism as GNU Make but it
never drains the pipe until it is full. When Android moved to v5.10
kernel, all applications using this library stopped working.
The C program at the end of this email mimics the library code.

The program works with 5.4 kernel. It fails with v5.10 and I am fairly
certain it will fail wiht v5.5 as well. The single patch in this series
restores the old behavior. With the patch, the test and all affected
Android applications start working with v5.10

After reading through epoll(7), I think the pipe should be drained after
each epoll_wait() comes back. Also, that a non-empty pipe is
considered to be "ready" for readers. The problem is that prior
to the commit above, any new data written to non-empty pipes
would wakeup threads waiting in epoll(EPOLLIN|EPILLET) and thats
how this library worked.

I do think the program below is using EPOLLET wrong. However, it
used to work before and now it doesn't. So, I thought it is
worth asking if this counts as userspace break.

There was also a symmetrical change made to pipe_read in commit
<f467a6a66419> ("pipe: fix and clarify pipe read wakeup logic")
that I am not sure needs changing.

The library has since been fixed[3] but it will be a while
before all applications incorporate the updated library.


- ssp

1. https://lore.kernel.org/lkml/CAHk-=wjeG0q1vgzu4iJhW5juPkTsjTYmiqiMUYAebWW+0bam6w@mail.gmail.com/
2. https://github.com/realm/realm-core
3. https://github.com/realm/realm-core/issues/4666

====
#include <stdio.h>
#include <error.h>
#include <errno.h>
#include <fcntl.h>
#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <time.h>
#include <sys/epoll.h>
#include <sys/stat.h>
#include <sys/types.h>

#define FIFO_NAME "epoll-test-fifo"

pthread_t tid;
int max_delay_ms = 20;
int fifo_fd;
unsigned char written;
unsigned char received;
int epoll_fd;

void *wait_on_fifo(void *unused)
{
	while (1) {
		struct epoll_event ev;
		int ret;
		unsigned char c;

		ret = epoll_wait(epoll_fd, &ev, 1, 5000);
		if (ret == -1) {
			/* If interrupted syscall, continue .. */
			if (errno == EINTR)
				continue;
			/* epoll_wait failed, bail.. */
			error(99, errno, "epoll_wait failed \n");
		}

		/* timeout */
		if (ret == 0)
			break;

		if (ev.data.fd == fifo_fd) {
			/* Assume this is notification where the thread is catching up.
			 * pipe is emptied by the writer when it detects it is full.
			 */
			received = written;
		}
	}

	return NULL;
}

int write_fifo(int fd, unsigned char c)
{
	while (1) {
		int actual;
		char buf[1024];

		ssize_t ret = write(fd, &c, 1);
		if (ret == 1)
			break;
		/*
		 * If the pipe's buffer is full, we need to read some of the old data in
		 * it to make space. We dont read in the code waiting for
		 * notifications so that we can notify multiple waiters with a single
		 * write.
		 */
		if (ret != 0) {
			if (errno != EAGAIN)
				return -EIO;
		}
		actual = read(fd, buf, 1024);
		if (actual == 0)
			return -errno;
	}

	return 0;
}

int create_and_setup_fifo()
{
	int ret;
	char fifo_path[4096];
	struct epoll_event ev;

	char *tmpdir = getenv("TMPDIR");
	if (tmpdir == NULL)
		tmpdir = ".";

	ret = sprintf(fifo_path, "%s/%s", tmpdir, FIFO_NAME);
	if (access(fifo_path, F_OK) == 0)
		unlink(fifo_path);

	ret = mkfifo(fifo_path, 0600);
	if (ret < 0)
		error(1, errno, "Failed to create fifo");

	fifo_fd = open(fifo_path, O_RDWR | O_NONBLOCK);
	if (fifo_fd < 0)
		error(2, errno, "Failed to open Fifo");

	ev.events = EPOLLIN | EPOLLET;
	ev.data.fd = fifo_fd;

	ret = epoll_ctl(epoll_fd, EPOLL_CTL_ADD, fifo_fd, &ev);
	if (ret < 0)
		error(4, errno, "Failed to add fifo to epoll instance");

	return 0;
}

int main(int argc, char *argv[])
{
	int ret, random;
	unsigned char c = 1;

	epoll_fd = epoll_create(1);
	if (epoll_fd == -1)
		error(3, errno, "Failed to create epoll instance");

	ret = create_and_setup_fifo();
	if (ret != 0)
		error(45, EINVAL, "Failed to setup fifo");

	ret = pthread_create(&tid, NULL, wait_on_fifo, NULL);
	if (ret != 0)
		error(2, errno, "Failed to create a thread");

	srand(time(NULL));

	/* Write 256 bytes to fifo one byte at a time with random delays upto 20ms */
	do {
		written = c;
		ret = write_fifo(fifo_fd, c);
		if (ret != 0)
			error(55, errno, "Failed to notify fifo, write #%u", (unsigned int)c);
		c++;

		random = rand();
		usleep((random % max_delay_ms) * 1000);
	} while (written <= c); /* stop after c = 255 */

	pthread_join(tid, NULL);

	printf("Test: %s", written == received ? "PASS\n" : "FAIL");
	if (written != received)
		printf(": Written (%d) Received (%d)\n", written, received);

	close(fifo_fd);
	close(epoll_fd);

	return 0;
}
====

Sandeep Patil (1):
  fs: pipe: wakeup readers everytime new data written is to pipe

 fs/pipe.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.32.0.554.ge1b32706d8-goog