fsnotify: tree-watching support
in [Kernel]
|
Prev: [PATCH -next] radio_timberdale: depends on I2c
Next: [PATCH 28/28] debug_core: Turn off tracing while in the debugger
From: Randy Dunlap on 12 Feb 2010 16:30 On 02/12/10 13:10, Joris Dolderer wrote: > Add tree-watching support to fsnotify. > Signed-off-by: Joris Dolderer <vorstadtkind(a)googlemail.com> > --- Hi, The patch is mangled. I don't have that kind of problem with sylpheed, so I'm surprised that you had it with claws. We usually ask that one sends a patch to oneself and verifies that it is correct (not mangled, not whitespace damaged, etc.) before posting on the mailing list. Also, please include a diffstat summary in patches. See Documentation/SubmittingPatches for more info on that. thanks, -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Randy Dunlap on 13 Feb 2010 14:20 On 02/13/10 01:05, Joris Dolderer wrote: > Add tree-watching support to fsnotify. > Hope mail works now... Yes, much better, thanks. The following review just concerns documentation... > Signed-off-by: Joris Dolderer <vorstadtkind(a)googlemail.com> > --- > fs/debugfs/inode.c | 8 - > fs/namei.c | 8 - > fs/notify/fsnotify.c | 188 +++++++++++++++++++++++------ > fs/notify/fsnotify.h | 1 > fs/notify/inode_mark.c | 46 ++++++- > include/linux/dcache.h | 3 > include/linux/fsnotify.h | 55 +++++--- > include/linux/fsnotify_backend.h | 51 +++++-- > 8 files changed, 279 insertions(+), 81 deletions(-) > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 037e878..17cd902 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c [snip] > +/* Notify this dentry's ancestors about a child's events. */ > +void __fsnotify_ancestors(struct dentry *dentry, __u32 mask) > +{ > + if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) { > + struct dentry *parent; > + struct inode *p_inode; > + bool should_update_children = false; > + bool send = false; > + > + spin_lock(&dentry->d_lock); > + > + parent = dentry->d_parent; > + p_inode = parent->d_inode; > > - if (fsnotify_inode_watches_children(p_inode)) { > - if (p_inode->i_fsnotify_mask & mask) { > + if (fsnotify_inode_watches_children(p_inode)) { > + if (p_inode->i_fsnotify_mask & mask) { > + dget(parent); > + send = true; > + } > + } else { > + /* > + * The parent doesn't care about events on it's children but its (yes, it's just moved, but please correct it) ("it's" means "it is", not possessive) > + * at least one child thought it did. We need to run all the > + * children and update their d_flags to let them know p_inode > + * doesn't care about them any more. > + */ > dget(parent); > - send = true; > + should_update_children = true; > } [snip] > +} > +EXPORT_SYMBOL_GPL(__fsnotify_ancestors); > + > +/* > + * notify tree-watching ancestors > + * @dentry: The dentry the walkup should start with > + * @file_name: The string that should be appended to this dentries' path > + * @file_len: The length of this string > + */ Please use kernel-doc notation for this and other exported symbols. See Documentation/kernel-doc-nano-HOWTO.txt for details, or ask me if you have questions about it. E.g.: /** * fsnotify_far_ancestors - notify tree-watching ancestors * @dentry: The dentry the walkup should start with * @file_name: The string that should be appended to this dentries' path * @file_len: The length of this string * @mask: <description> */ > +void fsnotify_far_ancestors(struct dentry *dentry, const unsigned char *file_name, int file_len, __u32 mask) > +{ .... > } > -EXPORT_SYMBOL_GPL(__fsnotify_parent); > +EXPORT_SYMBOL_GPL(fsnotify_far_ancestors); > > /* > * This is the main call to fsnotify. The VFS calls into hook specific functions > diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c > index 3165d85..67ad9cb 100644 > --- a/fs/notify/inode_mark.c > +++ b/fs/notify/inode_mark.c > @@ -195,14 +216,16 @@ void fsnotify_destroy_mark_by_entry(struct fsnotify_mark_entry *entry) > > /* > * __fsnotify_update_child_dentry_flags(inode); > + * or __fsnotify_update_descents; > * > * I really want to call that, but we can't, we have no idea if the inode > * still exists the second we drop the entry->lock. > * > * The next time an event arrive to this inode from one of it's children arrives its > - * __fsnotify_parent will see that the inode doesn't care about it's > - * children and will update all of these flags then. So really this > - * is just a lazy update (and could be a perf win...) > + * __fsnotify_ancestors resp. fsnotify_far_ancestors will see that the What is "resp." ? > + * inode doesn't care about it's children and will update all of these its > + * flags then. So really this is just a lazy update (and could be a > + * perf win...) > */ > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index 4d6f47b..1bea473 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h [snip] > -static inline int fsnotify_inode_watches_children(struct inode *inode) > +static inline bool fsnotify_inode_watches_something(struct inode *inode, u32 what) > { > - /* FS_EVENT_ON_CHILD is set if the inode may care */ > - if (!(inode->i_fsnotify_mask & FS_EVENT_ON_CHILD)) > + /* what is set if the inode may care */ > + if (!(inode->i_fsnotify_mask & what)) > return 0; return false; > /* this inode might care about child events, does it care about the > * specific set of events that can happen on a child? */ > return inode->i_fsnotify_mask & FS_EVENTS_POSS_ON_CHILD; > } -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Randy Dunlap on 13 Feb 2010 14:40 On 02/13/10 11:31, Joris Dolderer wrote: > Shall I, now, resubmit immediately or wait for other reviews? I would wait 2-3 days for other reviews. -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Joris Dolderer on 13 Feb 2010 14:40 Shall I, now, resubmit immediately or wait for other reviews? On Sat, 13 Feb 2010 11:12:15 -0800 Randy Dunlap <rdunlap(a)xenotime.net> wrote: > On 02/13/10 01:05, Joris Dolderer wrote: > > Add tree-watching support to fsnotify. > > Hope mail works now... > > Yes, much better, thanks. > > The following review just concerns documentation... > > > Signed-off-by: Joris Dolderer <vorstadtkind(a)googlemail.com> > > --- > > fs/debugfs/inode.c | 8 - > > fs/namei.c | 8 - > > fs/notify/fsnotify.c | 188 +++++++++++++++++++++++------ > > fs/notify/fsnotify.h | 1 > > fs/notify/inode_mark.c | 46 ++++++- > > include/linux/dcache.h | 3 > > include/linux/fsnotify.h | 55 +++++--- > > include/linux/fsnotify_backend.h | 51 +++++-- > > 8 files changed, 279 insertions(+), 81 deletions(-) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 037e878..17cd902 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > [snip] > > > +/* Notify this dentry's ancestors about a child's events. */ > > +void __fsnotify_ancestors(struct dentry *dentry, __u32 mask) > > +{ > > + if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) { > > + struct dentry *parent; > > + struct inode *p_inode; > > + bool should_update_children = false; > > + bool send = false; > > + > > + spin_lock(&dentry->d_lock); > > + > > + parent = dentry->d_parent; > > + p_inode = parent->d_inode; > > > > - if (fsnotify_inode_watches_children(p_inode)) { > > - if (p_inode->i_fsnotify_mask & mask) { > > + if (fsnotify_inode_watches_children(p_inode)) { > > + if (p_inode->i_fsnotify_mask & mask) { > > + dget(parent); > > + send = true; > > + } > > + } else { > > + /* > > + * The parent doesn't care about events on it's children but > > its > (yes, it's just moved, but please correct it) > ("it's" means "it is", not possessive) > > > + * at least one child thought it did. We need to run all the > > + * children and update their d_flags to let them know p_inode > > + * doesn't care about them any more. > > + */ > > dget(parent); > > - send = true; > > + should_update_children = true; > > } > > [snip] > > > +} > > +EXPORT_SYMBOL_GPL(__fsnotify_ancestors); > > + > > +/* > > + * notify tree-watching ancestors > > + * @dentry: The dentry the walkup should start with > > + * @file_name: The string that should be appended to this dentries' path > > + * @file_len: The length of this string > > + */ > > Please use kernel-doc notation for this and other exported symbols. > See Documentation/kernel-doc-nano-HOWTO.txt for details, or ask me if you > have questions about it. > > E.g.: > > /** > * fsnotify_far_ancestors - notify tree-watching ancestors > * @dentry: The dentry the walkup should start with > * @file_name: The string that should be appended to this dentries' path > * @file_len: The length of this string > * @mask: <description> > */ > > > > +void fsnotify_far_ancestors(struct dentry *dentry, const unsigned char *file_name, int file_len, __u32 mask) > > +{ > ... > > > } > > -EXPORT_SYMBOL_GPL(__fsnotify_parent); > > +EXPORT_SYMBOL_GPL(fsnotify_far_ancestors); > > > > /* > > * This is the main call to fsnotify. The VFS calls into hook specific functions > > > diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c > > index 3165d85..67ad9cb 100644 > > --- a/fs/notify/inode_mark.c > > +++ b/fs/notify/inode_mark.c > > @@ -195,14 +216,16 @@ void fsnotify_destroy_mark_by_entry(struct fsnotify_mark_entry *entry) > > > > /* > > * __fsnotify_update_child_dentry_flags(inode); > > + * or __fsnotify_update_descents; > > * > > * I really want to call that, but we can't, we have no idea if the inode > > * still exists the second we drop the entry->lock. > > * > > * The next time an event arrive to this inode from one of it's children > > arrives its > > > - * __fsnotify_parent will see that the inode doesn't care about it's > > - * children and will update all of these flags then. So really this > > - * is just a lazy update (and could be a perf win...) > > + * __fsnotify_ancestors resp. fsnotify_far_ancestors will see that the > > What is "resp." ? > > > + * inode doesn't care about it's children and will update all of these > > its > > > + * flags then. So really this is just a lazy update (and could be a > > + * perf win...) > > */ > > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > > index 4d6f47b..1bea473 100644 > > --- a/include/linux/fsnotify_backend.h > > +++ b/include/linux/fsnotify_backend.h > > [snip] > > > -static inline int fsnotify_inode_watches_children(struct inode *inode) > > +static inline bool fsnotify_inode_watches_something(struct inode *inode, u32 what) > > { > > - /* FS_EVENT_ON_CHILD is set if the inode may care */ > > - if (!(inode->i_fsnotify_mask & FS_EVENT_ON_CHILD)) > > + /* what is set if the inode may care */ > > + if (!(inode->i_fsnotify_mask & what)) > > return 0; > > return false; > > > /* this inode might care about child events, does it care about the > > * specific set of events that can happen on a child? */ > > return inode->i_fsnotify_mask & FS_EVENTS_POSS_ON_CHILD; > > } > > > -- > ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric Paris on 1 Mar 2010 14:50
On Fri, 2010-02-19 at 14:46 +0100, Joris Dolderer wrote: > Add tree-watching support to fsnotify. > See http://patchwork.kernel.org/patch/79171/ and http://patchwork.kernel.org/patch/79172/ for other patches. > Signed-off-by: Joris Dolderer <vorstadtkind(a)googlemail.com> > --- > fs/debugfs/inode.c | 8 + > fs/namei.c | 12 +- > fs/notify/fsnotify.c | 189 +++++++++++++++++++++++++++++++-------- > fs/notify/fsnotify.h | 1 > fs/notify/inode_mark.c | 46 ++++++++- > include/linux/dcache.h | 3 > include/linux/fsnotify.h | 55 +++++++---- > include/linux/fsnotify_backend.h | 51 +++++++--- > 8 files changed, 282 insertions(+), 83 deletions(-) > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 037e878..17cd902 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -76,55 +76,165 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode) > spin_unlock(&dcache_lock); > } > > -/* Notify this dentry's parent about a child's events. */ > -void __fsnotify_parent(struct dentry *dentry, __u32 mask) > +/* > + * Does the work when updating descents of a dentry > + */ > +static void fsnotify_update_descents(struct dentry *dentry, bool watched) > { > - struct dentry *parent; > - struct inode *p_inode; > - bool send = false; > - bool should_update_children = false; > + struct dentry *child; > + > + list_for_each_entry(child, &dentry->d_subdirs, d_u.d_child) { > + if (!child->d_inode) > + continue; > + > + spin_lock(&child->d_lock); > + > + if (watched) > + child->d_flags |= DCACHE_FSNOTIFY_ANCESTOR_WATCHED; > + else > + child->d_flags &= ~DCACHE_FSNOTIFY_ANCESTOR_WATCHED; > + > + if (!fsnotify_inode_watches_descents(child->d_inode)) { > + fsnotify_update_descents(child, watched); > + } > + > + spin_unlock(&child->d_lock); > + } > +} I haven't tested the code at all, just started to even peek at it and only made it this far..... Did you run this with lockdep enabled? Turn it on, understand and fix anything it complains about. Any kind of recursion, especially that which is not tail recursion is highly frowned upon. What happens when you do mkdir /tmp/subdir ln -s ../ /tmp/subdir/upone and then put a recursive watch on /tmp? I'm betting deadlock for 1 of 2 reasons, either you A) try to take the dentry->d_lock on subdir twice (deadlock) B) run indefinitely around the /tmp -> /tmp/subdir -> /tmp/subdir/upone -> /tmp/subidr -> /tmp/subdir/upone -> ... loop. Maybe I'm missing something, but both of these make this a non-starter.... Maybe symlinks don't break like that, but I'm betting I could do A with bind mounts if not B..... -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |