Messages in this thread | | | Date | Fri, 26 Jul 2013 16:49:00 +0200 | From | Oleg Nesterov <> | Subject | Re: Re: PATCH? debugfs_remove_recursive() must not rely on list_empty(d_subdirs) |
| |
On 07/26, Masami Hiramatsu wrote: > > (2013/07/26 5:04), Oleg Nesterov wrote: > > > > parent = dentry; > > down: > > mutex_lock(&parent->d_inode->i_mutex); > > list_for_each_entry_safe(child, next, &parent->d_subdirs, d_u.d_child) { > > Perhaps, you can use list_for_each_entry_safe_continue() here, as below. > > parent = dentry; > down: > child = list_first_entry_or_null(&parent->d_subdirs, > typeof(*child), d_u.d_child); > mutex_lock(&parent->d_inode->i_mutex); > > restart: > list_for_each_entry_safe_continue(child, next, &parent->d_subdirs, d_u.d_child) { > > > if (!debugfs_positive(child)) > > continue; > > > > /* XXX: simple_empty(child) instead ? */ > > if (!list_empty(&child->d_subdirs)) { > > mutex_unlock(&parent->d_inode->i_mutex); > > parent = child; > > goto down; > > } > > up: > > __debugfs_remove(child, parent); > > } > > Then, you can avoid jumping into the loop, just restart it from > parent as below.
Yes, but I'd prefer to jump into the loop. This is subjective, but looks a bit more understandable to me.
Because "goto down/up" are actually "call/return", and "jump up" looks like return-after-recursive-debugfs_remove_recursive-call.
However,
> if (child != dentry) > goto restart; > > > mutex_unlock(&parent->d_inode->i_mutex);
Yes, I realized this right after I sent the email ;)
We can factor out the final ->d_parent/mutex_lock if we check "child != dentry" instead of "parent != dentry".
I'll send the patch in a minute. Thanks.
Oleg.
| |