[PATCH] um: hostfs: catch EINTR and partial read/write
Anton Ivanov
anton.ivanov at kot-begemot.co.uk
Fri Nov 10 02:42:21 PST 2023
On 10/11/2023 09:44, benjamin at sipsolutions.net wrote:
> From: Benjamin Berg <benjamin.berg at intel.com>
>
> The UM kernel uses signals for various purposes (SIGALRM for scheduling
> for example). These signals are interrupts for the UM kernel, which
> should not affect file system operations from userspace processes.
>
> Said differently, in hostfs all operations are directly forwarded to the
> host and will block the entire UM kernel. As such, hostfs does not
> permit other tasks to be scheduled while operations are ongoing and
> these operations are fundamentally synchronous and not interruptible.
>
> With all this, the only reasonable thing to do is to catch all EINTR
> situations and retry them. This includes retrying short read/write
> operations in order to ensure they finish.
>
> If, at any point, hostfs becomes able to push operations into the
> background in order to schedule another task, then an abortion mechanism
> may be needed.
>
> Signed-off-by: Benjamin Berg <benjamin.berg at intel.com>
> ---
> arch/um/include/shared/os.h | 7 ++-
> fs/hostfs/hostfs_user.c | 115 +++++++++++++++++++++++-------------
> 2 files changed, 80 insertions(+), 42 deletions(-)
>
> diff --git a/arch/um/include/shared/os.h b/arch/um/include/shared/os.h
> index 0df646c6651e..7c5564ebc5bb 100644
> --- a/arch/um/include/shared/os.h
> +++ b/arch/um/include/shared/os.h
> @@ -18,7 +18,12 @@
> #include <sys/types.h>
> #endif
>
> -#define CATCH_EINTR(expr) while ((errno = 0, ((expr) < 0)) && (errno == EINTR))
> +#define CATCH_EINTR(expr) ({ \
> + int __result; \
> + while ((errno = 0, ((__result = (expr)) < 0)) && \
> + (errno == EINTR)); \
> + __result; \
> + })
>
> #define OS_TYPE_FILE 1
> #define OS_TYPE_DIR 2
> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> index 840619e39a1a..dd30bcc6d58f 100644
> --- a/fs/hostfs/hostfs_user.c
> +++ b/fs/hostfs/hostfs_user.c
> @@ -17,6 +17,7 @@
> #include <sys/syscall.h>
> #include "hostfs.h"
> #include <utime.h>
> +#include <os.h>
>
> static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
> {
> @@ -44,9 +45,9 @@ int stat_file(const char *path, struct hostfs_stat *p, int fd)
> struct stat64 buf;
>
> if (fd >= 0) {
> - if (fstat64(fd, &buf) < 0)
> + if (CATCH_EINTR(fstat64(fd, &buf)) < 0)
> return -errno;
> - } else if (lstat64(path, &buf) < 0) {
> + } else if (CATCH_EINTR(lstat64(path, &buf)) < 0) {
> return -errno;
> }
> stat64_to_hostfs(&buf, p);
> @@ -63,7 +64,7 @@ int access_file(char *path, int r, int w, int x)
> mode |= W_OK;
> if (x)
> mode |= X_OK;
> - if (access(path, mode) != 0)
> + if (CATCH_EINTR(access(path, mode)) != 0)
> return -errno;
> else return 0;
> }
> @@ -82,7 +83,7 @@ int open_file(char *path, int r, int w, int append)
>
> if (append)
> mode |= O_APPEND;
> - fd = open64(path, mode);
> + fd = CATCH_EINTR(open64(path, mode));
> if (fd < 0)
> return -errno;
> else return fd;
> @@ -92,7 +93,11 @@ void *open_dir(char *path, int *err_out)
> {
> DIR *dir;
>
> - dir = opendir(path);
> + do {
> + errno = 0;
> + dir = opendir(path);
> + } while (dir == NULL && errno == -EINTR);
> +
> *err_out = errno;
>
> return dir;
> @@ -112,9 +117,14 @@ char *read_dir(void *stream, unsigned long long *pos_out,
> DIR *dir = stream;
> struct dirent *ent;
>
> - ent = readdir(dir);
> + do {
> + errno = 0;
> + ent = readdir(dir);
> + } while (ent == 0 && errno == EINTR);
> +
> if (ent == NULL)
> return NULL;
> +
> *len_out = strlen(ent->d_name);
> *ino_out = ent->d_ino;
> *type_out = ent->d_type;
> @@ -125,30 +135,52 @@ char *read_dir(void *stream, unsigned long long *pos_out,
> int read_file(int fd, unsigned long long *offset, char *buf, int len)
> {
> int n;
> + int read = 0;
> +
> + do {
> + errno = 0;
> + n = pread64(fd, buf, len, *offset);
> + if (n > 0) {
> + read += n;
> + *offset += n;
> + len -= n;
> + continue;
> + }
> + } while (n < 0 && errno == EINTR);
>
> - n = pread64(fd, buf, len, *offset);
> - if (n < 0)
> - return -errno;
> - *offset += n;
> - return n;
> + if (read > 0)
> + return read;
> +
> + return -errno;
> }
>
> int write_file(int fd, unsigned long long *offset, const char *buf, int len)
> {
> int n;
> + int written = 0;
> +
> + do {
> + errno = 0;
> + n = pwrite64(fd, buf, len, *offset);
> + if (n > 0) {
> + written += n;
> + *offset += n;
> + len -= n;
> + continue;
> + }
> + } while (n < 0 && errno == EINTR);
>
> - n = pwrite64(fd, buf, len, *offset);
> - if (n < 0)
> - return -errno;
> - *offset += n;
> - return n;
> + if (written > 0)
> + return written;
> +
> + return -errno;
> }
>
> int lseek_file(int fd, long long offset, int whence)
> {
> int ret;
>
> - ret = lseek64(fd, offset, whence);
> + ret = CATCH_EINTR(lseek64(fd, offset, whence));
> if (ret < 0)
> return -errno;
> return 0;
> @@ -158,9 +190,9 @@ int fsync_file(int fd, int datasync)
> {
> int ret;
> if (datasync)
> - ret = fdatasync(fd);
> + ret = CATCH_EINTR(fdatasync(fd));
> else
> - ret = fsync(fd);
> + ret = CATCH_EINTR(fsync(fd));
>
> if (ret < 0)
> return -errno;
> @@ -169,24 +201,24 @@ int fsync_file(int fd, int datasync)
>
> int replace_file(int oldfd, int fd)
> {
> - return dup2(oldfd, fd);
> + return CATCH_EINTR(dup2(oldfd, fd));
> }
>
> void close_file(void *stream)
> {
> - close(*((int *) stream));
> + CATCH_EINTR(close(*((int *) stream)));
> }
>
> void close_dir(void *stream)
> {
> - closedir(stream);
> + CATCH_EINTR(closedir(stream));
> }
>
> int file_create(char *name, int mode)
> {
> int fd;
>
> - fd = open64(name, O_CREAT | O_RDWR, mode);
> + fd = CATCH_EINTR(open64(name, O_CREAT | O_RDWR, mode));
> if (fd < 0)
> return -errno;
> return fd;
> @@ -200,33 +232,33 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
>
> if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
> if (fd >= 0) {
> - if (fchmod(fd, attrs->ia_mode) != 0)
> + if (CATCH_EINTR(fchmod(fd, attrs->ia_mode)) != 0)
> return -errno;
> - } else if (chmod(file, attrs->ia_mode) != 0) {
> + } else if (CATCH_EINTR(chmod(file, attrs->ia_mode)) != 0) {
> return -errno;
> }
> }
> if (attrs->ia_valid & HOSTFS_ATTR_UID) {
> if (fd >= 0) {
> - if (fchown(fd, attrs->ia_uid, -1))
> + if (CATCH_EINTR(fchown(fd, attrs->ia_uid, -1)))
> return -errno;
> - } else if (chown(file, attrs->ia_uid, -1)) {
> + } else if (CATCH_EINTR(chown(file, attrs->ia_uid, -1))) {
> return -errno;
> }
> }
> if (attrs->ia_valid & HOSTFS_ATTR_GID) {
> if (fd >= 0) {
> - if (fchown(fd, -1, attrs->ia_gid))
> + if (CATCH_EINTR(fchown(fd, -1, attrs->ia_gid)))
> return -errno;
> - } else if (chown(file, -1, attrs->ia_gid)) {
> + } else if (CATCH_EINTR(chown(file, -1, attrs->ia_gid))) {
> return -errno;
> }
> }
> if (attrs->ia_valid & HOSTFS_ATTR_SIZE) {
> if (fd >= 0) {
> - if (ftruncate(fd, attrs->ia_size))
> + if (CATCH_EINTR(ftruncate(fd, attrs->ia_size)))
> return -errno;
> - } else if (truncate(file, attrs->ia_size)) {
> + } else if (CATCH_EINTR(truncate(file, attrs->ia_size))) {
> return -errno;
> }
> }
> @@ -279,7 +311,7 @@ int make_symlink(const char *from, const char *to)
> {
> int err;
>
> - err = symlink(to, from);
> + err = CATCH_EINTR(symlink(to, from));
> if (err)
> return -errno;
> return 0;
> @@ -289,7 +321,7 @@ int unlink_file(const char *file)
> {
> int err;
>
> - err = unlink(file);
> + err = CATCH_EINTR(unlink(file));
> if (err)
> return -errno;
> return 0;
> @@ -299,7 +331,7 @@ int do_mkdir(const char *file, int mode)
> {
> int err;
>
> - err = mkdir(file, mode);
> + err = CATCH_EINTR(mkdir(file, mode));
> if (err)
> return -errno;
> return 0;
> @@ -309,7 +341,7 @@ int hostfs_do_rmdir(const char *file)
> {
> int err;
>
> - err = rmdir(file);
> + err = CATCH_EINTR(rmdir(file));
> if (err)
> return -errno;
> return 0;
> @@ -319,7 +351,7 @@ int do_mknod(const char *file, int mode, unsigned int major, unsigned int minor)
> {
> int err;
>
> - err = mknod(file, mode, os_makedev(major, minor));
> + err = CATCH_EINTR(mknod(file, mode, os_makedev(major, minor)));
> if (err)
> return -errno;
> return 0;
> @@ -329,7 +361,7 @@ int link_file(const char *to, const char *from)
> {
> int err;
>
> - err = link(to, from);
> + err = CATCH_EINTR(link(to, from));
> if (err)
> return -errno;
> return 0;
> @@ -339,7 +371,7 @@ int hostfs_do_readlink(char *file, char *buf, int size)
> {
> int n;
>
> - n = readlink(file, buf, size);
> + n = CATCH_EINTR(readlink(file, buf, size));
> if (n < 0)
> return -errno;
> if (n < size)
> @@ -351,7 +383,7 @@ int rename_file(char *from, char *to)
> {
> int err;
>
> - err = rename(from, to);
> + err = CATCH_EINTR(rename(from, to));
> if (err < 0)
> return -errno;
> return 0;
> @@ -371,7 +403,8 @@ int rename2_file(char *from, char *to, unsigned int flags)
> #endif
>
> #ifdef SYS_renameat2
> - err = syscall(SYS_renameat2, AT_FDCWD, from, AT_FDCWD, to, flags);
> + err = CATCH_EINTR(syscall(SYS_renameat2, AT_FDCWD, from,
> + AT_FDCWD, to, flags));
> if (err < 0) {
> if (errno != ENOSYS)
> return -errno;
> @@ -392,7 +425,7 @@ int do_statfs(char *root, long *bsize_out, long long *blocks_out,
> struct statfs64 buf;
> int err;
>
> - err = statfs64(root, &buf);
> + err = CATCH_EINTR(statfs64(root, &buf));
> if (err < 0)
> return -errno;
>
If we are going to use this definition of CATCH_EINTR throughout we might as well remove the partial read/write code in UBD and other places.
--
Anton R. Ivanov
https://www.kot-begemot.co.uk/
More information about the linux-um
mailing list