| Message ID | 1508920899-8115-2-git-send-email-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote: > This patch doesn't actually fix any bug, just paves the way for fixing mark > and group pinning. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Cc: <stable@vger.kernel.org> # v4.12 > --- > fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------ > 1 file changed, 41 insertions(+), 48 deletions(-) > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index 9991f8826734..982527ce6a58 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark) > atomic_inc(&mark->refcnt); > } > > -/* > - * Get mark reference when we found the mark via lockless traversal of object > - * list. Mark can be already removed from the list by now and on its way to be > - * destroyed once SRCU period ends. > - */ > -static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) > -{ > - return atomic_inc_not_zero(&mark->refcnt); > -} > - > static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > { > u32 new_mask = 0; > @@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) > FSNOTIFY_REAPER_DELAY); > } > > -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) > +/* > + * Get mark reference when we found the mark via lockless traversal of object > + * list. Mark can be already removed from the list by now and on its way to be > + * destroyed once SRCU period ends. > + */ > +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) > { > struct fsnotify_group *group; > > - if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark)) > - return false; > - > - if (iter_info->inode_mark) > - group = iter_info->inode_mark->group; > - else > - group = iter_info->vfsmount_mark->group; > + if (!mark) > + return true; > > + group = mark->group; > /* > * Since acquisition of mark reference is an atomic op as well, we can > * be sure this inc is seen before any effect of refcount increment. > */ > atomic_inc(&group->user_waits); > + if (atomic_inc_not_zero(&mark->refcnt)) > + return true; > + Please replicate the "We abuse notification_waitq on group shutdown" comment here as well to clarify why this wakeup is needed. Although comment says "for waiting for all marks pinned" and this code path did not change mark refcount, so is the wakeup really needed here? > + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) > + wake_up(&group->notification_waitq); > > - if (iter_info->inode_mark) { > - /* This can fail if mark is being removed */ > - if (!fsnotify_get_mark_safe(iter_info->inode_mark)) > - goto out_wait; > + return false; > +} > + > +static void fsnotify_put_mark_wait(struct fsnotify_mark *mark) > +{ > + if (mark) { > + struct fsnotify_group *group = mark->group; > + > + fsnotify_put_mark(mark); > + /* > + * We abuse notification_waitq on group shutdown for waiting for > + * all marks pinned when waiting for userspace. > + */ > + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) > + wake_up(&group->notification_waitq); > } > - if (iter_info->vfsmount_mark) { > - if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) > - goto out_inode; > +} > + > +bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) > +{ > + /* This can fail if mark is being removed */ > + if (!fsnotify_get_mark_safe(iter_info->inode_mark)) > + return false; > + if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) { > + fsnotify_put_mark_wait(iter_info->inode_mark); > + return false; > } > > /* > @@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) > srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx); > > return true; > -out_inode: > - if (iter_info->inode_mark) > - fsnotify_put_mark(iter_info->inode_mark); > -out_wait: > - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) > - wake_up(&group->notification_waitq); > - return false; > } > > void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info) > { > - struct fsnotify_group *group = NULL; > - > iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu); > - if (iter_info->inode_mark) { > - group = iter_info->inode_mark->group; > - fsnotify_put_mark(iter_info->inode_mark); > - } > - if (iter_info->vfsmount_mark) { > - group = iter_info->vfsmount_mark->group; > - fsnotify_put_mark(iter_info->vfsmount_mark); > - } > - /* > - * We abuse notification_waitq on group shutdown for waiting for all > - * marks pinned when waiting for userspace. > - */ > - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) > - wake_up(&group->notification_waitq); > + fsnotify_put_mark_wait(iter_info->inode_mark); > + fsnotify_put_mark_wait(iter_info->vfsmount_mark); > } > > /* > -- > 2.5.5 >
On Wed, Oct 25, 2017 at 3:06 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote: >> This patch doesn't actually fix any bug, just paves the way for fixing mark >> and group pinning. >> >> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> >> Cc: <stable@vger.kernel.org> # v4.12 >> --- >> fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------ >> 1 file changed, 41 insertions(+), 48 deletions(-) >> >> diff --git a/fs/notify/mark.c b/fs/notify/mark.c >> index 9991f8826734..982527ce6a58 100644 >> --- a/fs/notify/mark.c >> +++ b/fs/notify/mark.c >> @@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark) >> atomic_inc(&mark->refcnt); >> } >> >> -/* >> - * Get mark reference when we found the mark via lockless traversal of object >> - * list. Mark can be already removed from the list by now and on its way to be >> - * destroyed once SRCU period ends. >> - */ >> -static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) >> -{ >> - return atomic_inc_not_zero(&mark->refcnt); >> -} >> - >> static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) >> { >> u32 new_mask = 0; >> @@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) >> FSNOTIFY_REAPER_DELAY); >> } >> >> -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) >> +/* >> + * Get mark reference when we found the mark via lockless traversal of object >> + * list. Mark can be already removed from the list by now and on its way to be >> + * destroyed once SRCU period ends. >> + */ >> +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) Sorry, hit send too soon... This helper is now more than "get mark safe" how about fsnotify_get_mark_wait() to pair with fsnotify_put_mark_wait()? >> { >> struct fsnotify_group *group; >> >> - if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark)) >> - return false; >> - >> - if (iter_info->inode_mark) >> - group = iter_info->inode_mark->group; >> - else >> - group = iter_info->vfsmount_mark->group; >> + if (!mark) >> + return true; >> >> + group = mark->group; >> /* >> * Since acquisition of mark reference is an atomic op as well, we can >> * be sure this inc is seen before any effect of refcount increment. >> */ >> atomic_inc(&group->user_waits); >> + if (atomic_inc_not_zero(&mark->refcnt)) >> + return true; >> + > > Please replicate the "We abuse notification_waitq on group shutdown" comment > here as well to clarify why this wakeup is needed. Although comment says > "for waiting for all marks pinned" and this code path did not change > mark refcount, > so is the wakeup really needed here? > >> + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) >> + wake_up(&group->notification_waitq); >> >> - if (iter_info->inode_mark) { >> - /* This can fail if mark is being removed */ >> - if (!fsnotify_get_mark_safe(iter_info->inode_mark)) >> - goto out_wait; >> + return false; >> +} >> + >> +static void fsnotify_put_mark_wait(struct fsnotify_mark *mark) >> +{ >> + if (mark) { >> + struct fsnotify_group *group = mark->group; >> + >> + fsnotify_put_mark(mark); >> + /* >> + * We abuse notification_waitq on group shutdown for waiting for >> + * all marks pinned when waiting for userspace. >> + */ >> + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) >> + wake_up(&group->notification_waitq); >> } >> - if (iter_info->vfsmount_mark) { >> - if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) >> - goto out_inode; >> +} >> + >> +bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) >> +{ >> + /* This can fail if mark is being removed */ >> + if (!fsnotify_get_mark_safe(iter_info->inode_mark)) >> + return false; >> + if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) { >> + fsnotify_put_mark_wait(iter_info->inode_mark); >> + return false; >> } >> >> /* >> @@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) >> srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx); >> >> return true; >> -out_inode: >> - if (iter_info->inode_mark) >> - fsnotify_put_mark(iter_info->inode_mark); >> -out_wait: >> - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) >> - wake_up(&group->notification_waitq); >> - return false; >> } >> >> void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info) >> { >> - struct fsnotify_group *group = NULL; >> - >> iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu); >> - if (iter_info->inode_mark) { >> - group = iter_info->inode_mark->group; >> - fsnotify_put_mark(iter_info->inode_mark); >> - } >> - if (iter_info->vfsmount_mark) { >> - group = iter_info->vfsmount_mark->group; >> - fsnotify_put_mark(iter_info->vfsmount_mark); >> - } >> - /* >> - * We abuse notification_waitq on group shutdown for waiting for all >> - * marks pinned when waiting for userspace. >> - */ >> - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) >> - wake_up(&group->notification_waitq); >> + fsnotify_put_mark_wait(iter_info->inode_mark); >> + fsnotify_put_mark_wait(iter_info->vfsmount_mark); >> } >> >> /* >> -- >> 2.5.5 >>
On Wed, Oct 25, 2017 at 2:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) >>> +/* >>> + * Get mark reference when we found the mark via lockless traversal of object >>> + * list. Mark can be already removed from the list by now and on its way to be >>> + * destroyed once SRCU period ends. >>> + */ >>> +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) > > Sorry, hit send too soon... > This helper is now more than "get mark safe" > how about fsnotify_get_mark_wait() to pair with fsnotify_put_mark_wait()? It's get mark and pin mark->group. I think "safe" describes that adequately. Added comments to reflect that. Also renamed fsnotify_put_mark_wait() to fsnotify_put_mark_wake(), because that's what it does. Thanks, Miklos
diff --git a/fs/notify/mark.c b/fs/notify/mark.c index 9991f8826734..982527ce6a58 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark) atomic_inc(&mark->refcnt); } -/* - * Get mark reference when we found the mark via lockless traversal of object - * list. Mark can be already removed from the list by now and on its way to be - * destroyed once SRCU period ends. - */ -static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) -{ - return atomic_inc_not_zero(&mark->refcnt); -} - static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) { u32 new_mask = 0; @@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) FSNOTIFY_REAPER_DELAY); } -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) +/* + * Get mark reference when we found the mark via lockless traversal of object + * list. Mark can be already removed from the list by now and on its way to be + * destroyed once SRCU period ends. + */ +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark) { struct fsnotify_group *group; - if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark)) - return false; - - if (iter_info->inode_mark) - group = iter_info->inode_mark->group; - else - group = iter_info->vfsmount_mark->group; + if (!mark) + return true; + group = mark->group; /* * Since acquisition of mark reference is an atomic op as well, we can * be sure this inc is seen before any effect of refcount increment. */ atomic_inc(&group->user_waits); + if (atomic_inc_not_zero(&mark->refcnt)) + return true; + + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) + wake_up(&group->notification_waitq); - if (iter_info->inode_mark) { - /* This can fail if mark is being removed */ - if (!fsnotify_get_mark_safe(iter_info->inode_mark)) - goto out_wait; + return false; +} + +static void fsnotify_put_mark_wait(struct fsnotify_mark *mark) +{ + if (mark) { + struct fsnotify_group *group = mark->group; + + fsnotify_put_mark(mark); + /* + * We abuse notification_waitq on group shutdown for waiting for + * all marks pinned when waiting for userspace. + */ + if (atomic_dec_and_test(&group->user_waits) && group->shutdown) + wake_up(&group->notification_waitq); } - if (iter_info->vfsmount_mark) { - if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) - goto out_inode; +} + +bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) +{ + /* This can fail if mark is being removed */ + if (!fsnotify_get_mark_safe(iter_info->inode_mark)) + return false; + if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) { + fsnotify_put_mark_wait(iter_info->inode_mark); + return false; } /* @@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info) srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx); return true; -out_inode: - if (iter_info->inode_mark) - fsnotify_put_mark(iter_info->inode_mark); -out_wait: - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) - wake_up(&group->notification_waitq); - return false; } void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info) { - struct fsnotify_group *group = NULL; - iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu); - if (iter_info->inode_mark) { - group = iter_info->inode_mark->group; - fsnotify_put_mark(iter_info->inode_mark); - } - if (iter_info->vfsmount_mark) { - group = iter_info->vfsmount_mark->group; - fsnotify_put_mark(iter_info->vfsmount_mark); - } - /* - * We abuse notification_waitq on group shutdown for waiting for all - * marks pinned when waiting for userspace. - */ - if (atomic_dec_and_test(&group->user_waits) && group->shutdown) - wake_up(&group->notification_waitq); + fsnotify_put_mark_wait(iter_info->inode_mark); + fsnotify_put_mark_wait(iter_info->vfsmount_mark); } /*
This patch doesn't actually fix any bug, just paves the way for fixing mark and group pinning. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Cc: <stable@vger.kernel.org> # v4.12 --- fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 48 deletions(-)