[PATCH] um: hostfs: catch EINTR and partial read/write
Anton Ivanov
anton.ivanov at kot-begemot.co.uk
Fri Nov 10 02:56:30 PST 2023
On 10/11/2023 10:42, Anton Ivanov wrote:
>
>
> 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.
>
Actually - some of it. In some places it will still be needed (trying to get my head around where this will work and where not).
--
Anton R. Ivanov
https://www.kot-begemot.co.uk/
More information about the linux-um
mailing list