| Message ID | 20190909162804.5694-1-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] Add proc interface to set PF_MEMALLOC flags | expand |
Forgot to cc linux-mm. On 09/09/2019 11:28 AM, Mike Christie wrote: > There are several storage drivers like dm-multipath, iscsi, and nbd that > have userspace components that can run in the IO path. For example, > iscsi and nbd's userspace deamons may need to recreate a socket and/or > send IO on it, and dm-multipath's daemon multipathd may need to send IO > to figure out the state of paths and re-set them up. > > In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the > memalloc_*_save/restore functions to control the allocation behavior, > but for userspace we would end up hitting a allocation that ended up > writing data back to the same device we are trying to allocate for. > > This patch allows the userspace deamon to set the PF_MEMALLOC* flags > through procfs. It currently only supports PF_MEMALLOC_NOIO, but > depending on what other drivers and userspace file systems need, for > the final version I can add the other flags for that file or do a file > per flag or just do a memalloc_noio file. > > Signed-off-by: Mike Christie <mchristi@redhat.com> > --- > Documentation/filesystems/proc.txt | 6 ++++ > fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt > index 99ca040e3f90..b5456a61a013 100644 > --- a/Documentation/filesystems/proc.txt > +++ b/Documentation/filesystems/proc.txt > @@ -46,6 +46,7 @@ Table of Contents > 3.10 /proc/<pid>/timerslack_ns - Task timerslack value > 3.11 /proc/<pid>/patch_state - Livepatch patch operation state > 3.12 /proc/<pid>/arch_status - Task architecture specific information > + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > > 4 Configuring procfs > 4.1 Mount options > @@ -1980,6 +1981,11 @@ Example > $ cat /proc/6753/arch_status > AVX512_elapsed_ms: 8 > > +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > +----------------------------------------------------------------------- > +A value of "noio" indicates that when a task allocates memory it will not > +reclaim memory that requires starting phisical IO. > + > Description > ----------- > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ebea9501afb8..c4faa3464602 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { > .llseek = default_llseek, > }; > > +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct task_struct *task; > + ssize_t rc = 0; > + > + task = get_proc_task(file_inode(file)); > + if (!task) > + return -ESRCH; > + > + if (task->flags & PF_MEMALLOC_NOIO) > + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); > + put_task_struct(task); > + return rc; > +} > + > +static ssize_t memalloc_write(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct task_struct *task; > + char buffer[5]; > + int rc = count; > + > + memset(buffer, 0, sizeof(buffer)); > + if (count != sizeof(buffer) - 1) > + return -EINVAL; > + > + if (copy_from_user(buffer, buf, count)) > + return -EFAULT; > + buffer[count] = '\0'; > + > + task = get_proc_task(file_inode(file)); > + if (!task) > + return -ESRCH; > + > + if (!strcmp(buffer, "noio")) { > + task->flags |= PF_MEMALLOC_NOIO; > + } else { > + rc = -EINVAL; > + } > + > + put_task_struct(task); > + return rc; > +} > + > +static const struct file_operations proc_memalloc_operations = { > + .read = memalloc_read, > + .write = memalloc_write, > + .llseek = default_llseek, > +}; > + > #ifdef CONFIG_AUDIT > #define TMPBUFLEN 11 > static ssize_t proc_loginuid_read(struct file * file, char __user * buf, > @@ -3097,6 +3148,7 @@ static const struct pid_entry tgid_base_stuff[] = { > #ifdef CONFIG_PROC_PID_ARCH_STATUS > ONE("arch_status", S_IRUGO, proc_pid_arch_status), > #endif > + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), > }; > > static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) > @@ -3487,6 +3539,7 @@ static const struct pid_entry tid_base_stuff[] = { > #ifdef CONFIG_PROC_PID_ARCH_STATUS > ONE("arch_status", S_IRUGO, proc_pid_arch_status), > #endif > + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), > }; > > static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx) >
Mike, On 2019/09/09 19:26, Mike Christie wrote: > Forgot to cc linux-mm. > > On 09/09/2019 11:28 AM, Mike Christie wrote: >> There are several storage drivers like dm-multipath, iscsi, and nbd that >> have userspace components that can run in the IO path. For example, >> iscsi and nbd's userspace deamons may need to recreate a socket and/or >> send IO on it, and dm-multipath's daemon multipathd may need to send IO >> to figure out the state of paths and re-set them up. >> >> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >> memalloc_*_save/restore functions to control the allocation behavior, >> but for userspace we would end up hitting a allocation that ended up >> writing data back to the same device we are trying to allocate for. >> >> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >> depending on what other drivers and userspace file systems need, for >> the final version I can add the other flags for that file or do a file >> per flag or just do a memalloc_noio file. Awesome. That probably will be the perfect solution for the problem we hit with tcmu-runner a while back (please see this thread: https://www.spinics.net/lists/linux-fsdevel/msg148912.html). I think we definitely need nofs as well for dealing with cases where the backend storage for the user daemon is a file. I will give this patch a try as soon as possible (I am traveling currently). Best regards. >> >> Signed-off-by: Mike Christie <mchristi@redhat.com> >> --- >> Documentation/filesystems/proc.txt | 6 ++++ >> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt >> index 99ca040e3f90..b5456a61a013 100644 >> --- a/Documentation/filesystems/proc.txt >> +++ b/Documentation/filesystems/proc.txt >> @@ -46,6 +46,7 @@ Table of Contents >> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value >> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state >> 3.12 /proc/<pid>/arch_status - Task architecture specific information >> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> >> 4 Configuring procfs >> 4.1 Mount options >> @@ -1980,6 +1981,11 @@ Example >> $ cat /proc/6753/arch_status >> AVX512_elapsed_ms: 8 >> >> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> +----------------------------------------------------------------------- >> +A value of "noio" indicates that when a task allocates memory it will not >> +reclaim memory that requires starting phisical IO. >> + >> Description >> ----------- >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index ebea9501afb8..c4faa3464602 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { >> .llseek = default_llseek, >> }; >> >> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, >> + loff_t *ppos) >> +{ >> + struct task_struct *task; >> + ssize_t rc = 0; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (task->flags & PF_MEMALLOC_NOIO) >> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); >> + put_task_struct(task); >> + return rc; >> +} >> + >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct task_struct *task; >> + char buffer[5]; >> + int rc = count; >> + >> + memset(buffer, 0, sizeof(buffer)); >> + if (count != sizeof(buffer) - 1) >> + return -EINVAL; >> + >> + if (copy_from_user(buffer, buf, count)) >> + return -EFAULT; >> + buffer[count] = '\0'; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (!strcmp(buffer, "noio")) { >> + task->flags |= PF_MEMALLOC_NOIO; >> + } else { >> + rc = -EINVAL; >> + } >> + >> + put_task_struct(task); >> + return rc; >> +} >> + >> +static const struct file_operations proc_memalloc_operations = { >> + .read = memalloc_read, >> + .write = memalloc_write, >> + .llseek = default_llseek, >> +}; >> + >> #ifdef CONFIG_AUDIT >> #define TMPBUFLEN 11 >> static ssize_t proc_loginuid_read(struct file * file, char __user * buf, >> @@ -3097,6 +3148,7 @@ static const struct pid_entry tgid_base_stuff[] = { >> #ifdef CONFIG_PROC_PID_ARCH_STATUS >> ONE("arch_status", S_IRUGO, proc_pid_arch_status), >> #endif >> + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), >> }; >> >> static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) >> @@ -3487,6 +3539,7 @@ static const struct pid_entry tid_base_stuff[] = { >> #ifdef CONFIG_PROC_PID_ARCH_STATUS >> ONE("arch_status", S_IRUGO, proc_pid_arch_status), >> #endif >> + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), >> }; >> >> static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx) >> > >
On Mon, Sep 09, 2019 at 11:28:04AM -0500, Mike Christie wrote: > There are several storage drivers like dm-multipath, iscsi, and nbd that > have userspace components that can run in the IO path. For example, > iscsi and nbd's userspace deamons may need to recreate a socket and/or > send IO on it, and dm-multipath's daemon multipathd may need to send IO > to figure out the state of paths and re-set them up. > > In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the > memalloc_*_save/restore functions to control the allocation behavior, > but for userspace we would end up hitting a allocation that ended up > writing data back to the same device we are trying to allocate for. > > This patch allows the userspace deamon to set the PF_MEMALLOC* flags > through procfs. It currently only supports PF_MEMALLOC_NOIO, but > depending on what other drivers and userspace file systems need, for > the final version I can add the other flags for that file or do a file > per flag or just do a memalloc_noio file. > > Signed-off-by: Mike Christie <mchristi@redhat.com> > --- > Documentation/filesystems/proc.txt | 6 ++++ > fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt > index 99ca040e3f90..b5456a61a013 100644 > --- a/Documentation/filesystems/proc.txt > +++ b/Documentation/filesystems/proc.txt > @@ -46,6 +46,7 @@ Table of Contents > 3.10 /proc/<pid>/timerslack_ns - Task timerslack value > 3.11 /proc/<pid>/patch_state - Livepatch patch operation state > 3.12 /proc/<pid>/arch_status - Task architecture specific information > + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > > 4 Configuring procfs > 4.1 Mount options > @@ -1980,6 +1981,11 @@ Example > $ cat /proc/6753/arch_status > AVX512_elapsed_ms: 8 > > +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > +----------------------------------------------------------------------- > +A value of "noio" indicates that when a task allocates memory it will not > +reclaim memory that requires starting phisical IO. > + > Description > ----------- > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index ebea9501afb8..c4faa3464602 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { > .llseek = default_llseek, > }; > > +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct task_struct *task; > + ssize_t rc = 0; > + > + task = get_proc_task(file_inode(file)); > + if (!task) > + return -ESRCH; > + > + if (task->flags & PF_MEMALLOC_NOIO) > + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); > + put_task_struct(task); > + return rc; > +} > + > +static ssize_t memalloc_write(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct task_struct *task; > + char buffer[5]; > + int rc = count; > + > + memset(buffer, 0, sizeof(buffer)); > + if (count != sizeof(buffer) - 1) > + return -EINVAL; > + > + if (copy_from_user(buffer, buf, count)) > + return -EFAULT; > + buffer[count] = '\0'; > + > + task = get_proc_task(file_inode(file)); > + if (!task) > + return -ESRCH; > + > + if (!strcmp(buffer, "noio")) { > + task->flags |= PF_MEMALLOC_NOIO; > + } else { > + rc = -EINVAL; > + } Really? Without any privilege check? So any random user can tap into __GFP_NOIO allocations? NAK. I don't think that it's great idea in general to expose this low-level machinery to userspace. But it's better to get comment from people move familiar with reclaim path.
On 2019/09/10 11:00, Kirill A. Shutemov wrote: > On Mon, Sep 09, 2019 at 11:28:04AM -0500, Mike Christie wrote: >> There are several storage drivers like dm-multipath, iscsi, and nbd that >> have userspace components that can run in the IO path. For example, >> iscsi and nbd's userspace deamons may need to recreate a socket and/or >> send IO on it, and dm-multipath's daemon multipathd may need to send IO >> to figure out the state of paths and re-set them up. >> >> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >> memalloc_*_save/restore functions to control the allocation behavior, >> but for userspace we would end up hitting a allocation that ended up >> writing data back to the same device we are trying to allocate for. >> >> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >> depending on what other drivers and userspace file systems need, for >> the final version I can add the other flags for that file or do a file >> per flag or just do a memalloc_noio file. >> >> Signed-off-by: Mike Christie <mchristi@redhat.com> >> --- >> Documentation/filesystems/proc.txt | 6 ++++ >> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt >> index 99ca040e3f90..b5456a61a013 100644 >> --- a/Documentation/filesystems/proc.txt >> +++ b/Documentation/filesystems/proc.txt >> @@ -46,6 +46,7 @@ Table of Contents >> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value >> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state >> 3.12 /proc/<pid>/arch_status - Task architecture specific information >> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> >> 4 Configuring procfs >> 4.1 Mount options >> @@ -1980,6 +1981,11 @@ Example >> $ cat /proc/6753/arch_status >> AVX512_elapsed_ms: 8 >> >> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> +----------------------------------------------------------------------- >> +A value of "noio" indicates that when a task allocates memory it will not >> +reclaim memory that requires starting phisical IO. >> + >> Description >> ----------- >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index ebea9501afb8..c4faa3464602 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { >> .llseek = default_llseek, >> }; >> >> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, >> + loff_t *ppos) >> +{ >> + struct task_struct *task; >> + ssize_t rc = 0; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (task->flags & PF_MEMALLOC_NOIO) >> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); >> + put_task_struct(task); >> + return rc; >> +} >> + >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct task_struct *task; >> + char buffer[5]; >> + int rc = count; >> + >> + memset(buffer, 0, sizeof(buffer)); >> + if (count != sizeof(buffer) - 1) >> + return -EINVAL; >> + >> + if (copy_from_user(buffer, buf, count)) >> + return -EFAULT; >> + buffer[count] = '\0'; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (!strcmp(buffer, "noio")) { >> + task->flags |= PF_MEMALLOC_NOIO; >> + } else { >> + rc = -EINVAL; >> + } > > Really? Without any privilege check? So any random user can tap into > __GFP_NOIO allocations? OK. It probably should have a test on capable(CAP_SYS_ADMIN) or similar. Since these storage daemons are generally run as root anyway, that would still work for most setup I think. > > NAK. > > I don't think that it's great idea in general to expose this low-level > machinery to userspace. But it's better to get comment from people move > familiar with reclaim path. Any setup with stacked file systems and one of the IO path component being a user level process can benefit from this. See the problem described in this patch I pushed for (unsuccessfully as it was a heavy handed solution): https://www.spinics.net/lists/linux-fsdevel/msg148912.html As the discussion in this thread shows, there is no existing simple solution to deal with this reclaim recursion problem. And automatic detection is too hard, if at all possible. With the proper access rights added, this user accessible interface does look very sensible to me. Best regards.
On Tue, Sep 10, 2019 at 12:05:33PM +0000, Damien Le Moal wrote: > On 2019/09/10 11:00, Kirill A. Shutemov wrote: > > On Mon, Sep 09, 2019 at 11:28:04AM -0500, Mike Christie wrote: > >> There are several storage drivers like dm-multipath, iscsi, and nbd that > >> have userspace components that can run in the IO path. For example, > >> iscsi and nbd's userspace deamons may need to recreate a socket and/or > >> send IO on it, and dm-multipath's daemon multipathd may need to send IO > >> to figure out the state of paths and re-set them up. > >> > >> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the > >> memalloc_*_save/restore functions to control the allocation behavior, > >> but for userspace we would end up hitting a allocation that ended up > >> writing data back to the same device we are trying to allocate for. > >> > >> This patch allows the userspace deamon to set the PF_MEMALLOC* flags > >> through procfs. It currently only supports PF_MEMALLOC_NOIO, but > >> depending on what other drivers and userspace file systems need, for > >> the final version I can add the other flags for that file or do a file > >> per flag or just do a memalloc_noio file. > >> > >> Signed-off-by: Mike Christie <mchristi@redhat.com> > >> --- > >> Documentation/filesystems/proc.txt | 6 ++++ > >> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ > >> 2 files changed, 59 insertions(+) > >> > >> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt > >> index 99ca040e3f90..b5456a61a013 100644 > >> --- a/Documentation/filesystems/proc.txt > >> +++ b/Documentation/filesystems/proc.txt > >> @@ -46,6 +46,7 @@ Table of Contents > >> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value > >> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state > >> 3.12 /proc/<pid>/arch_status - Task architecture specific information > >> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > >> > >> 4 Configuring procfs > >> 4.1 Mount options > >> @@ -1980,6 +1981,11 @@ Example > >> $ cat /proc/6753/arch_status > >> AVX512_elapsed_ms: 8 > >> > >> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior > >> +----------------------------------------------------------------------- > >> +A value of "noio" indicates that when a task allocates memory it will not > >> +reclaim memory that requires starting phisical IO. > >> + > >> Description > >> ----------- > >> > >> diff --git a/fs/proc/base.c b/fs/proc/base.c > >> index ebea9501afb8..c4faa3464602 100644 > >> --- a/fs/proc/base.c > >> +++ b/fs/proc/base.c > >> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { > >> .llseek = default_llseek, > >> }; > >> > >> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, > >> + loff_t *ppos) > >> +{ > >> + struct task_struct *task; > >> + ssize_t rc = 0; > >> + > >> + task = get_proc_task(file_inode(file)); > >> + if (!task) > >> + return -ESRCH; > >> + > >> + if (task->flags & PF_MEMALLOC_NOIO) > >> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); > >> + put_task_struct(task); > >> + return rc; > >> +} > >> + > >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, > >> + size_t count, loff_t *ppos) > >> +{ > >> + struct task_struct *task; > >> + char buffer[5]; > >> + int rc = count; > >> + > >> + memset(buffer, 0, sizeof(buffer)); > >> + if (count != sizeof(buffer) - 1) > >> + return -EINVAL; > >> + > >> + if (copy_from_user(buffer, buf, count)) > >> + return -EFAULT; > >> + buffer[count] = '\0'; > >> + > >> + task = get_proc_task(file_inode(file)); > >> + if (!task) > >> + return -ESRCH; > >> + > >> + if (!strcmp(buffer, "noio")) { > >> + task->flags |= PF_MEMALLOC_NOIO; > >> + } else { > >> + rc = -EINVAL; > >> + } > > > > Really? Without any privilege check? So any random user can tap into > > __GFP_NOIO allocations? > > OK. It probably should have a test on capable(CAP_SYS_ADMIN) or similar. Since > these storage daemons are generally run as root anyway, that would still work > for most setup I think. > > > > > NAK. > > > > I don't think that it's great idea in general to expose this low-level > > machinery to userspace. But it's better to get comment from people move > > familiar with reclaim path. > > Any setup with stacked file systems and one of the IO path component being a > user level process can benefit from this. See the problem described in this > patch I pushed for (unsuccessfully as it was a heavy handed solution): > https://www.spinics.net/lists/linux-fsdevel/msg148912.html > > As the discussion in this thread shows, there is no existing simple solution to > deal with this reclaim recursion problem. And automatic detection is too hard, > if at all possible. With the proper access rights added, this user accessible > interface does look very sensible to me. Looking into the thread, have you find out if there's anything on FUSE side that helps it to avoid deadlocks? Or FUSE just relies on luck with this?
+ Miklos On 2019/09/10 13:41, Kirill A. Shutemov wrote: > On Tue, Sep 10, 2019 at 12:05:33PM +0000, Damien Le Moal wrote: >> On 2019/09/10 11:00, Kirill A. Shutemov wrote: >>> On Mon, Sep 09, 2019 at 11:28:04AM -0500, Mike Christie wrote: >>>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>>> have userspace components that can run in the IO path. For example, >>>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>>> to figure out the state of paths and re-set them up. >>>> >>>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>>> memalloc_*_save/restore functions to control the allocation behavior, >>>> but for userspace we would end up hitting a allocation that ended up >>>> writing data back to the same device we are trying to allocate for. >>>> >>>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>>> depending on what other drivers and userspace file systems need, for >>>> the final version I can add the other flags for that file or do a file >>>> per flag or just do a memalloc_noio file. >>>> >>>> Signed-off-by: Mike Christie <mchristi@redhat.com> >>>> --- >>>> Documentation/filesystems/proc.txt | 6 ++++ >>>> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ >>>> 2 files changed, 59 insertions(+) >>>> >>>> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt >>>> index 99ca040e3f90..b5456a61a013 100644 >>>> --- a/Documentation/filesystems/proc.txt >>>> +++ b/Documentation/filesystems/proc.txt >>>> @@ -46,6 +46,7 @@ Table of Contents >>>> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value >>>> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state >>>> 3.12 /proc/<pid>/arch_status - Task architecture specific information >>>> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >>>> >>>> 4 Configuring procfs >>>> 4.1 Mount options >>>> @@ -1980,6 +1981,11 @@ Example >>>> $ cat /proc/6753/arch_status >>>> AVX512_elapsed_ms: 8 >>>> >>>> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >>>> +----------------------------------------------------------------------- >>>> +A value of "noio" indicates that when a task allocates memory it will not >>>> +reclaim memory that requires starting phisical IO. >>>> + >>>> Description >>>> ----------- >>>> >>>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>>> index ebea9501afb8..c4faa3464602 100644 >>>> --- a/fs/proc/base.c >>>> +++ b/fs/proc/base.c >>>> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { >>>> .llseek = default_llseek, >>>> }; >>>> >>>> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, >>>> + loff_t *ppos) >>>> +{ >>>> + struct task_struct *task; >>>> + ssize_t rc = 0; >>>> + >>>> + task = get_proc_task(file_inode(file)); >>>> + if (!task) >>>> + return -ESRCH; >>>> + >>>> + if (task->flags & PF_MEMALLOC_NOIO) >>>> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); >>>> + put_task_struct(task); >>>> + return rc; >>>> +} >>>> + >>>> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >>>> + size_t count, loff_t *ppos) >>>> +{ >>>> + struct task_struct *task; >>>> + char buffer[5]; >>>> + int rc = count; >>>> + >>>> + memset(buffer, 0, sizeof(buffer)); >>>> + if (count != sizeof(buffer) - 1) >>>> + return -EINVAL; >>>> + >>>> + if (copy_from_user(buffer, buf, count)) >>>> + return -EFAULT; >>>> + buffer[count] = '\0'; >>>> + >>>> + task = get_proc_task(file_inode(file)); >>>> + if (!task) >>>> + return -ESRCH; >>>> + >>>> + if (!strcmp(buffer, "noio")) { >>>> + task->flags |= PF_MEMALLOC_NOIO; >>>> + } else { >>>> + rc = -EINVAL; >>>> + } >>> >>> Really? Without any privilege check? So any random user can tap into >>> __GFP_NOIO allocations? >> >> OK. It probably should have a test on capable(CAP_SYS_ADMIN) or similar. Since >> these storage daemons are generally run as root anyway, that would still work >> for most setup I think. >> >>> >>> NAK. >>> >>> I don't think that it's great idea in general to expose this low-level >>> machinery to userspace. But it's better to get comment from people move >>> familiar with reclaim path. >> >> Any setup with stacked file systems and one of the IO path component being a >> user level process can benefit from this. See the problem described in this >> patch I pushed for (unsuccessfully as it was a heavy handed solution): >> https://www.spinics.net/lists/linux-fsdevel/msg148912.html >> >> As the discussion in this thread shows, there is no existing simple solution to >> deal with this reclaim recursion problem. And automatic detection is too hard, >> if at all possible. With the proper access rights added, this user accessible >> interface does look very sensible to me. > > Looking into the thread, have you find out if there's anything on FUSE > side that helps it to avoid deadlocks? Or FUSE just relies on luck with > this? I did not see anything relevant. The nofs allocations seem to all be in the writpage/writepages methods for the client side, to prepare requests to send to the fuse daemon serving them. I think that that is equivalent to a regular FS (e.g. XFS) using NOFS allocations during writeback on top of the emulated device served by a user level daemon (e.g. tcmu-runner in the problem case I reported). So it does look like a fuse daemon actually serving the request may still trigger a reclaim into the fuse FS. I wonder if such problem ever was reported or if there are some clever tricks I am missing. Miklos, Could you comment on this ? Is there a mechanism in fuse preventing the userspace fuse daemon memory triggering a reclaim into the fuse FS being processed ? Best regards.
On 09/10/2019 05:00 AM, Kirill A. Shutemov wrote: > On Mon, Sep 09, 2019 at 11:28:04AM -0500, Mike Christie wrote: >> There are several storage drivers like dm-multipath, iscsi, and nbd that >> have userspace components that can run in the IO path. For example, >> iscsi and nbd's userspace deamons may need to recreate a socket and/or >> send IO on it, and dm-multipath's daemon multipathd may need to send IO >> to figure out the state of paths and re-set them up. >> >> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >> memalloc_*_save/restore functions to control the allocation behavior, >> but for userspace we would end up hitting a allocation that ended up >> writing data back to the same device we are trying to allocate for. >> >> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >> depending on what other drivers and userspace file systems need, for >> the final version I can add the other flags for that file or do a file >> per flag or just do a memalloc_noio file. >> >> Signed-off-by: Mike Christie <mchristi@redhat.com> >> --- >> Documentation/filesystems/proc.txt | 6 ++++ >> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt >> index 99ca040e3f90..b5456a61a013 100644 >> --- a/Documentation/filesystems/proc.txt >> +++ b/Documentation/filesystems/proc.txt >> @@ -46,6 +46,7 @@ Table of Contents >> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value >> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state >> 3.12 /proc/<pid>/arch_status - Task architecture specific information >> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> >> 4 Configuring procfs >> 4.1 Mount options >> @@ -1980,6 +1981,11 @@ Example >> $ cat /proc/6753/arch_status >> AVX512_elapsed_ms: 8 >> >> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >> +----------------------------------------------------------------------- >> +A value of "noio" indicates that when a task allocates memory it will not >> +reclaim memory that requires starting phisical IO. >> + >> Description >> ----------- >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index ebea9501afb8..c4faa3464602 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { >> .llseek = default_llseek, >> }; >> >> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, >> + loff_t *ppos) >> +{ >> + struct task_struct *task; >> + ssize_t rc = 0; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (task->flags & PF_MEMALLOC_NOIO) >> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); >> + put_task_struct(task); >> + return rc; >> +} >> + >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct task_struct *task; >> + char buffer[5]; >> + int rc = count; >> + >> + memset(buffer, 0, sizeof(buffer)); >> + if (count != sizeof(buffer) - 1) >> + return -EINVAL; >> + >> + if (copy_from_user(buffer, buf, count)) >> + return -EFAULT; >> + buffer[count] = '\0'; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (!strcmp(buffer, "noio")) { >> + task->flags |= PF_MEMALLOC_NOIO; >> + } else { >> + rc = -EINVAL; >> + } > > Really? Without any privilege check? So any random user can tap into > __GFP_NOIO allocations? That was a mistake on my part. I will add it in.
On 2019/09/10 3:26, Mike Christie wrote: > Forgot to cc linux-mm. > > On 09/09/2019 11:28 AM, Mike Christie wrote: >> There are several storage drivers like dm-multipath, iscsi, and nbd that >> have userspace components that can run in the IO path. For example, >> iscsi and nbd's userspace deamons may need to recreate a socket and/or >> send IO on it, and dm-multipath's daemon multipathd may need to send IO >> to figure out the state of paths and re-set them up. >> >> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >> memalloc_*_save/restore functions to control the allocation behavior, >> but for userspace we would end up hitting a allocation that ended up >> writing data back to the same device we are trying to allocate for. >> >> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >> depending on what other drivers and userspace file systems need, for >> the final version I can add the other flags for that file or do a file >> per flag or just do a memalloc_noio file. Interesting patch. But can't we instead globally mask __GFP_NOFS / __GFP_NOIO than playing games with per a thread masking (which suffers from inability to propagate current thread's mask to other threads indirectly involved)? >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct task_struct *task; >> + char buffer[5]; >> + int rc = count; >> + >> + memset(buffer, 0, sizeof(buffer)); >> + if (count != sizeof(buffer) - 1) >> + return -EINVAL; >> + >> + if (copy_from_user(buffer, buf, count)) copy_from_user() / copy_to_user() might involve memory allocation via page fault which has to be done under the mask? Moreover, since just open()ing this file can involve memory allocation, do we forbid open("/proc/thread-self/memalloc") ? >> + return -EFAULT; >> + buffer[count] = '\0'; >> + >> + task = get_proc_task(file_inode(file)); >> + if (!task) >> + return -ESRCH; >> + >> + if (!strcmp(buffer, "noio")) { >> + task->flags |= PF_MEMALLOC_NOIO; >> + } else { >> + rc = -EINVAL; >> + } >> + >> + put_task_struct(task); >> + return rc; >> +}
On Wed, Sep 11, 2019 at 07:12:06AM +0900, Tetsuo Handa wrote: > >> +static ssize_t memalloc_write(struct file *file, const char __user *buf, > >> + size_t count, loff_t *ppos) > >> +{ > >> + struct task_struct *task; > >> + char buffer[5]; > >> + int rc = count; > >> + > >> + memset(buffer, 0, sizeof(buffer)); > >> + if (count != sizeof(buffer) - 1) > >> + return -EINVAL; > >> + > >> + if (copy_from_user(buffer, buf, count)) > > copy_from_user() / copy_to_user() might involve memory allocation > via page fault which has to be done under the mask? Moreover, since > just open()ing this file can involve memory allocation, do we forbid > open("/proc/thread-self/memalloc") ? Not saying that I'm okay with the approach in general, but I don't think this a problem. The application has to set allocation policy before inserting itself into IO or FS path.
On 9/9/19 5:28 PM, Mike Christie wrote: > There are several storage drivers like dm-multipath, iscsi, and nbd that > have userspace components that can run in the IO path. For example, > iscsi and nbd's userspace deamons may need to recreate a socket and/or > send IO on it, and dm-multipath's daemon multipathd may need to send IO > to figure out the state of paths and re-set them up. > > [ ... ] Should the linux-api mailing list be Cc-ed for a patch like this one? See also https://www.kernel.org/doc/man-pages/linux-api-ml.html. Thanks, Bart.
On 10.09.2019 10:35 Damien Le Moal wrote: > Mike, > > On 2019/09/09 19:26, Mike Christie wrote: >> Forgot to cc linux-mm. >> >> On 09/09/2019 11:28 AM, Mike Christie wrote: >>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>> have userspace components that can run in the IO path. For example, >>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>> to figure out the state of paths and re-set them up. >>> >>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>> memalloc_*_save/restore functions to control the allocation behavior, >>> but for userspace we would end up hitting a allocation that ended up >>> writing data back to the same device we are trying to allocate for. >>> >>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>> depending on what other drivers and userspace file systems need, for >>> the final version I can add the other flags for that file or do a file >>> per flag or just do a memalloc_noio file. > Awesome. That probably will be the perfect solution for the problem we hit with > tcmu-runner a while back (please see this thread: > https://www.spinics.net/lists/linux-fsdevel/msg148912.html). > > I think we definitely need nofs as well for dealing with cases where the backend > storage for the user daemon is a file. > > I will give this patch a try as soon as possible (I am traveling currently). > > Best regards. I had issues with this as well, and work on this is appreciated! In my case it is a loop block device on a fuse file system. Setting PF_LESS_THROTTLE was the one that helped the most, though, so add an option for that as well? I set this via prctl() for the thread calling it (was easiest to add to). Sorry, I have no idea about the current rationale, but wouldn't it be better to have a way to mask a set of block devices/file systems not to write-back to in a thread. So in my case I'd specify that the fuse daemon threads cannot write-back to the file system and loop device running on top of the fuse file system, while all other block devices/file systems can be write-back to (causing less swapping/OOM issues). > >>> Signed-off-by: Mike Christie <mchristi@redhat.com> >>> --- >>> Documentation/filesystems/proc.txt | 6 ++++ >>> fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ >>> 2 files changed, 59 insertions(+) >>> >>> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt >>> index 99ca040e3f90..b5456a61a013 100644 >>> --- a/Documentation/filesystems/proc.txt >>> +++ b/Documentation/filesystems/proc.txt >>> @@ -46,6 +46,7 @@ Table of Contents >>> 3.10 /proc/<pid>/timerslack_ns - Task timerslack value >>> 3.11 /proc/<pid>/patch_state - Livepatch patch operation state >>> 3.12 /proc/<pid>/arch_status - Task architecture specific information >>> + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >>> >>> 4 Configuring procfs >>> 4.1 Mount options >>> @@ -1980,6 +1981,11 @@ Example >>> $ cat /proc/6753/arch_status >>> AVX512_elapsed_ms: 8 >>> >>> +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior >>> +----------------------------------------------------------------------- >>> +A value of "noio" indicates that when a task allocates memory it will not >>> +reclaim memory that requires starting phisical IO. >>> + >>> Description >>> ----------- >>> >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index ebea9501afb8..c4faa3464602 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { >>> .llseek = default_llseek, >>> }; >>> >>> +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, >>> + loff_t *ppos) >>> +{ >>> + struct task_struct *task; >>> + ssize_t rc = 0; >>> + >>> + task = get_proc_task(file_inode(file)); >>> + if (!task) >>> + return -ESRCH; >>> + >>> + if (task->flags & PF_MEMALLOC_NOIO) >>> + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); >>> + put_task_struct(task); >>> + return rc; >>> +} >>> + >>> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct task_struct *task; >>> + char buffer[5]; >>> + int rc = count; >>> + >>> + memset(buffer, 0, sizeof(buffer)); >>> + if (count != sizeof(buffer) - 1) >>> + return -EINVAL; >>> + >>> + if (copy_from_user(buffer, buf, count)) >>> + return -EFAULT; >>> + buffer[count] = '\0'; >>> + >>> + task = get_proc_task(file_inode(file)); >>> + if (!task) >>> + return -ESRCH; >>> + >>> + if (!strcmp(buffer, "noio")) { >>> + task->flags |= PF_MEMALLOC_NOIO; >>> + } else { >>> + rc = -EINVAL; >>> + } >>> + >>> + put_task_struct(task); >>> + return rc; >>> +} >>> + >>> +static const struct file_operations proc_memalloc_operations = { >>> + .read = memalloc_read, >>> + .write = memalloc_write, >>> + .llseek = default_llseek, >>> +}; >>> + >>> #ifdef CONFIG_AUDIT >>> #define TMPBUFLEN 11 >>> static ssize_t proc_loginuid_read(struct file * file, char __user * buf, >>> @@ -3097,6 +3148,7 @@ static const struct pid_entry tgid_base_stuff[] = { >>> #ifdef CONFIG_PROC_PID_ARCH_STATUS >>> ONE("arch_status", S_IRUGO, proc_pid_arch_status), >>> #endif >>> + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), >>> }; >>> >>> static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) >>> @@ -3487,6 +3539,7 @@ static const struct pid_entry tid_base_stuff[] = { >>> #ifdef CONFIG_PROC_PID_ARCH_STATUS >>> ONE("arch_status", S_IRUGO, proc_pid_arch_status), >>> #endif >>> + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), >>> }; >>> >>> static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx) >>> >> >
On 09/10/2019 05:12 PM, Tetsuo Handa wrote: > On 2019/09/10 3:26, Mike Christie wrote: >> Forgot to cc linux-mm. >> >> On 09/09/2019 11:28 AM, Mike Christie wrote: >>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>> have userspace components that can run in the IO path. For example, >>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>> to figure out the state of paths and re-set them up. >>> >>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>> memalloc_*_save/restore functions to control the allocation behavior, >>> but for userspace we would end up hitting a allocation that ended up >>> writing data back to the same device we are trying to allocate for. >>> >>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>> depending on what other drivers and userspace file systems need, for >>> the final version I can add the other flags for that file or do a file >>> per flag or just do a memalloc_noio file. > > Interesting patch. But can't we instead globally mask __GFP_NOFS / __GFP_NOIO > than playing games with per a thread masking (which suffers from inability to > propagate current thread's mask to other threads indirectly involved)? If I understood you, then that had been discussed in the past: https://www.spinics.net/lists/linux-fsdevel/msg149035.html We only need this for specific threads which implement part of a storage driver in userspace. > >>> +static ssize_t memalloc_write(struct file *file, const char __user *buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct task_struct *task; >>> + char buffer[5]; >>> + int rc = count; >>> + >>> + memset(buffer, 0, sizeof(buffer)); >>> + if (count != sizeof(buffer) - 1) >>> + return -EINVAL; >>> + >>> + if (copy_from_user(buffer, buf, count)) > > copy_from_user() / copy_to_user() might involve memory allocation > via page fault which has to be done under the mask? Moreover, since > just open()ing this file can involve memory allocation, do we forbid > open("/proc/thread-self/memalloc") ? I was having the daemons set the flag when they initialize.
On 09/11/2019 03:40 AM, Martin Raiber wrote: > On 10.09.2019 10:35 Damien Le Moal wrote: >> Mike, >> >> On 2019/09/09 19:26, Mike Christie wrote: >>> Forgot to cc linux-mm. >>> >>> On 09/09/2019 11:28 AM, Mike Christie wrote: >>>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>>> have userspace components that can run in the IO path. For example, >>>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>>> to figure out the state of paths and re-set them up. >>>> >>>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>>> memalloc_*_save/restore functions to control the allocation behavior, >>>> but for userspace we would end up hitting a allocation that ended up >>>> writing data back to the same device we are trying to allocate for. >>>> >>>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>>> depending on what other drivers and userspace file systems need, for >>>> the final version I can add the other flags for that file or do a file >>>> per flag or just do a memalloc_noio file. >> Awesome. That probably will be the perfect solution for the problem we hit with >> tcmu-runner a while back (please see this thread: >> https://www.spinics.net/lists/linux-fsdevel/msg148912.html). >> >> I think we definitely need nofs as well for dealing with cases where the backend >> storage for the user daemon is a file. >> >> I will give this patch a try as soon as possible (I am traveling currently). >> >> Best regards. > > I had issues with this as well, and work on this is appreciated! In my > case it is a loop block device on a fuse file system. > Setting PF_LESS_THROTTLE was the one that helped the most, though, so > add an option for that as well? I set this via prctl() for the thread > calling it (was easiest to add to). > > Sorry, I have no idea about the current rationale, but wouldn't it be > better to have a way to mask a set of block devices/file systems not to > write-back to in a thread. So in my case I'd specify that the fuse > daemon threads cannot write-back to the file system and loop device > running on top of the fuse file system, while all other block > devices/file systems can be write-back to (causing less swapping/OOM > issues). I'm not sure I understood you. The storage daemons I mentioned normally kick off N threads per M devices. The threads handle duties like IO and error handling for those devices. Those threads would set the flag, so those IO/error-handler related operations do not end up writing back to them. So it works similar to how storage drivers work in the kernel where iscsi_tcp has an xmit thread and that does memalloc_noreclaim_save. Only the threads for those specific devices being would set the flag. In your case, it sounds like you have a thread/threads that would operate on multiple devices and some need the behavior and some do not. Is that right?
On 11.09.2019 18:56 Mike Christie wrote: > On 09/11/2019 03:40 AM, Martin Raiber wrote: >> On 10.09.2019 10:35 Damien Le Moal wrote: >>> Mike, >>> >>> On 2019/09/09 19:26, Mike Christie wrote: >>>> Forgot to cc linux-mm. >>>> >>>> On 09/09/2019 11:28 AM, Mike Christie wrote: >>>>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>>>> have userspace components that can run in the IO path. For example, >>>>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>>>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>>>> to figure out the state of paths and re-set them up. >>>>> >>>>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>>>> memalloc_*_save/restore functions to control the allocation behavior, >>>>> but for userspace we would end up hitting a allocation that ended up >>>>> writing data back to the same device we are trying to allocate for. >>>>> >>>>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>>>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>>>> depending on what other drivers and userspace file systems need, for >>>>> the final version I can add the other flags for that file or do a file >>>>> per flag or just do a memalloc_noio file. >>> Awesome. That probably will be the perfect solution for the problem we hit with >>> tcmu-runner a while back (please see this thread: >>> https://www.spinics.net/lists/linux-fsdevel/msg148912.html). >>> >>> I think we definitely need nofs as well for dealing with cases where the backend >>> storage for the user daemon is a file. >>> >>> I will give this patch a try as soon as possible (I am traveling currently). >>> >>> Best regards. >> I had issues with this as well, and work on this is appreciated! In my >> case it is a loop block device on a fuse file system. >> Setting PF_LESS_THROTTLE was the one that helped the most, though, so >> add an option for that as well? I set this via prctl() for the thread >> calling it (was easiest to add to). >> >> Sorry, I have no idea about the current rationale, but wouldn't it be >> better to have a way to mask a set of block devices/file systems not to >> write-back to in a thread. So in my case I'd specify that the fuse >> daemon threads cannot write-back to the file system and loop device >> running on top of the fuse file system, while all other block >> devices/file systems can be write-back to (causing less swapping/OOM >> issues). > I'm not sure I understood you. > > The storage daemons I mentioned normally kick off N threads per M > devices. The threads handle duties like IO and error handling for those > devices. Those threads would set the flag, so those IO/error-handler > related operations do not end up writing back to them. So it works > similar to how storage drivers work in the kernel where iscsi_tcp has an > xmit thread and that does memalloc_noreclaim_save. Only the threads for > those specific devices being would set the flag. > > In your case, it sounds like you have a thread/threads that would > operate on multiple devices and some need the behavior and some do not. > Is that right? No, sounds the same as your case. As an example think of vdfuse (or qemu-nbd locally). You'd have something like ext4(a) <- loop <- fuse file system <- vdfuse <- disk.vdi container file <- ext4(b) <- block device If vdfuse threads cause writeback to ext4(a), you'd get the issue we have. Setting PF_LESS_THROTTLE and/or PF_MEMALLOC_NOIO mostly avoids this problem, but with only PF_LESS_THROTTLE there are still corner cases (I think if ext4(b) slows down suddenly) where it wedges itself and the side effect of setting PF_MEMALLOC_NOIO are being discussed... The best solution would be, I guess, to have a way for vdfuse to set something, such that write-back to ext4(a) isn't allowed from those threads, but write-back to ext4(b) (and all other block devices) is. But I only have a rough idea of how write-back works, so this is really only a guess.
On 09/11/2019 02:21 PM, Martin Raiber wrote: > On 11.09.2019 18:56 Mike Christie wrote: >> On 09/11/2019 03:40 AM, Martin Raiber wrote: >>> On 10.09.2019 10:35 Damien Le Moal wrote: >>>> Mike, >>>> >>>> On 2019/09/09 19:26, Mike Christie wrote: >>>>> Forgot to cc linux-mm. >>>>> >>>>> On 09/09/2019 11:28 AM, Mike Christie wrote: >>>>>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>>>>> have userspace components that can run in the IO path. For example, >>>>>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>>>>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>>>>> to figure out the state of paths and re-set them up. >>>>>> >>>>>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>>>>> memalloc_*_save/restore functions to control the allocation behavior, >>>>>> but for userspace we would end up hitting a allocation that ended up >>>>>> writing data back to the same device we are trying to allocate for. >>>>>> >>>>>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>>>>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>>>>> depending on what other drivers and userspace file systems need, for >>>>>> the final version I can add the other flags for that file or do a file >>>>>> per flag or just do a memalloc_noio file. >>>> Awesome. That probably will be the perfect solution for the problem we hit with >>>> tcmu-runner a while back (please see this thread: >>>> https://www.spinics.net/lists/linux-fsdevel/msg148912.html). >>>> >>>> I think we definitely need nofs as well for dealing with cases where the backend >>>> storage for the user daemon is a file. >>>> >>>> I will give this patch a try as soon as possible (I am traveling currently). >>>> >>>> Best regards. >>> I had issues with this as well, and work on this is appreciated! In my >>> case it is a loop block device on a fuse file system. >>> Setting PF_LESS_THROTTLE was the one that helped the most, though, so >>> add an option for that as well? I set this via prctl() for the thread >>> calling it (was easiest to add to). >>> >>> Sorry, I have no idea about the current rationale, but wouldn't it be >>> better to have a way to mask a set of block devices/file systems not to >>> write-back to in a thread. So in my case I'd specify that the fuse >>> daemon threads cannot write-back to the file system and loop device >>> running on top of the fuse file system, while all other block >>> devices/file systems can be write-back to (causing less swapping/OOM >>> issues). >> I'm not sure I understood you. >> >> The storage daemons I mentioned normally kick off N threads per M >> devices. The threads handle duties like IO and error handling for those >> devices. Those threads would set the flag, so those IO/error-handler >> related operations do not end up writing back to them. So it works >> similar to how storage drivers work in the kernel where iscsi_tcp has an >> xmit thread and that does memalloc_noreclaim_save. Only the threads for >> those specific devices being would set the flag. >> >> In your case, it sounds like you have a thread/threads that would >> operate on multiple devices and some need the behavior and some do not. >> Is that right? > > No, sounds the same as your case. As an example think of vdfuse (or > qemu-nbd locally). You'd have something like > > ext4(a) <- loop <- fuse file system <- vdfuse <- disk.vdi container file > <- ext4(b) <- block device > > If vdfuse threads cause writeback to ext4(a), you'd get the issue we > have. Setting PF_LESS_THROTTLE and/or PF_MEMALLOC_NOIO mostly avoids > this problem, but with only PF_LESS_THROTTLE there are still corner > cases (I think if ext4(b) slows down suddenly) where it wedges itself > and the side effect of setting PF_MEMALLOC_NOIO are being discussed... > The best solution would be, I guess, to have a way for vdfuse to set > something, such that write-back to ext4(a) isn't allowed from those > threads, but write-back to ext4(b) (and all other block devices) is. But > I only have a rough idea of how write-back works, so this is really only > a guess. I see now. Initially, would it be ok to keep it simple and keep the existing kernel behavior? For your example, is the PF_MEMALLOC_NOIO use in loop today causing a lot of swap/oom issues? For iscsi_tcp and nbd their memalloc and GFP_NOIO use is not. The problem for the storage driver daemons I mentioned in the patch is that they are at the bottom of the stack and they do not know what is going to be added above them plus it can change, so we will have to walk the storage device stack while IO is running and allocations are trying to execute. It looks like I will end up having to insert extra locking/refcounts into multiple layers, and I am not sure if the extra complexity is going to be worth it if we are not seeing problems from existing kernel users.
On 09/12/2019 11:22 AM, Mike Christie wrote: > On 09/11/2019 02:21 PM, Martin Raiber wrote: >> On 11.09.2019 18:56 Mike Christie wrote: >>> On 09/11/2019 03:40 AM, Martin Raiber wrote: >>>> On 10.09.2019 10:35 Damien Le Moal wrote: >>>>> Mike, >>>>> >>>>> On 2019/09/09 19:26, Mike Christie wrote: >>>>>> Forgot to cc linux-mm. >>>>>> >>>>>> On 09/09/2019 11:28 AM, Mike Christie wrote: >>>>>>> There are several storage drivers like dm-multipath, iscsi, and nbd that >>>>>>> have userspace components that can run in the IO path. For example, >>>>>>> iscsi and nbd's userspace deamons may need to recreate a socket and/or >>>>>>> send IO on it, and dm-multipath's daemon multipathd may need to send IO >>>>>>> to figure out the state of paths and re-set them up. >>>>>>> >>>>>>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the >>>>>>> memalloc_*_save/restore functions to control the allocation behavior, >>>>>>> but for userspace we would end up hitting a allocation that ended up >>>>>>> writing data back to the same device we are trying to allocate for. >>>>>>> >>>>>>> This patch allows the userspace deamon to set the PF_MEMALLOC* flags >>>>>>> through procfs. It currently only supports PF_MEMALLOC_NOIO, but >>>>>>> depending on what other drivers and userspace file systems need, for >>>>>>> the final version I can add the other flags for that file or do a file >>>>>>> per flag or just do a memalloc_noio file. >>>>> Awesome. That probably will be the perfect solution for the problem we hit with >>>>> tcmu-runner a while back (please see this thread: >>>>> https://www.spinics.net/lists/linux-fsdevel/msg148912.html). >>>>> >>>>> I think we definitely need nofs as well for dealing with cases where the backend >>>>> storage for the user daemon is a file. >>>>> >>>>> I will give this patch a try as soon as possible (I am traveling currently). >>>>> >>>>> Best regards. >>>> I had issues with this as well, and work on this is appreciated! In my >>>> case it is a loop block device on a fuse file system. >>>> Setting PF_LESS_THROTTLE was the one that helped the most, though, so >>>> add an option for that as well? I set this via prctl() for the thread >>>> calling it (was easiest to add to). >>>> >>>> Sorry, I have no idea about the current rationale, but wouldn't it be >>>> better to have a way to mask a set of block devices/file systems not to >>>> write-back to in a thread. So in my case I'd specify that the fuse >>>> daemon threads cannot write-back to the file system and loop device >>>> running on top of the fuse file system, while all other block >>>> devices/file systems can be write-back to (causing less swapping/OOM >>>> issues). >>> I'm not sure I understood you. >>> >>> The storage daemons I mentioned normally kick off N threads per M >>> devices. The threads handle duties like IO and error handling for those >>> devices. Those threads would set the flag, so those IO/error-handler >>> related operations do not end up writing back to them. So it works >>> similar to how storage drivers work in the kernel where iscsi_tcp has an >>> xmit thread and that does memalloc_noreclaim_save. Only the threads for >>> those specific devices being would set the flag. >>> >>> In your case, it sounds like you have a thread/threads that would >>> operate on multiple devices and some need the behavior and some do not. >>> Is that right? >> >> No, sounds the same as your case. As an example think of vdfuse (or >> qemu-nbd locally). You'd have something like >> >> ext4(a) <- loop <- fuse file system <- vdfuse <- disk.vdi container file >> <- ext4(b) <- block device >> >> If vdfuse threads cause writeback to ext4(a), you'd get the issue we >> have. Setting PF_LESS_THROTTLE and/or PF_MEMALLOC_NOIO mostly avoids >> this problem, but with only PF_LESS_THROTTLE there are still corner >> cases (I think if ext4(b) slows down suddenly) where it wedges itself >> and the side effect of setting PF_MEMALLOC_NOIO are being discussed... >> The best solution would be, I guess, to have a way for vdfuse to set >> something, such that write-back to ext4(a) isn't allowed from those >> threads, but write-back to ext4(b) (and all other block devices) is. But >> I only have a rough idea of how write-back works, so this is really only >> a guess. > > I see now. > > Initially, would it be ok to keep it simple and keep the existing kernel > behavior? For your example, is the PF_MEMALLOC_NOIO use in loop today Or do it in two stages. 1. For devices like mine, we just use the existing behavior where it gets set for the thread and is for all devices. We know from iscsi/nbd it is already ok from their kernel use. I do not need to add any extra locking/complexity to the block, vm, fs code. 2. We can then add the ability to pass in a mount or upper layer block device for setups like yours where we already know the topology and it isn't going to change. > causing a lot of swap/oom issues? For iscsi_tcp and nbd their memalloc > and GFP_NOIO use is not. > > The problem for the storage driver daemons I mentioned in the patch is > that they are at the bottom of the stack and they do not know what is > going to be added above them plus it can change, so we will have to walk > the storage device stack while IO is running and allocations are trying > to execute. It looks like I will end up having to insert extra > locking/refcounts into multiple layers, and I am not sure if the extra > complexity is going to be worth it if we are not seeing problems from > existing kernel users. >
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt index 99ca040e3f90..b5456a61a013 100644 --- a/Documentation/filesystems/proc.txt +++ b/Documentation/filesystems/proc.txt @@ -46,6 +46,7 @@ Table of Contents 3.10 /proc/<pid>/timerslack_ns - Task timerslack value 3.11 /proc/<pid>/patch_state - Livepatch patch operation state 3.12 /proc/<pid>/arch_status - Task architecture specific information + 3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior 4 Configuring procfs 4.1 Mount options @@ -1980,6 +1981,11 @@ Example $ cat /proc/6753/arch_status AVX512_elapsed_ms: 8 +3.13 /proc/<pid>/memalloc - Control task's memory reclaim behavior +----------------------------------------------------------------------- +A value of "noio" indicates that when a task allocates memory it will not +reclaim memory that requires starting phisical IO. + Description ----------- diff --git a/fs/proc/base.c b/fs/proc/base.c index ebea9501afb8..c4faa3464602 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1223,6 +1223,57 @@ static const struct file_operations proc_oom_score_adj_operations = { .llseek = default_llseek, }; +static ssize_t memalloc_read(struct file *file, char __user *buf, size_t count, + loff_t *ppos) +{ + struct task_struct *task; + ssize_t rc = 0; + + task = get_proc_task(file_inode(file)); + if (!task) + return -ESRCH; + + if (task->flags & PF_MEMALLOC_NOIO) + rc = simple_read_from_buffer(buf, count, ppos, "noio", 4); + put_task_struct(task); + return rc; +} + +static ssize_t memalloc_write(struct file *file, const char __user *buf, + size_t count, loff_t *ppos) +{ + struct task_struct *task; + char buffer[5]; + int rc = count; + + memset(buffer, 0, sizeof(buffer)); + if (count != sizeof(buffer) - 1) + return -EINVAL; + + if (copy_from_user(buffer, buf, count)) + return -EFAULT; + buffer[count] = '\0'; + + task = get_proc_task(file_inode(file)); + if (!task) + return -ESRCH; + + if (!strcmp(buffer, "noio")) { + task->flags |= PF_MEMALLOC_NOIO; + } else { + rc = -EINVAL; + } + + put_task_struct(task); + return rc; +} + +static const struct file_operations proc_memalloc_operations = { + .read = memalloc_read, + .write = memalloc_write, + .llseek = default_llseek, +}; + #ifdef CONFIG_AUDIT #define TMPBUFLEN 11 static ssize_t proc_loginuid_read(struct file * file, char __user * buf, @@ -3097,6 +3148,7 @@ static const struct pid_entry tgid_base_stuff[] = { #ifdef CONFIG_PROC_PID_ARCH_STATUS ONE("arch_status", S_IRUGO, proc_pid_arch_status), #endif + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), }; static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx) @@ -3487,6 +3539,7 @@ static const struct pid_entry tid_base_stuff[] = { #ifdef CONFIG_PROC_PID_ARCH_STATUS ONE("arch_status", S_IRUGO, proc_pid_arch_status), #endif + REG("memalloc", S_IRUGO|S_IWUSR, proc_memalloc_operations), }; static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
There are several storage drivers like dm-multipath, iscsi, and nbd that have userspace components that can run in the IO path. For example, iscsi and nbd's userspace deamons may need to recreate a socket and/or send IO on it, and dm-multipath's daemon multipathd may need to send IO to figure out the state of paths and re-set them up. In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the memalloc_*_save/restore functions to control the allocation behavior, but for userspace we would end up hitting a allocation that ended up writing data back to the same device we are trying to allocate for. This patch allows the userspace deamon to set the PF_MEMALLOC* flags through procfs. It currently only supports PF_MEMALLOC_NOIO, but depending on what other drivers and userspace file systems need, for the final version I can add the other flags for that file or do a file per flag or just do a memalloc_noio file. Signed-off-by: Mike Christie <mchristi@redhat.com> --- Documentation/filesystems/proc.txt | 6 ++++ fs/proc/base.c | 53 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+)