deferring __fput()
Al,
I really appreciate all of the work that has gone into making __fput() lockless - making the syscalls use get/put_light(), signal changes, and using Oleg's task_work_add() to defer processes.
Currently, when unmapping a file after closing it, I'm not getting the mmap_sem/i_mutex lockdep any more. This seems due to Tyler Hick's commit 978d6d8 "vfs: Correctly set the dir i_mutex lockdep class", rather than the above changes.
Although the mmap_sem/i_mutex lockdep doesn't exist anymore, before upstreaming IMA-appraisal, the issue with __fput() should probably be
resolved. :) What else needs to be done to make __fput() lockless?
How can I help?
thanks,
Mimi
Al Viro
maintainerQuoted 11 lines · Mimi Zohar
Al,
I really appreciate all of the work that has gone into making
__fput()lockless - making the syscalls use get/put_light(), signal changes, and using Oleg'stask_work_add()to defer processes.Currently, when unmapping a file after closing it, I'm not getting the mmap_sem/i_mutex lockdep any more. This seems due to Tyler Hick's commit 978d6d8 "vfs: Correctly set the dir
i_mutexlockdep class", rather than the above changes.
Quoted 4 lines
Although the mmap_sem/i_mutex lockdep doesn't exist anymore, before upstreaming IMA-appraisal, the issue with
__fput()should probably be
resolved. :) What else needs to be done to make__fput()lockless?
How can I help?
Deadlock is still there. I'll resurrect the patch switching the final fput() to task_work_add() and throw it into #for-next tomorrow; the main question is what to do with fput() from kernel threads and whether we want to allow it from interrupts - solution for the former (definitely needed) would cover the latter as well. I suspect that we want to go for "just use schedule_work()" for these cases; among other things, that would allow to simplify aio.c quite a bit.
Somewhat curious part is what to do with daemonize() - by the time we do (potentially) piles of fput() in there, we *are* a kernel thread. We won't be doing any task_work() handling anymore. So it's "do the final fput() ourselves" or "have them done asynchronously". OTOH, daemonize() probably needs to be killed anyway - it had all callers in mainline removed, only to get one more in drivers/staging. Which caller looks like it ought to be switched to kthread_create(), along with other callers of kernel_thread() in there...
What I have in mind is something like
if (unlikely(in_atomic() || current->flags & PF_KTHREAD))
move file to global list, use schedul_work() to have it dealt with
else
move to per-task list, do task_work_add() if it was empty (i.e. if we hadn't scheduled its emptying already).
The latter is completely thread-synchronous, so we shouldn't need any locking whatsoever. The former... I'd probably go for a single list, protected by spin_lock_irq(), keeping the possibility to switch to per-CPU lists if we find a load that gets serious contention on that list. In any case, worker will start with taking the list contents, emptying the list and then killing the suckers off one by one.
Al Viro
maintainerQuoted 4 lines · Al Viro
What I have in mind is something like
if (unlikely(in_atomic() || current->flags & PF_KTHREAD))
move file to global list, useschedul_work()to have it dealt with
else
Quoted 8 lines
move to per-task list, do
task_work_add()if it was empty (i.e. if we hadn't scheduled its emptying already).
The latter is completely thread-synchronous, so we shouldn't need any locking whatsoever. The former... I'd probably go for a single list, protected byspin_lock_irq(), keeping the possibility to switch to per-CPU lists if we find a load that gets serious contention on that list. In any case, worker will start with taking the list contents, emptying the list and then killing the suckers off one by one.
BTW, I really wonder why do we need to have that void *data in task_work; we can always embed the sucker into a bigger struct (if nothing else, task_work + void *data) and get to it via container_of(). And in quite a few cases we don't want that data thing at all. Moreover, the reasons to use hlist_head instead of a single forward pointer are very thin on the ground:
* task_work_add() adds to the beginning of the list
* task_work_cancel() walks the list to find the entry to remove
* trask_work_run() takes the list out, walks it to the end,
then walks it backwards via pprev. Could as well reverse the pointers while walking forward...
Oleg, do you see any reasons why trimming it down to forward pointer + callback pointer wouldn't work? Matter of fact, it would become identical to struct rcu_head after that...
Quoted 5 lines · Al Viro
BTW, I really wonder why do we need to have that void *data in
task_work; we can always embed the sucker into a bigger struct (if nothing else,task_work+ void *data) and get to it viacontainer_of(). And in quite a few cases we don't want that data thing at all.
Yes, it is not strictly needed. From the changelog:
"struct task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.
Namely, for keyctl_session_to_parent(). Probably it has ->data just because I failed to invent the good name for the struct with task_work + cred.
Quoted 2 lines
Moreover, the reasons to use
hlist_headinstead of a single forward pointer are very thin on the ground:
Oh, yes, there is no any reason. Except the code looks a bit simpler.
Quoted 2 lines
Oleg, do you see any reasons why trimming it down to forward pointer + callback pointer wouldn't work?
OK. will do.
Quoted 2 lines
Matter of fact, it would become identical to struct
rcu_headafter that...
This is not clear to me... Why this is good?
I understand that sizeof(task_work) == sizeof(rcu_head) would be nice, probably you meant just this?
Oleg.
Al Viro
maintainerQuoted 4 lines · Oleg Nesterov
Quoted 2 lines
Matter of fact, it would become identical to struct
rcu_headafter that...This is not clear to me... Why this is good?
Occam's Razor.
Quoted 2 lines
I understand that
sizeof(task_work)==sizeof(rcu_head)would be nice, probably you meant just this?
More than that - the callback type is also the same (pointer to such struct -> void). IOW, they both look like two instances of the same thing ("list of callbacks"), differing only in what and when does calling.
Al Viro
maintainerQuoted 9 lines · Al Viro
Quoted 4 lines · Oleg Nesterov
Quoted 2 lines
Matter of fact, it would become identical to struct
rcu_headafter that...This is not clear to me... Why this is good?
Occam's Razor.
Quoted 7 lines
Quoted 2 lines
I understand that
sizeof(task_work)==sizeof(rcu_head)would be nice, probably you meant just this?More than that - the callback type is also the same (pointer to such struct -> void). IOW, they both look like two instances of the same thing ("list of callbacks"), differing only in what and when does calling.
BTW, scratch that "task_work + cred" - cred has rcu_head in it. So we can put a union in there and slap the trimmed task_work into it. Voila - there goes separate allocation and *any* need for ->data. It's just container_of(), in that case as well.
Al Viro
maintainerQuoted 9 lines · Al Viro
Quoted 4 lines · Oleg Nesterov
Quoted 2 lines
Matter of fact, it would become identical to struct
rcu_headafter that...This is not clear to me... Why this is good?
Occam's Razor.
Quoted 7 lines
Quoted 2 lines
I understand that
sizeof(task_work)==sizeof(rcu_head)would be nice, probably you meant just this?More than that - the callback type is also the same (pointer to such struct -> void). IOW, they both look like two instances of the same thing ("list of callbacks"), differing only in what and when does calling.
BTW, I suspect that we really want to move exit_task_work() down past the calls of exit_mm()/exit_files() (and lose the PF_EXITING check in task_work_add(), making that ordering responsibility of callers). It's not strictly necessary - we can just treat PF_EXITING the same way we treat PF_KTHREAD, but that means driving those final fput on exit through schedule_work(). Extra context switch...
I'm not 100% sure about that one - if you have planned task_work users relying on e.g. task->mm still being there when callback runs, we obviously can't go that way, but it would be nice to have. AFAICS, existing users are fine with such reordering.
We could, in principle, add a "ok_late" argument, allowing to add after PF_EXITING has been set only if it's true and run the list twice, but that's really more convoluted than I would like...
Comments?
Al Viro
maintainerQuoted 5 lines · Al Viro
We could, in principle, add a "
ok_late" argument, allowing to add afterPF_EXITINGhas been set only if it's true and run the list twice, but that's really more convoluted than I would like...Comments?
OK... What I had in mind is (modulo really dire need of saner commit messages and probably a different ordering/splitup of the first 3 commits) is in vfs.git#untested. WARNING: the branch name is no joke; it builds, but I hadn't even tried to boot the resulting kernel yet.
Comments would be very welcome. It should get us the situation when
* __fput() is always called with no locks held by caller and
can take any locks whatsoever.
* fput() is legal to call from any contexts
* fput() done by a syscall will be completed before the process
returns to userland or terminates
* no extra context switches, unless we have the final fput() done
from interrupt (instant death on the current kernel) or from the kernel thread.
* SCM_RIGHTS datagram destruction should be no worse than it is now;
probably a bit kinder on stack, even... Again, no extra context switches.
* Neither struct file nor struct task_struct changed size.
* task_work and rcu_head are identical at that point; I'd appreciate
a better name (I ended up calling that sucker callback_head, defined in types.h, with #define rcu_head callback_head next to it, to avoid global rename from hell). I can live with two identical structs (and a union of those two in a few places), but I really see no point in going that way.
Quoted 2 lines · Al Viro
in vfs.git#untested.
include/linux/types.h:
#define rcu_head callback_head
OK, now I understand why did you want task_work/rcu identical.
So, you are not going to add a rcu/task_work union in (say) struct cred, task_work_add() should "silently" reuse cred->rcu.
OK, perhaps this makes more sense...
Oleg.
Quoted 3 lines · Al Viro
BTW, I suspect that we really want to move
exit_task_work()down past the calls of exit_mm()/exit_files()
Yes, probably. But I do not know how far we should move it.
Quoted 2 lines
(and lose the
PF_EXITINGcheck intask_work_add(), making that ordering responsibility of callers).
No, we can't do this?
OK, perhaps we can check something else instead of PF_EXITING. But somehow we should ensuree that if task_work_add(twork) succeeds, then twork->func() will be called. IOW, if task_work_add() races with the exiting task, it should not succeed after exit_task_work().
Quoted 2 lines
I'm not 100% sure about that one - if you have planned
task_workusers relying on e.g.task->mmstill being there when callback runs,
No, I didn't.
Oleg.
Al Viro
maintainerQuoted 6 lines · Oleg Nesterov
No, we can't do this?
OK, perhaps we can check something else instead of
PF_EXITING. But somehow we should ensuree that iftask_work_add(twork)succeeds, thentwork->func()will be called. IOW, iftask_work_add()races with the exiting task, it should not succeed afterexit_task_work().
Hrm... I still think that callers can bloody well check it themselves, but anyway - we can add a new PF_... bit and have it set on kernel threads (all along) and during exit_task_work(); the real question is in locking and barriers needed there. Suggestions?
Quoted 10 lines · Al Viro
Quoted 6 lines · Oleg Nesterov
No, we can't do this?
OK, perhaps we can check something else instead of
PF_EXITING. But somehow we should ensuree that iftask_work_add(twork)succeeds, thentwork->func()will be called. IOW, iftask_work_add()races with the exiting task, it should not succeed afterexit_task_work().Hrm... I still think that callers can bloody well check it themselves,
Why? I don't think this would be very convenient, and it is not easy to avoid the races. Unless task == current.
OK, if task == current it can do the necessary checks, so we could add "force" argument for fput(). But I agree, it would be better to avoid this.
And since we want to move exit_task_work() after exit_fs() we can't rely on PF_EXITING (unless we add "force").
Quoted 2 lines
but anyway - we can add a new
PF_... bit and have it set on kernel threads (all along)
Why? irq_thread() already uses task_work_add()...
Quoted 2 lines
the real question is in locking
and barriers needed there. Suggestions?
Yes, we need more barries. Or, perhaps exit_task_work() should simply take ->pi_lock unconditionally? I don't think additional STORE + mb() is better.
And if it always takes ->pi_lock we do not need the new PF_ or something else, exit_task_work() can set task->task_works = NO_MORE under ->pi_lock (task_work_run() can check PF_EXITING), and task_work_add() ensures that task_works != NO_MORE.
What do you think?
Oleg.
cover Was: deferring __fput()
Quoted 7 lines · Oleg Nesterov
And if it always takes ->pi_lock we do not need the new
PF_or something else,exit_task_work()can settask->task_works=NO_MOREunder ->pi_lock (task_work_run()can checkPF_EXITING), andtask_work_add()ensures thattask_works!=NO_MORE.What do you think?
It is not clear to me if you agree or not. So I am simply sending the patches I have.
Feel free to ignore or re-do.
Seriously, why should we add 2 pointers into task_struct? Sure, this is minor, but still... But perhaps task_work.c should not play tricks with the circular list, task_work_run() can reverse the list as you initially suggested.
Also, I am not sure about "define rcu_head callback_head", this series doesn't do this. But again, up to you.
Oleg.
PATCH 1/4 task_work: use the single-linked list to shrink sizeof(task_work)
Change struct task_work to have a single forward pointer. The pending task_work's create the circular list, and task->last_twork points to the last element. This way we can access head/tail in O(1).
--- include/linux/sched.h | 3 +- include/linux/task_work.h | 5 +-- include/linux/tracehook.h | 4 +- kernel/fork.c | 2 +- kernel/task_work.c | 62 +++++++++++++++++++++++++------------------- 5 files changed, 42 insertions(+), 34 deletions(-)
include/linux/sched.h +2 -1
@@ -101,6 +101,7 @@ struct bio_list;struct fs_struct ;struct perf_event_context ;struct blk_plug ; +struct task_work ;/* * List of flags we want to share for kernel threads,
@@ -1405,7 +1406,7 @@ struct task_struct {int (*notifier)(void *priv);void *notifier_data;sigset_t *notifier_mask; -struct hlist_head task_works; +struct task_work *last_twork;struct audit_context *audit_context;#ifdef CONFIG_AUDITSYSCALL
include/linux/task_work.h +2 -3
@@ -1,14 +1,13 @@#ifndef _LINUX_TASK_WORK_H#define _LINUX_TASK_WORK_H -#include <linux/list.h> #include <linux/sched.h> struct task_work ;typedef void (*task_work_func_t )(struct task_work *);struct task_work { -struct hlist_node hlist; +struct task_work *next;task_work_func_t func;void *data; };
@@ -26,7 +25,7 @@ void task_work_run(void);static inline void exit_task_work (struct task_struct *task) { -if (unlikely (!hlist_empty (&task->task_works))) +if (unlikely (task->last_twork))task_work_run (); }
include/linux/tracehook.h +2 -2
@@ -189,10 +189,10 @@ static inline void tracehook_notify_resume(struct pt_regs *regs)/* * The caller just cleared TIF_NOTIFY_RESUME. This barrier * pairs with task_work_add()->set_notify_resume() after -* hlist_add_head(task->task_works); +* addition to task->last_twork list. */ smp_mb__after_clear_bit (); -if (unlikely (!hlist_empty (¤t->task_works))) +if (unlikely (current->last_twork))task_work_run (); }
kernel/fork.c +1 -1
@@ -1415,7 +1415,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,*/ p->group_leader = p;INIT_LIST_HEAD (&p->thread_group); -INIT_HLIST_HEAD (&p->task_works); + p->last_twork =NULL ;/* Now that the task is set up, run cgroup callbacks if * necessary. We need to run them before the task is visible
kernel/task_work.c +35 -27
98 lines not on this page · raw message
Signature · 1 line
1.5.5.1
PATCH 2/4 task_work: don't rely on PF_EXITING
In reply to Oleg Nesterovtask_work_add() checks PF_EXITING to ensure it can't insert the new twork after exit_task_work(). This means that the exiting task can not use it after exit_signals().
Change task_work_add() and task_work_run() to use the fake TWORK_EXITED pointer to synchronize with each other, now we can move exit_task_work() down and make task_work_add() more useful.
Unfortunately, this means that the exiting task needs to take pi_lock unconditionally.
--- include/linux/task_work.h | 6 ++---- kernel/task_work.c | 15 ++++++++------- 2 files changed, 10 insertions(+), 11 deletions(-)
include/linux/task_work.h +2 -4
@@ -1,9 +1,8 @@#ifndef _LINUX_TASK_WORK_H#define _LINUX_TASK_WORK_H -#include <linux/sched.h> -struct task_work ; +struct task_struct ;typedef void (*task_work_func_t )(struct task_work *); struct task_work {
@@ -25,8 +24,7 @@ void task_work_run(void);static inline void exit_task_work (struct task_struct *task) { -if (unlikely (task->last_twork)) -task_work_run (); +task_work_run (); }#endif /* _LINUX_TASK_WORK_H */
kernel/task_work.c +8 -7
@@ -6,6 +6,8 @@* task->last_twork points to the last node in the circular single-linked list. */ +#define TWORK_EXITED ((struct task_work *)1) +int task_work_add (struct task_struct *task,struct task_work *twork,bool notify) {
@@ -18,12 +20,11 @@ task_work_add(struct task_struct *task, struct task_work *twork, bool notify)return -ENOTSUPP ;#endif /* -* We must not insert the new work if the task has already passed -* exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait() -* and check PF_EXITING under pi_lock. +* We must not insert the new work if the exiting task has already +* passed task_work_run(). */ raw_spin_lock_irqsave (&task->pi_lock, flags); -if (likely (!(task->flags &PF_EXITING ))) { +if (likely (task->last_twork !=TWORK_EXITED )) { last = task->last_twork ?: twork; task->last_twork = twork; twork->next = last->next;
@@ -46,7 +47,7 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)raw_spin_lock_irqsave (&task->pi_lock, flags); last = task->last_twork; -if (last) { +if (last && last !=TWORK_EXITED ) { twork = last; do { prev = twork;
@@ -77,10 +78,10 @@ void task_work_run(void)raw_spin_lock_irq (&task->pi_lock); last = task->last_twork; - task->last_twork =NULL ; + task->last_twork = (task->flags &PF_EXITING ) ?TWORK_EXITED :NULL ;raw_spin_unlock_irq (&task->pi_lock); -if (unlikely (!last)) +if (!last)return ; next = last->next;
Signature · 1 line
1.5.5.1
PATCH 3/4 task_work: deal with task_work callbacks adding more work
In reply to Oleg NesterovShamelessly stolen Al Viro's patch and the changelog, just I didn't dare to add the proper From tag.
It doesn't matter on normal return to userland path (we'll recheck the NOTIFY_RESUME flag anyway), but in case of exit_task_work() we'll need that as soon as we get callbacks capable of triggering more task_work_add().
--- kernel/task_work.c | 33 +++++++++++++++++++++------------ 1 files changed, 21 insertions(+), 12 deletions(-)
kernel/task_work.c +21 -12
@@ -76,18 +76,27 @@ void task_work_run(void)struct task_struct *task = current;struct task_work *last, *next, *twork; -raw_spin_lock_irq (&task->pi_lock); - last = task->last_twork; - task->last_twork = (task->flags &PF_EXITING ) ?TWORK_EXITED :NULL ; -raw_spin_unlock_irq (&task->pi_lock); +for (;;) { +/* +* twork->func() can do task_work_add(), do not +* set TWORK_EXITED until the list becomes empty. +*/ +raw_spin_lock_irq (&task->pi_lock); + last = task->last_twork; +if (last) + task->last_twork =NULL ; +else if (task->flags &PF_EXITING ) + task->last_twork =TWORK_EXITED ; +raw_spin_unlock_irq (&task->pi_lock); -if (!last) -return ; +if (!last) +return ; - next = last->next; -do { - twork = next; - next = twork->next; - twork->func (twork); - }while (twork != last); + next = last->next; +do { + twork = next; + next = twork->next; + twork->func (twork); + }while (twork != last); + } }
Signature · 1 line
1.5.5.1
PATCH 4/4 task_work: kill task_work->data
In reply to Oleg NesterovKill task_work->data, this makes sizeof(task_work) == sizeof(rcu_head).
Turn cred->rcu into rcu_head/task_work union for keyctl_session_to_parent().
--- include/linux/cred.h | 6 +++++- include/linux/task_work.h | 4 +--- kernel/irq/manage.c | 2 +- security/keys/keyctl.c | 31 +++++++++++-------------------- security/keys/process_keys.c | 3 +-- 5 files changed, 19 insertions(+), 27 deletions(-)
include/linux/cred.h +5 -1
@@ -18,6 +18,7 @@#include <linux/selinux.h> #include <linux/atomic.h> #include <linux/uidgid.h> +#include <linux/task_work.h> struct user_struct ;struct cred ;
@@ -149,7 +150,10 @@ struct cred {struct user_struct *user;/* real user ID subscription */ struct user_namespace *user_ns;/* user_ns the caps and keyrings are relative to. */ struct group_info *group_info;/* supplementary groups for euid/fsgid */ -struct rcu_head rcu;/* RCU deletion hook */ +union { +struct rcu_head rcu;/* RCU deletion hook */ +struct task_work twork;/* keyctl_session_to_parent() */ + }; };extern void __put_cred (struct cred *);
include/linux/task_work.h +1 -3
@@ -8,14 +8,12 @@ typedef void (*task_work_func_t)(struct task_work *);struct task_work {struct task_work *next;task_work_func_t func; -void *data; };static inline void -init_task_work (struct task_work *twork,task_work_func_t func,void *data) +init_task_work (struct task_work *twork,task_work_func_t func) { twork->func = func; - twork->data = data; }int task_work_add (struct task_struct *task,struct task_work *twork,bool );
kernel/irq/manage.c +1 -1
@@ -830,7 +830,7 @@ static int irq_thread(void *data)sched_setscheduler (current,SCHED_FIFO , ¶m); -init_task_work (&on_exit_work, irq_thread_dtor,NULL ); +init_task_work (&on_exit_work, irq_thread_dtor);task_work_add (current, &on_exit_work, false);while (!irq_wait_for_interrupt (action)) {
security/keys/keyctl.c +11 -20
@@ -1456,8 +1456,8 @@ long keyctl_session_to_parent(void) {struct task_struct *me, *parent;const struct cred *mycred, *pcred; -struct task_work *newwork, *oldwork;key_ref_t keyring_r; +struct task_work *oldwork;struct cred *cred;int ret;
@@ -1465,20 +1465,16 @@ long keyctl_session_to_parent(void)if (IS_ERR (keyring_r))return PTR_ERR (keyring_r); - ret = -ENOMEM ; - newwork =kmalloc (sizeof (struct task_work ),GFP_KERNEL ); -if (!newwork) - goto error_keyring; -/* our parent is going to need a new cred struct, a new tgcred struct * and new security data, so we allocate them here to prevent ENOMEM in * our parent */ + ret = -ENOMEM ; cred =cred_alloc_blank ();if (!cred) - goto error_newwork; + goto error_keyring; cred->tgcred->session_keyring =key_ref_to_ptr (keyring_r); -init_task_work (newwork, key_change_session_keyring, cred); +init_task_work (&cred->twork, key_change_session_keyring); me = current;rcu_read_lock ();
@@ -1527,24 +1523,19 @@ long keyctl_session_to_parent(void)/* the replacement session keyring is applied just prior to userspace * restarting */ - ret =task_work_add (parent, newwork, true); + ret =task_work_add (parent, &cred->twork, true);if (!ret) - newwork =NULL ; + cred =NULL ; unlock:write_unlock_irq (&tasklist_lock);rcu_read_unlock (); -if (oldwork) { -put_cred (oldwork->data); -kfree (oldwork); - } -if (newwork) { -put_cred (newwork->data); -kfree (newwork); - } + +if (oldwork) + put_cred(container_of(oldwork, struct cred, twork)); +if (cred) +put_cred (cred);return ret; -error_newwork: -kfree (newwork); error_keyring:key_ref_put (keyring_r);return ret;
security/keys/process_keys.c +1 -2
@@ -837,9 +837,8 @@ error:void key_change_session_keyring (struct task_work *twork) {const struct cred *old =current_cred (); -struct cred *new = twork->data; + struct cred *new = container_of(twork, struct cred, twork); -kfree (twork);if (unlikely (current->flags &PF_EXITING )) {put_cred (new);return ;
Signature · 1 line
1.5.5.1
Re: [PATCH 4/4] task_work: kill task_work->data
Quoted 2 lines · Oleg Nesterov
Turn
cred->rcuinto rcu_head/task_work union forkeyctl_session_to_parent().
I tried to make the minimal patch, but perhaps with this change we can also simplify the error handling in keyctl_session_to_parent() a little bit.
We can do cred_alloc_blank() before lookup_user_key() and remove error_keyring/key_ref_put.
IOW, on top of this patch:
security/keys/keyctl.c +8 -10
@@ -1461,17 +1461,18 @@ long keyctl_session_to_parent(void)struct cred *cred;int ret; - keyring_r =lookup_user_key (KEY_SPEC_SESSION_KEYRING ,0 ,KEY_LINK ); -if (IS_ERR (keyring_r)) -return PTR_ERR (keyring_r); -/* our parent is going to need a new cred struct, a new tgcred struct * and new security data, so we allocate them here to prevent ENOMEM in * our parent */ - ret = -ENOMEM ; cred =cred_alloc_blank ();if (!cred) - goto error_keyring; +return -ENOMEM ; + + keyring_r =lookup_user_key (KEY_SPEC_SESSION_KEYRING ,0 ,KEY_LINK ); +if (IS_ERR (keyring_r)) { + ret =PTR_ERR (keyring_r); + goto free_cred; + } cred->tgcred->session_keyring =key_ref_to_ptr (keyring_r);init_task_work (&cred->twork, key_change_session_keyring);
@@ -1532,13 +1533,10 @@ unlock:if (oldwork) put_cred(container_of(oldwork, struct cred, twork)); +free_cred:if (cred)put_cred (cred);return ret; - -error_keyring: -key_ref_put (keyring_r); -return ret; }/*
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
In reply to Oleg NesterovQuoted 14 lines · Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
And if it always takes ->pi_lock we do not need the new
PF_or something else,exit_task_work()can settask->task_works=NO_MOREunder ->pi_lock (task_work_run()can checkPF_EXITING), andtask_work_add()ensures thattask_works!=NO_MORE.What do you think?
It is not clear to me if you agree or not. So I am simply sending the patches I have.
Feel free to ignore or re-do.
Quoted 7 lines
Seriously, why should we add 2 pointers into
task_struct? Sure, this is minor, but still... But perhapstask_work.cshould not play tricks with the circular list,task_work_run()can reverse the list as you initially suggested.Also, I am not sure about "define
rcu_headcallback_head", this series doesn't do this. But again, up to you.
Umm... FWIW, my variant circa yesterday is in vfs.git#untested; it seems to survive on uml/amd64 at least. I'll look through your patches and see what can be nicked. The list removal logics in mine looks really ugly ;-/
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 2 lines · Al Viro
The list removal logics in mine looks really ugly ;-/
Basically the code is almost the same as mine.
But task_work_add() can be simplified a bit, no need to check last != NULL twice.
last = task->task_works ?: twork; task->last_twork = twork; twork->next = last->next; /* XXX */ last->next = twork;
If ->task_works == NULL, then XXX is meaningless but safe.
Oleg.
Re: [PATCH 0/4] Was: deferring __fput()
Forgot to mention...
And I still think that task_work_add() should not succeed unconditionally, it synchronize with exit_task_work(). Otherwise keyctl_session_to_parent() is broken.
Quoted 18 lines · Oleg Nesterov
Quoted 2 lines · Al Viro
The list removal logics in mine looks really ugly ;-/
Basically the code is almost the same as mine.
But
task_work_add()can be simplified a bit, no need to check last != NULL twice.last = task->task_works ?: twork; task->last_twork = twork; twork->next = last->next; /* XXX */ last->next = twork;If ->task_works == NULL, then XXX is meaningless but safe.
Oleg.
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 5 lines · Oleg Nesterov
Forgot to mention...
And I still think that
task_work_add()should not succeed unconditionally, it synchronize withexit_task_work(). Otherwisekeyctl_session_to_parent()is broken.
Hmm... Look: if nothing else, we have
/* the parent mustn't be init and mustn't be a kernel thread */
if (parent->pid <= 1 || !parent->mm)
goto unlock;in the caller. OTOH, on the exit side we have exit_mm() done first. And that will have ->mm set to NULL. So we are closing a very narrow race to start with. So why not do the following and be done with that?
security/keys/keyctl.c +2 -0
@@ -1486,6 +1486,7 @@ long keyctl_session_to_parent(void) oldwork =NULL ; parent = me->real_parent; +task_lock (parent);/* the parent mustn't be init and mustn't be a kernel thread */ if (parent->pid <=1 || !parent->mm) goto unlock;
@@ -1529,6 +1530,7 @@ long keyctl_session_to_parent(void)if (!ret) newwork =NULL ; unlock: +task_unlock (parent);write_unlock_irq (&tasklist_lock);rcu_read_unlock ();if (oldwork)
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 8 lines · Al Viro
Quoted 5 lines · Oleg Nesterov
Forgot to mention...
And I still think that
task_work_add()should not succeed unconditionally, it synchronize withexit_task_work(). Otherwisekeyctl_session_to_parent()is broken.
Quoted 7 lines
Hmm... Look: if nothing else, we have
/* the parent mustn't be init and mustn't be a kernel thread */ if (parent->pid <= 1 || !parent->mm) goto unlock;in the caller. OTOH, on the exit side we have
exit_mm()done first. And that will have ->mm set to NULL. So we are closing a very narrow race to start with. So why not do the following and be done with that?
Of course we can fix keyctl_session_to_parent(). But why? I mean, why do you dislike the idea to put this synchronization into add/run ?
IMO, this makes task_work much less useful/convenient. Every caller has to fight with this race somehow. And the lock it can take depends on the context, say, you can't use task_lock() in irq.
IOW, what is wrong with
[PATCH 2/4] task_work: don't rely on PF_EXITING http://marc.info/?l=linux-kernel&m=134082265321691
and
[PATCH 3/4] task_work: deal with task_work callbacks adding more work http://marc.info/?t=134082275400004
?
perhaps you do not like the fact that the exiting task takes pi_lock unconditionally?
and in fact I think that probably it makes sense to change fput,
- task_work_add(current, ...);
+ BUG_ON(task_work_add(current, ...) != 0);
Oleg.
Re: [PATCH 0/4] Was: deferring __fput()
In reply to Al ViroQuoted 20 lines · Al Viro
Quoted 19 lines · Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
And if it always takes ->pi_lock we do not need the new
PF_or something else,exit_task_work()can settask->task_works=NO_MOREunder ->pi_lock (task_work_run()can checkPF_EXITING), andtask_work_add()ensures thattask_works!=NO_MORE.What do you think?
It is not clear to me if you agree or not. So I am simply sending the patches I have.
Feel free to ignore or re-do.
Seriously, why should we add 2 pointers into
task_struct? Sure, this is minor, but still... But perhapstask_work.cshould not play tricks with the circular list,task_work_run()can reverse the list as you initially suggested.
Quoted 6 lines
Quoted 2 lines
Also, I am not sure about "define
rcu_headcallback_head", this series doesn't do this. But again, up to you.Umm... FWIW, my variant circa yesterday is in vfs.git#untested; it seems to survive on uml/amd64 at least. I'll look through your patches and see what can be nicked. The list removal logics in mine looks really ugly ;-/
Still failing to boot. Fails to boot starting with commit "b24dfa6 switch fput to task_work_add".
Mimi
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 24 lines · Mimi Zohar
Quoted 23 lines · Al Viro
Quoted 21 lines · Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
And if it always takes ->pi_lock we do not need the new
PF_or something else,exit_task_work()can settask->task_works=NO_MOREunder ->pi_lock (task_work_run()can checkPF_EXITING), andtask_work_add()ensures thattask_works!=NO_MORE.What do you think?
It is not clear to me if you agree or not. So I am simply sending the patches I have.
Feel free to ignore or re-do.
Seriously, why should we add 2 pointers into
task_struct? Sure, this is minor, but still... But perhapstask_work.cshould not play tricks with the circular list,task_work_run()can reverse the list as you initially suggested.Also, I am not sure about "define
rcu_headcallback_head", this series doesn't do this. But again, up to you.
Quoted 6 lines
Quoted 3 lines
Umm... FWIW, my variant circa yesterday is in vfs.git#untested; it seems to survive on uml/amd64 at least. I'll look through your patches and see what can be nicked. The list removal logics in mine looks really ugly ;-/
Still failing to boot. Fails to boot starting with commit "b24dfa6 switch fput to
task_work_add".
Details, please... .config, root fs type, etc. Failed execve() of init is, unfortunately, not too informative...
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 29 lines · Al Viro
Quoted 28 lines · Mimi Zohar
Quoted 26 lines · Al Viro
Quoted 14 lines · Oleg Nesterov
Quoted 7 lines · Oleg Nesterov
And if it always takes ->pi_lock we do not need the new
PF_or something else,exit_task_work()can settask->task_works=NO_MOREunder ->pi_lock (task_work_run()can checkPF_EXITING), andtask_work_add()ensures thattask_works!=NO_MORE.What do you think?
It is not clear to me if you agree or not. So I am simply sending the patches I have.
Feel free to ignore or re-do.
Quoted 7 lines
Seriously, why should we add 2 pointers into
task_struct? Sure, this is minor, but still... But perhapstask_work.cshould not play tricks with the circular list,task_work_run()can reverse the list as you initially suggested.Also, I am not sure about "define
rcu_headcallback_head", this series doesn't do this. But again, up to you.Umm... FWIW, my variant circa yesterday is in vfs.git#untested; it seems to survive on uml/amd64 at least. I'll look through your patches and see what can be nicked. The list removal logics in mine looks really ugly ;-/
Quoted 5 lines
Quoted 2 lines
Still failing to boot. Fails to boot starting with commit "b24dfa6 switch fput to
task_work_add".Details, please... .config, root fs type, etc. Failed
execve()of init is, unfortunately, not too informative...
In addition to failing with my tailored .config (eg. IMA, TPM builtin), it fails with config-3.4.2-1.fc16.x86_64, with new default options, running on a Lenovo W520.
/dev/sda7 on / type ext4 (rw,relatime,seclabel,i_version,data=ordered)
Mimi
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 5 lines · Mimi Zohar
In addition to failing with my tailored .config (eg. IMA, TPM builtin), it fails with config-3.4.2-1.fc16.x86_64, with new default options, running on a Lenovo W520.
/dev/sda7on / type ext4 (rw,relatime,seclabel,i_version,data=ordered)
Lovely... Smells like something making dracut unhappy. Does it say anything interesting when booting with rdinitdebug in command line? What happens with rdshell in there?
I don't have easily accessible Fedora boxen at the moment and it does look like something fishy going on while initramfs stuff runs...
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 6 lines · Al Viro
Quoted 4 lines · Mimi Zohar
In addition to failing with my tailored .config (eg. IMA, TPM builtin), it fails with config-3.4.2-1.fc16.x86_64, with new default options, running on a Lenovo W520.
Quoted 8 lines
Quoted 1 line
/dev/sda7on / type ext4 (rw,relatime,seclabel,i_version,data=ordered)Lovely... Smells like something making dracut unhappy. Does it say anything interesting when booting with rdinitdebug in command line? What happens with rdshell in there?
I don't have easily accessible Fedora boxen at the moment and it does look like something fishy going on while initramfs stuff runs...
No difference with rdinitdebug, rdshell, but the message "ata5: SATA link down (SStatus 0 SControl 300)" prior to "Freeing unused kernel memory... ", "Failed to execute /init" and the trace back, probably helps. Don't know how I missed that.
Mimi
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 13 lines · Mimi Zohar
Quoted 12 lines · Al Viro
Quoted 5 lines · Mimi Zohar
In addition to failing with my tailored .config (eg. IMA, TPM builtin), it fails with config-3.4.2-1.fc16.x86_64, with new default options, running on a Lenovo W520.
/dev/sda7on / type ext4 (rw,relatime,seclabel,i_version,data=ordered)Lovely... Smells like something making dracut unhappy. Does it say anything interesting when booting with rdinitdebug in command line? What happens with rdshell in there?
Quoted 7 lines
Quoted 2 lines
I don't have easily accessible Fedora boxen at the moment and it does look like something fishy going on while initramfs stuff runs...
No difference with rdinitdebug, rdshell, but the message "ata5: SATA link down (SStatus 0 SControl 300)" prior to "Freeing unused kernel memory... ", "Failed to execute /init" and the trace back, probably helps. Don't know how I missed that.
Looking at /var/log/messages, seems like the ata4 and ata5 "SATA link down (SStatus 0 SControl 300)" messages are normal.
ata5: SATA link down (SStatus 0 SControl 300) Freeing unused kernel memory: 1016k freed Write protecting the kernel read-only data: 12288k Freeing unused kernel memory: 1964k freed Freeing unused kernel memory: 1468k freed Failed to execute /init
Kernel panic - not syncing. No init Found. Try passing init= option ... Pid: 1, comm: swapper/0 not tainted 3.5.0-rc1+ Call Trace: panic init_post kernel_init ?do_early_param kernel_thread_helper start_kernel
Mimi
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 9 lines · Mimi Zohar
Looking at
/var/log/messages, seems like the ata4 and ata5 "SATA link down (SStatus 0 SControl 300)" messages are normal.ata5: SATA link down (SStatus 0 SControl 300) Freeing unused kernel memory: 1016k freed Write protecting the kernel read-only data: 12288k Freeing unused kernel memory: 1964k freed Freeing unused kernel memory: 1468k freed Failed to execute /init
Quoted 9 lines
Kernel panic - not syncing. No init Found. Try passing init= option ... Pid: 1, comm: swapper/0 not tainted 3.5.0-rc1+ Call Trace: panic init_post kernel_init ?do_early_param kernel_thread_helper start_kernel
Just to make sure - you are not getting IMA violations among all that? AFAICS, the damn thing should behave no worse in that respect than your own patch a while ago, and you haven't mentioned them in this thread, but...
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 20 lines · Al Viro
Quoted 9 lines · Mimi Zohar
Looking at
/var/log/messages, seems like the ata4 and ata5 "SATA link down (SStatus 0 SControl 300)" messages are normal.ata5: SATA link down (SStatus 0 SControl 300) Freeing unused kernel memory: 1016k freed Write protecting the kernel read-only data: 12288k Freeing unused kernel memory: 1964k freed Freeing unused kernel memory: 1468k freed Failed to execute /init
Quoted 9 lines
Kernel panic - not syncing. No init Found. Try passing init= option ... Pid: 1, comm: swapper/0 not tainted 3.5.0-rc1+ Call Trace: panic init_post kernel_init ?do_early_param kernel_thread_helper start_kernel
Quoted 1 line
Just to make sure - you are not getting IMA violations among all that?
I'm not running with the IMA-appraisal patches, nor does the Fedora .config enable IMA. So I'm not getting violations.
Quoted 3 lines
AFAICS,
the damn thing should behave no worse in that respect than your own patch a while ago, and you haven't mentioned them in this thread, but...
I haven't mentioned the "ima: defer calling __fput()" patch, since I've compiled git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
#untested with a .config based on config-3.4.2-1.fc16.x86_64 and am having this problem. No need to add more confusion. The "ima: defer calling __fput()" will be dropped from the patchset, as soon as the general method works.
I've isolated the problem to the PF_KTHREAD section of fput().
void fput(struct file *file)
{
if (atomic_long_dec_and_test(&file->f_count)) {
struct task_struct *task = current;
file_sb_list_del(file);
if (unlikely(in_interrupt() || task->flags & PF_KTHREAD)) {
unsigned long flags;
spin_lock_irqsave(&delayed_fput_lock, flags);
list_add(&file->f_u.fu_list, &delayed_fput_list);
schedule_work(&delayed_fput_work);
spin_unlock_irqrestore(&delayed_fput_lock, flags);
return;
}
init_task_work(&file->f_u.fu_rcuhead, ____fput);
task_work_add(task, &file->f_u.fu_rcuhead, true);
}
}Replacing it with a call to __fput(), the system boots.
thanks,
Mimi
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 28 lines · Mimi Zohar
I haven't mentioned the "ima: defer calling
__fput()" patch, since I've compiledgit://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
#untested with a .config based on config-3.4.2-1.fc16.x86_64 and am having this problem. No need to add more confusion. The "ima: defer calling__fput()" will be dropped from the patchset, as soon as the general method works.I've isolated the problem to the
PF_KTHREADsection offput().void
fput(struct file *file){ if (atomic_long_dec_and_test(&file->f_count)) { struct task_struct *task = current; file_sb_list_del(file); if (unlikely(in_interrupt() || task->flags & PF_KTHREAD)) { unsigned long flags; spin_lock_irqsave(&delayed_fput_lock, flags); list_add(&file->f_u.fu_list, &delayed_fput_list); schedule_work(&delayed_fput_work); spin_unlock_irqrestore(&delayed_fput_lock, flags); return; } init_task_work(&file->f_u.fu_rcuhead, ____fput); task_work_add(task, &file->f_u.fu_rcuhead, true); } }
Quoted 1 line
Replacing it with a call to
__fput(), the system boots.
"it" being just the part under that if (unlikely(...)))? Very interesting... If so, we have some kernel thread ending up with delayed __fput() which somehow makes dracut (assuimg you are using fedora initramfs to go with fedora config) unhappy. With your own patch, doing async __fput() in a lot of cases when this one doesn't delay past the return to userland managing to survive the boot... I wonder which files end up triggering that fun and which kernel thread is responsible... Could you slap a printk() in there, showing file->f_dentry->d_inode->i_mode (octal) and at least file->f_dentry->d_name.name? Along with the current->comm[], all under that inner if (). And see which ones end up going that way by the time execve() of /sbin/init fails.
It would be nice to see which sys_mount() calls are made and which (if any) fail, BTW. I wonder if it even gets to mounting the right root...
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 12 lines · Al Viro
Quoted 1 line · Mimi Zohar
Replacing it with a call to
__fput(), the system boots."it" being just the part under that if (unlikely(...)))? Very interesting... If so, we have some kernel thread ending up with delayed
__fput()which somehow makes dracut (assuimg you are using fedora initramfs to go with fedora config) unhappy. With your own patch, doing async__fput()in a lot of cases when this one doesn't delay past the return to userland managing to survive the boot... I wonder which files end up triggering that fun and which kernel thread is responsible... Could you slap aprintk()in there, showingfile->f_dentry->d_inode->i_mode(octal) and at leastfile->f_dentry->d_name.name? Along with the current->comm[], all under that inner if (). And see which ones end up going that way by the timeexecve()of/sbin/initfails.
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
Quoted 2 lines
It would be nice to see which
sys_mount()calls are made and which (if any) fail, BTW. I wonder if it even gets to mounting the right root...
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Al Viro
Quoted 1 line · Mimi Zohar
Replacing it with a call to
__fput(), the system boots."it" being just the part under that if (unlikely(...)))? Very interesting... If so, we have some kernel thread ending up with delayed
__fput()which somehow makes dracut (assuimg you are using fedora initramfs to go with fedora config) unhappy. With your own patch, doing async__fput()in a lot of cases when this one doesn't delay past the return to userland managing to survive the boot... I wonder which files end up triggering that fun and which kernel thread is responsible... Could you slap aprintk()in there, showingfile->f_dentry->d_inode->i_mode(octal) and at leastfile->f_dentry->d_name.name? Along with the current->comm[], all under that inner if (). And see which ones end up going that way by the timeexecve()of/sbin/initfails.
Quoted 2 lines
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
OK... Here's what I suspect is going on:
* populating initramfs writes binaries there. We open files (for write) from
the kernel thread (there's nothing other than kernel threads at that point), write to them, then close(). Final fput() gets delayed.
* Then we proceed to execve(). Which means mapping the binary with MAP_DENYWRITE.
Which fails, since there's a struct file still opened for write on that sucker.
Your patch did not delay those fput() - they were done without ->mmap_sem held. So it survived. Booting without initramfs always survives; booting with initramfs may or may not survive, depending on the timings - if that scheduled work manages to run by the time we do those execve(), we win. Note that async_synchronize_full() done in init_post() might easily affect that, depending on config.
As a quick test, could you try slapping a delay somewhere around the beginning of init_post() and see if it rescues the system?
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 25 lines · Al Viro
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Al Viro
Quoted 1 line · Mimi Zohar
Replacing it with a call to
__fput(), the system boots."it" being just the part under that if (unlikely(...)))? Very interesting... If so, we have some kernel thread ending up with delayed
__fput()which somehow makes dracut (assuimg you are using fedora initramfs to go with fedora config) unhappy. With your own patch, doing async__fput()in a lot of cases when this one doesn't delay past the return to userland managing to survive the boot... I wonder which files end up triggering that fun and which kernel thread is responsible... Could you slap aprintk()in there, showingfile->f_dentry->d_inode->i_mode(octal) and at leastfile->f_dentry->d_name.name? Along with the current->comm[], all under that inner if (). And see which ones end up going that way by the timeexecve()of/sbin/initfails.Quoted 2 lines
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
OK... Here's what I suspect is going on:
* populating initramfs writes binaries there. We open files (for write) from
the kernel thread (there's nothing other than kernel threads at that point), write to them, thenclose(). Finalfput()gets delayed.
* Then we proceed toexecve(). Which means mapping the binary withMAP_DENYWRITE.
Which fails, since there's a struct file still opened for write on that sucker.
Quoted 8 lines
Your patch did not delay those
fput()- they were done without ->mmap_sem held. So it survived. Booting without initramfs always survives; booting with initramfs may or may not survive, depending on the timings - if that scheduled work manages to run by the time we do thoseexecve(), we win. Note thatasync_synchronize_full()done ininit_post()might easily affect that, depending on config.As a quick test, could you try slapping a delay somewhere around the beginning of
init_post()and see if it rescues the system?
Ho-hum... How about this (modulo missing documentation of the whole sad mess):
fs/file_table.c +5 -0
@@ -284,6 +284,11 @@ static void ____fput(struct callback_head *work) __fput(container_of(work, struct file, f_u.fu_rcuhead)); } +void flush_delayed_fput (void ) +{ +delayed_fput (NULL ); +} + static DECLARE_WORK(delayed_fput_work, delayed_fput);void fput (struct file *file)
include/linux/file.h +2 -0
@@ -39,4 +39,6 @@ extern void put_unused_fd(unsigned int fd);extern void fd_install (unsigned int fd,struct file *file); +extern void flush_delayed_fput (void ); +#endif /* __LINUX_FILE_H */
init/main.c +2 -1
@@ -68,6 +68,7 @@#include <linux/shmem_fs.h> #include <linux/slab.h> #include <linux/perf_event.h> +#include <linux/file.h> #include <asm/io.h> #include <asm/bugs.h>
@@ -804,8 +805,8 @@ static noinline int init_post(void) system_state =SYSTEM_RUNNING ;numa_default_policy (); - current->signal->flags |=SIGNAL_UNKILLABLE ; +flush_delayed_fput ();if (ramdisk_execute_command) {run_init_process (ramdisk_execute_command);
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 32 lines · Al Viro
Quoted 31 lines · Al Viro
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Al Viro
Quoted 1 line · Mimi Zohar
Replacing it with a call to
__fput(), the system boots."it" being just the part under that if (unlikely(...)))? Very interesting... If so, we have some kernel thread ending up with delayed
__fput()which somehow makes dracut (assuimg you are using fedora initramfs to go with fedora config) unhappy. With your own patch, doing async__fput()in a lot of cases when this one doesn't delay past the return to userland managing to survive the boot... I wonder which files end up triggering that fun and which kernel thread is responsible... Could you slap aprintk()in there, showingfile->f_dentry->d_inode->i_mode(octal) and at leastfile->f_dentry->d_name.name? Along with the current->comm[], all under that inner if (). And see which ones end up going that way by the timeexecve()of/sbin/initfails.Quoted 2 lines
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
OK... Here's what I suspect is going on:
* populating initramfs writes binaries there. We open files (for write) from
the kernel thread (there's nothing other than kernel threads at that point), write to them, thenclose(). Finalfput()gets delayed.
* Then we proceed toexecve(). Which means mapping the binary withMAP_DENYWRITE.
Which fails, since there's a struct file still opened for write on that sucker.Your patch did not delay those
fput()- they were done without ->mmap_sem held. So it survived. Booting without initramfs always survives; booting with initramfs may or may not survive, depending on the timings - if that scheduled work manages to run by the time we do thoseexecve(), we win. Note thatasync_synchronize_full()done ininit_post()might easily affect that, depending on config.
Quoted 4 lines
Quoted 2 lines
As a quick test, could you try slapping a delay somewhere around the beginning of
init_post()and see if it rescues the system?Ho-hum... How about this (modulo missing documentation of the whole sad mess):
Sorry, neither adding the delay or this patch helped.
Quoted 49 lines
fs/file_table.c +5
-0@@ -284,6 +284,11 @@ static void ____fput(struct callback_head *work) __fput(container_of(work, struct file, f_u.fu_rcuhead)); } +void flush_delayed_fput (void ) +{ +delayed_fput (NULL ); +} + static DECLARE_WORK(delayed_fput_work, delayed_fput);void fput (struct file *file)include/linux/file.h +2
-0@@ -39,4 +39,6 @@ extern void put_unused_fd(unsigned int fd);extern void fd_install (unsigned int fd,struct file *file); +extern void flush_delayed_fput (void ); +#endif /* __LINUX_FILE_H */ init/main.c +2
-1@@ -68,6 +68,7 @@#include <linux/shmem_fs.h> #include <linux/slab.h> #include <linux/perf_event.h> +#include <linux/file.h> #include <asm/io.h> #include <asm/bugs.h> @@ -804,8 +805,8 @@ static noinline int init_post(void) system_state =SYSTEM_RUNNING ;numa_default_policy (); - current->signal->flags |=SIGNAL_UNKILLABLE ; +flush_delayed_fput ();if (ramdisk_execute_command) {run_init_process (ramdisk_execute_command);
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 16 lines · Mimi Zohar
Quoted 16 lines
Quoted 16 lines
Quoted 2 lines
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
OK... Here's what I suspect is going on:
* populating initramfs writes binaries there. We open files (for write) from
the kernel thread (there's nothing other than kernel threads at that point), write to them, thenclose(). Finalfput()gets delayed.
* Then we proceed toexecve(). Which means mapping the binary withMAP_DENYWRITE.
Which fails, since there's a struct file still opened for write on that sucker.Your patch did not delay those
fput()- they were done without ->mmap_sem held. So it survived. Booting without initramfs always survives; booting with initramfs may or may not survive, depending on the timings - if that scheduled work manages to run by the time we do thoseexecve(), we win. Note thatasync_synchronize_full()done ininit_post()might easily affect that, depending on config.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
As a quick test, could you try slapping a delay somewhere around the beginning of
init_post()and see if it rescues the system?Ho-hum... How about this (modulo missing documentation of the whole sad mess):
Sorry, neither adding the delay or this patch helped.
Really odd. Could you print the error returned by kernel_execve() in run_init_process()? At least that way we'll get some indication of what's going on there. Another thing:
could you slap matching printks into the nested if() in fput() and the loop in delayed_fput(), just to see if we do get __fput() done on all the right struct file? Just "fput: %p", file and "delayed_fput: %p", file would probably be enough.
I'm assuming that I hadn't misparsed what you wrote and that __fput() in nested if() in fput() was enough to get the thing working. Could you confirm that?
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 23 lines · Al Viro
Quoted 21 lines · Mimi Zohar
Quoted 20 lines
Quoted 10 lines
Quoted 2 lines
pid=1 uid=0 d_name=init comm=swapper/0 dev="rootfs" mode=100775 pid=1 uid=0 d_name=bash comm=swapper/0 dev="rootfs" mode=100755
OK... Here's what I suspect is going on:
* populating initramfs writes binaries there. We open files (for write) from
the kernel thread (there's nothing other than kernel threads at that point), write to them, thenclose(). Finalfput()gets delayed.
* Then we proceed toexecve(). Which means mapping the binary withMAP_DENYWRITE.
Which fails, since there's a struct file still opened for write on that sucker.Quoted 8 lines
Your patch did not delay those
fput()- they were done without ->mmap_sem held. So it survived. Booting without initramfs always survives; booting with initramfs may or may not survive, depending on the timings - if that scheduled work manages to run by the time we do thoseexecve(), we win. Note thatasync_synchronize_full()done ininit_post()might easily affect that, depending on config.As a quick test, could you try slapping a delay somewhere around the beginning of
init_post()and see if it rescues the system?Ho-hum... How about this (modulo missing documentation of the whole sad mess):
Quoted 7 lines
Quoted 1 line
Sorry, neither adding the delay or this patch helped.
Really odd. Could you print the error returned by
kernel_execve()inrun_init_process()? At least that way we'll get some indication of what's going on there. Another thing:
could you slap matching printks into the nestedif()infput()and the loop indelayed_fput(), just to see if we do get__fput()done on all the right struct file? Just "fput: %p", file and "delayed_fput: %p", file would probably be enough.
ok, it calls delayed_fput(), but never makes it into the delayed_fput() while statement. I added an additional printk to make sure delayed_fput() was being called.
delayed_fput:
fput: xxxxrun_init_process: -26
failed to execute /init
run_init_process: -2run_init_process: -2run_init_process: -2
delayed_fput:
fput: xxxxrun_init_process: -26
Quoted 2 lines
I'm assuming that I hadn't misparsed what you wrote and that
__fput()in nestedif()infput()was enough to get the thing working. Could you confirm that?
Yes, everything is exactly as you described.
Mimi
Al Viro
maintainerRe: [PATCH 0/4] Was: deferring __fput()
Quoted 2 lines · Mimi Zohar
ok, it calls
delayed_fput(), but never makes it into thedelayed_fput()while statement.
That's because I'm a blind idiot and managed to miss the idiocy in delayed_fput() - that
list_splice(&head, &delayed_fput_list); should've been list_splice_init(&delayed_fput_list, &head);
Even more embarrassingly, I've actually screwed up the build scripts and hadn't discovered until now that I'd been testing without initramfs; as soon as I'd found that, the bug had promptly reproduced itself.
Anyway, I've pushed the fix into vfs.git#master. Should propagate to git.kernel.org in a few (commit 2ae9632 is the branch head)
Re: [PATCH 0/4] Was: deferring __fput()
Quoted 2 lines · Al Viro
Anyway, I've pushed the fix into vfs.git#master. Should propagate to git.kernel.org in a few (commit 2ae9632 is the branch head)
Thanks! I'm now able to boot with the delayed __fput().
Mimi
PATCH 1/1 task_work: add a scheduling point in task_work_run()
From: Eric Dumazet <[email protected]>
It seems commit 4a9d4b02 (switch fput to task_work_add) reintroduced the problem addressed in commit 944be0b2 (close_files(): add scheduling point)
If a server process with a lot of files (say 2 million tcp sockets) is killed, we can spend a lot of time in task_work_run() and trigger a soft lockup.
--- kernel/task_work.c | 1 + 1 file changed, 1 insertion(+)
kernel/task_work.c +1 -0
@@ -75,6 +75,7 @@ void task_work_run(void) p = q->next; q->func (q); q = p; +cond_resched (); } } }
Re: [PATCH] task_work: add a scheduling point in task_work_run()
Quoted 20 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
It seems commit
4a9d4b02(switch fput totask_work_add) reintroduced the problem addressed in commit944be0b2(close_files(): add scheduling point)If a server process with a lot of files (say 2 million tcp sockets) is killed, we can spend a lot of time in
task_work_run()and trigger a soft lockup.Signed-off-by Eric Dumazet <[email protected]>--- kernel/task_work.c | 1 + 1 file changed, 1 insertion(+)kernel/task_work.c +0
-0
kernel/task_work.c void task_work_run(void) · line 81
Quoted 7 lines
kernel/task_work.c +1
-0@@ -75,6 +75,7 @@ void task_work_run(void) p = q->next; q->func (q); q = p; +cond_resched (); } } }
We're here, because fput() called schedule_work() to delay the last fput(). The execution needs to take place before the syscall returns to userspace. Need to read __schedule()... Do you know if cond_resched() can guarantee that it will be executed before the return to userspace?
thanks,
Mimi
Re: [PATCH] task_work: add a scheduling point in task_work_run()
Quoted 4 lines · Mimi Zohar
We're here, because
fput()calledschedule_work()to delay the lastfput(). The execution needs to take place before the syscall returns to userspace. Need to read__schedule()... Do you know ifcond_resched()can guarantee that it will be executed before the return to userspace?
Some clarifications :
- fput() does not call schedule_work() in this case but task_work_add()
- cond_resched() wont return to userspace.
Re: [PATCH] task_work: add a scheduling point in task_work_run()
Quoted 7 lines · Eric Dumazet
Quoted 4 lines · Mimi Zohar
We're here, because
fput()calledschedule_work()to delay the lastfput(). The execution needs to take place before the syscall returns to userspace. Need to read__schedule()... Do you know ifcond_resched()can guarantee that it will be executed before the return to userspace?
Quoted 5 lines
Some clarifications :
-
fput()does not callschedule_work()in this case buttask_work_add()-
cond_resched()wont return to userspace.
Thanks for the clarification.
Mimi
Re: [PATCH] task_work: add a scheduling point in task_work_run()
In reply to Eric DumazetHi, Eric
Quoted 9 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
It seems commit
4a9d4b02(switch fput totask_work_add) reintroduced the problem addressed in commit944be0b2(close_files(): add scheduling point)If a server process with a lot of files (say 2 million tcp sockets) is killed, we can spend a lot of time in
task_work_run()and trigger a soft lockup.
The thread will be rescheduled if we support kernel preempt, so this change may only help the case we haven't enabled CONFIG_PREEMPT, isn't it? What about using ifndef?
And can we make sure that it is safe to sleep(schedule) at this point? It may need some totally testing to cover all the situation...
Regards,
Michael Wang
Quoted 26 lines
Signed-off-by Eric Dumazet <[email protected]>--- kernel/task_work.c | 1 + 1 file changed, 1 insertion(+)kernel/task_work.c +1
-1@@ -75,6 +75,7 @@ void task_work_run(void) p = q->next; q->func (q); q = p; +cond_resched (); } } } --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Al Viro
maintainerRe: [PATCH] task_work: add a scheduling point in task_work_run()
Quoted 2 lines · Michael Wang
And can we make sure that it is safe to sleep(schedule) at this point? It may need some totally testing to cover all the situation...
task_work callback can bloody well block, so yes, it is safe. Hell, we are doing final close from that; that can lead to any amount of IO, up to and including on-disk file freeing and, in case of vfsmount kept alive by an opened file after we'd done umount -l, actual final unmount of a filesystem. That can more than just block, that can block for a long time if that's a network filesystem...
Al Viro
maintainerQuoted 10 lines · Al Viro
BTW, I really wonder why do we need to have that void *data in
task_work; we can always embed the sucker into a bigger struct (if nothing else,task_work+ void *data) and get to it viacontainer_of(). And in quite a few cases we don't want that data thing at all. Moreover, the reasons to usehlist_headinstead of a single forward pointer are very thin on the ground:
*task_work_add()adds to the beginning of the list
*task_work_cancel()walks the list to find the entry to remove
*trask_work_run()takes the list out, walks it to the end,
then walks it backwards via pprev. Could as well reverse the pointers while walking forward...
You know what... Let's dump that "reverse the list" thing completely. What we ought to do instead of that is honestly keeping both the head of the (single-linked) list and pointer to pointer to its last element. Sure, that'll eat one more word in task_struct. And it doesn't matter, since we'll be able to kill something else in there - namely, ->scm_work_list. The whole reason we have it is that we want to prevent recursion in __scm_destroy(); with __fput() getting done via task_work_add() this recursion will be gone automatically.
So here's what I want to do:
1) kill task_work->data; the only user that cares will allocate task_work + struct cred * and use container_of() to get to it.
2) replace current->task_works with struct task_work *task_works, **last_work and replace hlist_node in task_work with struct task_work *next; task_work_add() would add to the tail, task_work_cancel() would do the obvious "scan the single-linked list, remove the element found" wihtout bothering with hlist and task_work_run() would just walk the list in direct order.
3) at that point task_work is equal in size (and layout, BTW) to rcu_head. So we can add it into the same union in struct file where we already have list_head and rcu_head. No space eaten up. fput() would, once the counter reaches 0, remove the file from list (the only place walking that list skips the ones with zero refcount anyway) and, if we are in a normal process, use task_work_add() to have __fput() done to it. If we are in kernel thread or atomic context, just move the sucker to global list and use schedule_work() to have said list emptied and everything in it fed to __fput().
4) kill ->scm_work_list, along with all contortions in __scm_destroy(); just have it do all the fput() (if any) it wants to do for this cookie.
5) in fs/aio.c, replace
if (unlikely(!fput_atomic(req->ki_filp))) {
spin_lock(&fput_lock);
list_add(&req->ki_list, &fput_head);
spin_unlock(&fput_lock);
schedule_work(&fput_work);
} else {
req->ki_filp = NULL;
really_put_req(ctx, req);
}
with
fput(req->ki_filp);
req->ki_filp = NULL;
really_put_req(ctx, req);Sure, we are not allowed to block there. And we won't. fput_head/fput_work/aio_fput_routine() goes to hell, and so does the whole "called_fput" mess in kiocb_batch_refill() in there - retry loop and all.
Objections, anyone? Linus, Oleg, Dave?
Al Viro
maintainerQuoted 6 lines · Al Viro
3) at that point
task_workis equal in size (and layout, BTW) torcu_head. So we can add it into the same union in struct file where we already havelist_headandrcu_head. No space eaten up.fput()would, once the counter reaches 0, remove the file from list (the only place walking that list skips the ones with zero refcount anyway) and, if we are in a normal process, usetask_work_add()to have__fput()done to it. If we are in kernel thread or atomic context, just move the sucker to global list and useschedule_work()to have said
Only that's probably s/atomic/interrupt/
Quoted 1 line
list emptied and everything in it fed to
__fput().
Quoted 2 lines · Al Viro
You know what... Let's dump that "reverse the list" thing completely.
Yes, it was never really needed for fifo.
Quoted 5 lines
What we ought to
do instead of that is honestly keeping both the head of the (single-linked) list and pointer to pointer to its last element. Sure, that'll eat one more word intask_struct. And it doesn't matter, since we'll be able to kill something else in there - namely, ->scm_work_list.
Still it is better to not add the second pointer, task->task_works can point to the last work, and last_work->next points to the first one.
Quoted 2 lines
1) kill
task_work->data; the only user that cares will allocatetask_work+ struct cred * and usecontainer_of()to get to it.
OK. See the patch below. I need to cleanup it somehow (and test of course), cred.h needs task_work.h but task_work.h needs task_struct.
And, David, can you suggest the good name for cred->xxx?
Quoted 2 lines
2) replace
current->task_workswith structtask_work*task_works, **last_work and replacehlist_nodeintask_workwith structtask_work*next;
Yes, but see above.
Quoted 1 line
3) at that point
task_workis equal in size (and layout, BTW) torcu_head.
Yep.
Quoted 1 line
[... snip ...]
I'll try to understand fput/scm issues later ;)
Oleg.
include/linux/cred.h +5 -1
@@ -18,6 +18,7 @@#include <linux/selinux.h> #include <linux/atomic.h> #include <linux/uidgid.h> +#include <linux/task_work.h> struct user_struct ;struct cred ;
@@ -149,7 +150,10 @@ struct cred {struct user_struct *user;/* real user ID subscription */ struct user_namespace *user_ns;/* user_ns the caps and keyrings are relative to. */ struct group_info *group_info;/* supplementary groups for euid/fsgid */ -struct rcu_head rcu;/* RCU deletion hook */ +union { +struct rcu_head rcu;/* RCU deletion hook */ +struct task_work xxx;/* keyctl_session_to_parent() */ + }; };extern void __put_cred (struct cred *);
include/linux/task_work.h +9 -8
@@ -1,9 +1,6 @@#ifndef _LINUX_TASK_WORK_H#define _LINUX_TASK_WORK_H -#include <linux/list.h> -#include <linux/sched.h> -struct task_work ;typedef void (*task_work_func_t )(struct task_work *);
@@ -24,10 +21,14 @@ int task_work_add(struct task_struct *task, struct task_work *twork, bool);struct task_work *task_work_cancel (struct task_struct *,task_work_func_t );void task_work_run (void ); -static inline void exit_task_work (struct task_struct *task) -{ -if (unlikely (!hlist_empty (&task->task_works))) -task_work_run (); -} +#define exit_task_work (__task) \ + do { \ + struct task_struct *task = (__task); \ + if (unlikely(!hlist_empty(&task->task_works))) \ + task_work_run(); \ + } while (0) + +#include <linux/list.h> +#include <linux/sched.h> #endif /* _LINUX_TASK_WORK_H */
security/keys/keyctl.c +11 -20
@@ -1456,8 +1456,8 @@ long keyctl_session_to_parent(void) {struct task_struct *me, *parent;const struct cred *mycred, *pcred; -struct task_work *newwork, *oldwork;key_ref_t keyring_r; +struct task_work *oldwork;struct cred *cred;int ret;
@@ -1465,20 +1465,16 @@ long keyctl_session_to_parent(void)if (IS_ERR (keyring_r))return PTR_ERR (keyring_r); - ret = -ENOMEM ; - newwork =kmalloc (sizeof (struct task_work ),GFP_KERNEL ); -if (!newwork) - goto error_keyring; -/* our parent is going to need a new cred struct, a new tgcred struct * and new security data, so we allocate them here to prevent ENOMEM in * our parent */ + ret = -ENOMEM ; cred =cred_alloc_blank ();if (!cred) - goto error_newwork; + goto error_keyring; cred->tgcred->session_keyring =key_ref_to_ptr (keyring_r); -init_task_work (newwork, key_change_session_keyring, cred); +init_task_work (&cred->xxx, key_change_session_keyring,NULL ); me = current;rcu_read_lock ();
@@ -1527,24 +1523,19 @@ long keyctl_session_to_parent(void)/* the replacement session keyring is applied just prior to userspace * restarting */ - ret =task_work_add (parent, newwork, true); + ret =task_work_add (parent, &cred->xxx, true);if (!ret) - newwork =NULL ; + cred =NULL ; unlock:write_unlock_irq (&tasklist_lock);rcu_read_unlock (); -if (oldwork) { -put_cred (oldwork->data); -kfree (oldwork); - } -if (newwork) { -put_cred (newwork->data); -kfree (newwork); - } + +if (oldwork) + put_cred(container_of(oldwork, struct cred, xxx)); +if (cred) +put_cred (cred);return ret; -error_newwork: -kfree (newwork); error_keyring:key_ref_put (keyring_r);return ret;
security/keys/process_keys.c +1 -2
@@ -837,9 +837,8 @@ error:void key_change_session_keyring (struct task_work *twork) {const struct cred *old =current_cred (); -struct cred *new = twork->data; + struct cred *new = container_of(twork, struct cred, xxx); -kfree (twork);if (unlikely (current->flags &PF_EXITING )) {put_cred (new);return ;
Quoted 11 lines · Oleg Nesterov
Quoted 6 lines · Al Viro
What we ought to
do instead of that is honestly keeping both the head of the (single-linked) list and pointer to pointer to its last element. Sure, that'll eat one more word intask_struct. And it doesn't matter, since we'll be able to kill something else in there - namely, ->scm_work_list.Still it is better to not add the second pointer,
task->task_workscan point to the last work, andlast_work->nextpoints to the first one.
So. task_struct has the single "struct task_work *last_twork" pointer, task_work has ->next. I'll try to cleanup task_work_cancel() a bit, but this all doesn't look too complicated, see below. (untested, probably buggy, but hopefully close to correct).
Or do you still think we need the 2nd pointer in task_struct?
Oleg.
inttask_work_add(struct task_struct *task, struct task_work *twork, bool notify)
{
unsigned long flags;
struct task_work *last;
int err = -ESRCH;#ifndef TIF_NOTIFY_RESUME
if (notify)
return -ENOTSUPP;
#endif
/*
* We must not insert the new work if the task has already passed
* exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait()
* and check PF_EXITING under pi_lock.
*/
raw_spin_lock_irqsave(&task->pi_lock, flags);
if (likely(!(task->flags & PF_EXITING))) {
last = task->last_twork ?: twork;
task->last_twork = twork;
twork->next = last->next;
last->next = twork;
err = 0;
}
raw_spin_unlock_irqrestore(&task->pi_lock, flags);
/* test_and_set_bit() implies mb(), see tracehook_notify_resume(). */
if (likely(!err) && notify)
set_notify_resume(task);
return err;
}struct task_work *task_work_cancel(struct task_struct *task, task_work_func_t func)
{
unsigned long flags;
struct task_work *last;
struct task_work *twork;
struct task_work *prev;
raw_spin_lock_irqsave(&task->pi_lock, flags);
last = twork = task->last_twork;
if (!last)
goto unlock;
do {
prev = twork;
twork = twork->next;
if (twork->func != func)
continue;
prev->next = twork->next;
if (twork == last) {
if (prev == twork)
prev = NULL;
task->last_twork = prev;
}
goto unlock;
} while (twork != last);
twork = NULL;unlock:
raw_spin_unlock_irqrestore(&task->pi_lock, flags); return twork; }
void task_work_run(void)
{
struct task_struct *task = current;
struct task_work *last;
struct task_work *twork;
struct task_work *next;
raw_spin_lock_irq(&task->pi_lock);
last = task->last_twork;
task->last_twork = NULL;
raw_spin_unlock_irq(&task->pi_lock);
if (unlikely(!last))
return;
next = last->next;
do {
twork = next;
next = twork->next;
twork->func(twork);
} while (twork != last);
}Peter Zijlstra
maintainerQuoted 7 lines · Al Viro
3) at that point
task_workis equal in size (and layout, BTW) torcu_head. So we can add it into the same union in struct file where we already havelist_headandrcu_head. No space eaten up.fput()would, once the counter reaches 0, remove the file from list (the only place walking that list skips the ones with zero refcount anyway) and, if we are in a normal process, usetask_work_add()to have__fput()done to it. If we are in kernel thread or atomic context, just move the sucker to global list and useschedule_work()to have said list emptied and everything in it fed to__fput().
So we're now Ok with doing fput() async?
Last time I remember this coming up people thought this wasn't such a hot idea.
Al Viro
maintainerQuoted 9 lines · Peter Zijlstra
Quoted 7 lines · Al Viro
3) at that point
task_workis equal in size (and layout, BTW) torcu_head. So we can add it into the same union in struct file where we already havelist_headandrcu_head. No space eaten up.fput()would, once the counter reaches 0, remove the file from list (the only place walking that list skips the ones with zero refcount anyway) and, if we are in a normal process, usetask_work_add()to have__fput()done to it. If we are in kernel thread or atomic context, just move the sucker to global list and useschedule_work()to have said list emptied and everything in it fed to__fput().
Quoted 4 lines
So we're now Ok with doing
fput()async?Last time I remember this coming up people thought this wasn't such a hot idea.
You mean, doing that from RCU callbacks? Still a bad idea, IMO; you will end up with a context switch and unpleasantness with delayed user-visible effects of syscalls. With aio we did have a delayed execution of fput() anyway; all that has changed there is that we use generic mechanism instead of home-grown analog thereof.
I'll need to reread that thread to comment on the specifics (had been too long ago; I don't remember the details), but... See Linus' objections to full-async fput() circa this April or March. There's a reason why this patchset uses task_work_add() whenever possible.
Peter Zijlstra
maintainerQuoted 1 line · Al Viro
You mean, doing that from RCU callbacks?
Indirectly, yeah, but the RCU callback would schedule it or whatever.
Quoted 4 lines
Still a bad idea, IMO; you will end up with a context
switch and unpleasantness with delayed user-visible effects of syscalls. With aio we did have a delayed execution offput()anyway; all that has changed there is that we use generic mechanism instead of home-grown analog thereof.
Right, the delayed effect is the main concern. The example in the referred thread was unmount() returning -EBUSY after the last close()/munmap().
Quoted 3 lines
I'll need to reread that thread to comment on the specifics (had been too long ago; I don't remember the details), but... See Linus' objections to full-async
fput()circa this April or March. There's a reason why this patchset usestask_work_add()whenever possible.
Ok, I'll try and find that thread, so the advantage of task_work_add() is that you'll keep the work in the task that caused it wherever possible, right -- provided its actually sitll around.
If we make fput() deferable in general we'll be sure to grow some 'fun' cases. So are we going to add a sync against unmount someplace to avoid these un-expected -EBUSY things?
Al Viro
maintainerQuoted 11 lines · Peter Zijlstra
Quoted 3 lines
I'll need to reread that thread to comment on the specifics (had been too long ago; I don't remember the details), but... See Linus' objections to full-async
fput()circa this April or March. There's a reason why this patchset usestask_work_add()whenever possible.Ok, I'll try and find that thread, so the advantage of
task_work_add()is that you'll keep the work in the task that caused it wherever possible, right -- provided its actually sitll around.If we make
fput()deferable in general we'll be sure to grow some 'fun' cases. So are we going to add a sync against unmount someplace to avoid these un-expected-EBUSYthings?
No. We are not. And I don't care how tasty some application might be - this way madness lies. If you have _anything_ doing async fput() (which is perfectly possible with the current mainline), you have to live with the fact that yes, filesystem will be busy while your mechanism, whatever it might be, is in use.
BTW, you do realize that delayed fput() may mean delayed fs shutdown? Think of umount -l while you have an opened file...