| Message ID | 20190516102641.6574-7-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sort out fsnotify_nameremove() mess | expand |
On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > This will allow generating fsnotify delete events after the > fsnotify_nameremove() hook is removed from d_delete(). > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/debugfs/inode.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > index acef14ad53db..bc96198df1d4 100644 > --- a/fs/debugfs/inode.c > +++ b/fs/debugfs/inode.c > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > } > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > +static void __debugfs_file_removed(struct dentry *dentry) > { > struct debugfs_fsdata *fsd; > > - simple_unlink(d_inode(parent), dentry); > - d_delete(dentry); What happened to this call? Why no unlinking anymore? > - > /* > * Paired with the closing smp_mb() implied by a successful > * cmpxchg() in debugfs_file_get(): either > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > int ret = 0; > > if (simple_positive(dentry)) { > - dget(dentry); > - if (!d_is_reg(dentry)) { > - if (d_is_dir(dentry)) > - ret = simple_rmdir(d_inode(parent), dentry); > - else > - simple_unlink(d_inode(parent), dentry); > - if (!ret) > - d_delete(dentry); > - } else { > - __debugfs_remove_file(dentry, parent); > - } > - dput(dentry); > + ret = simple_remove(d_inode(parent), dentry); > + if (d_is_reg(dentry)) Can't dentry be gone here? This doesn't seem to match the same pattern as before. What am I missing? confused, greg k-h
On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > This will allow generating fsnotify delete events after the > > fsnotify_nameremove() hook is removed from d_delete(). > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/debugfs/inode.c | 20 ++++---------------- > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > index acef14ad53db..bc96198df1d4 100644 > > --- a/fs/debugfs/inode.c > > +++ b/fs/debugfs/inode.c > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > } > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > +static void __debugfs_file_removed(struct dentry *dentry) > > { > > struct debugfs_fsdata *fsd; > > > > - simple_unlink(d_inode(parent), dentry); > > - d_delete(dentry); > > What happened to this call? Why no unlinking anymore? > > > - > > /* > > * Paired with the closing smp_mb() implied by a successful > > * cmpxchg() in debugfs_file_get(): either > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > int ret = 0; > > > > if (simple_positive(dentry)) { > > - dget(dentry); > > - if (!d_is_reg(dentry)) { > > - if (d_is_dir(dentry)) > > - ret = simple_rmdir(d_inode(parent), dentry); > > - else > > - simple_unlink(d_inode(parent), dentry); > > - if (!ret) > > - d_delete(dentry); > > - } else { > > - __debugfs_remove_file(dentry, parent); > > - } > > - dput(dentry); > > + ret = simple_remove(d_inode(parent), dentry); > > + if (d_is_reg(dentry)) > > Can't dentry be gone here? This doesn't seem to match the same pattern > as before. > > What am I missing? > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() After change, the helper only does the post delete stuff. simple_unlink() is now done inside simple_remove(). This debugfs patch depends on a patch that adds the simple_remove() helper. sorry for not mentioning this explicitly. Thanks, Amir.
On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > This will allow generating fsnotify delete events after the > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/debugfs/inode.c | 20 ++++---------------- > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > index acef14ad53db..bc96198df1d4 100644 > > > --- a/fs/debugfs/inode.c > > > +++ b/fs/debugfs/inode.c > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > } > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > { > > > struct debugfs_fsdata *fsd; > > > > > > - simple_unlink(d_inode(parent), dentry); > > > - d_delete(dentry); > > > > What happened to this call? Why no unlinking anymore? > > > > > - > > > /* > > > * Paired with the closing smp_mb() implied by a successful > > > * cmpxchg() in debugfs_file_get(): either > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > int ret = 0; > > > > > > if (simple_positive(dentry)) { > > > - dget(dentry); > > > - if (!d_is_reg(dentry)) { > > > - if (d_is_dir(dentry)) > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > - else > > > - simple_unlink(d_inode(parent), dentry); > > > - if (!ret) > > > - d_delete(dentry); > > > - } else { > > > - __debugfs_remove_file(dentry, parent); > > > - } > > > - dput(dentry); > > > + ret = simple_remove(d_inode(parent), dentry); > > > + if (d_is_reg(dentry)) > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > as before. > > > > What am I missing? > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > After change, the helper only does the post delete stuff. > simple_unlink() is now done inside simple_remove(). > This debugfs patch depends on a patch that adds the simple_remove() helper. > sorry for not mentioning this explicitly. Right. But Greg is right that simple_remove() may have dropped last dentry reference and thus you now pass freed dentry to d_is_reg() and __debugfs_file_removed()? Honza
On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > > This will allow generating fsnotify delete events after the > > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/debugfs/inode.c | 20 ++++---------------- > > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > > index acef14ad53db..bc96198df1d4 100644 > > > > --- a/fs/debugfs/inode.c > > > > +++ b/fs/debugfs/inode.c > > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > > } > > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > > { > > > > struct debugfs_fsdata *fsd; > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > - d_delete(dentry); > > > > > > What happened to this call? Why no unlinking anymore? > > > > > > > - > > > > /* > > > > * Paired with the closing smp_mb() implied by a successful > > > > * cmpxchg() in debugfs_file_get(): either > > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > > int ret = 0; > > > > > > > > if (simple_positive(dentry)) { > > > > - dget(dentry); > > > > - if (!d_is_reg(dentry)) { > > > > - if (d_is_dir(dentry)) > > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > > - else > > > > - simple_unlink(d_inode(parent), dentry); > > > > - if (!ret) > > > > - d_delete(dentry); > > > > - } else { > > > > - __debugfs_remove_file(dentry, parent); > > > > - } > > > > - dput(dentry); > > > > + ret = simple_remove(d_inode(parent), dentry); > > > > + if (d_is_reg(dentry)) > > > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > > as before. > > > > > > What am I missing? > > > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > > After change, the helper only does the post delete stuff. > > simple_unlink() is now done inside simple_remove(). > > This debugfs patch depends on a patch that adds the simple_remove() helper. > > sorry for not mentioning this explicitly. > > Right. But Greg is right that simple_remove() may have dropped last dentry > reference and thus you now pass freed dentry to d_is_reg() and > __debugfs_file_removed()? > It seem so. Good spotting! Thanks, Amir.
On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote: > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > > > This will allow generating fsnotify delete events after the > > > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > --- > > > > > fs/debugfs/inode.c | 20 ++++---------------- > > > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > > > index acef14ad53db..bc96198df1d4 100644 > > > > > --- a/fs/debugfs/inode.c > > > > > +++ b/fs/debugfs/inode.c > > > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > > > } > > > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > > > { > > > > > struct debugfs_fsdata *fsd; > > > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > - d_delete(dentry); > > > > > > > > What happened to this call? Why no unlinking anymore? > > > > > > > > > - > > > > > /* > > > > > * Paired with the closing smp_mb() implied by a successful > > > > > * cmpxchg() in debugfs_file_get(): either > > > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > > > int ret = 0; > > > > > > > > > > if (simple_positive(dentry)) { > > > > > - dget(dentry); > > > > > - if (!d_is_reg(dentry)) { > > > > > - if (d_is_dir(dentry)) > > > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > > > - else > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > - if (!ret) > > > > > - d_delete(dentry); > > > > > - } else { > > > > > - __debugfs_remove_file(dentry, parent); > > > > > - } > > > > > - dput(dentry); > > > > > + ret = simple_remove(d_inode(parent), dentry); > > > > > + if (d_is_reg(dentry)) > > > > > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > > > as before. > > > > > > > > What am I missing? > > > > > > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > > > After change, the helper only does the post delete stuff. > > > simple_unlink() is now done inside simple_remove(). > > > This debugfs patch depends on a patch that adds the simple_remove() helper. > > > sorry for not mentioning this explicitly. > > > > Right. But Greg is right that simple_remove() may have dropped last dentry > > reference and thus you now pass freed dentry to d_is_reg() and > > __debugfs_file_removed()? > > > > It seem so. Good spotting! Yes, that's what I was trying to say. I don't think this conversion is correct, so you might either have to rework your simple_rmdir(), or this patch, to make it work properly. thanks, greg k-h
On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote: > > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > > > > This will allow generating fsnotify delete events after the > > > > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > --- > > > > > > fs/debugfs/inode.c | 20 ++++---------------- > > > > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > > > > index acef14ad53db..bc96198df1d4 100644 > > > > > > --- a/fs/debugfs/inode.c > > > > > > +++ b/fs/debugfs/inode.c > > > > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > > > > } > > > > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > > > > { > > > > > > struct debugfs_fsdata *fsd; > > > > > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > - d_delete(dentry); > > > > > > > > > > What happened to this call? Why no unlinking anymore? > > > > > > > > > > > - > > > > > > /* > > > > > > * Paired with the closing smp_mb() implied by a successful > > > > > > * cmpxchg() in debugfs_file_get(): either > > > > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > > > > int ret = 0; > > > > > > > > > > > > if (simple_positive(dentry)) { > > > > > > - dget(dentry); > > > > > > - if (!d_is_reg(dentry)) { > > > > > > - if (d_is_dir(dentry)) > > > > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > > > > - else > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > - if (!ret) > > > > > > - d_delete(dentry); > > > > > > - } else { > > > > > > - __debugfs_remove_file(dentry, parent); > > > > > > - } > > > > > > - dput(dentry); > > > > > > + ret = simple_remove(d_inode(parent), dentry); > > > > > > + if (d_is_reg(dentry)) > > > > > > > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > > > > as before. > > > > > > > > > > What am I missing? > > > > > > > > > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > > > > After change, the helper only does the post delete stuff. > > > > simple_unlink() is now done inside simple_remove(). > > > > This debugfs patch depends on a patch that adds the simple_remove() helper. > > > > sorry for not mentioning this explicitly. > > > > > > Right. But Greg is right that simple_remove() may have dropped last dentry > > > reference and thus you now pass freed dentry to d_is_reg() and > > > __debugfs_file_removed()? > > > > > > > It seem so. Good spotting! > > Yes, that's what I was trying to say. I don't think this conversion is > correct, so you might either have to rework your simple_rmdir(), or this > patch, to make it work properly. > To fix the correctness issue we will keep dget(dentry)/dput(dentry) in place both in __debugfs_remove() and in simple_remove(), i.e: dget(dentry); ret = simple_remove(d_inode(parent), dentry); if (d_is_reg(dentry)) __debugfs_file_removed(dentry); dput(dentry); Will this have addressed your concern? BTW, I forwarded you the dependency patch that is needed for the context of this review. Thanks, Amir.
On Thu, May 16, 2019 at 06:38:50PM +0300, Amir Goldstein wrote: > On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote: > > > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote: > > > > > > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > > > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > > > > > This will allow generating fsnotify delete events after the > > > > > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > --- > > > > > > > fs/debugfs/inode.c | 20 ++++---------------- > > > > > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > > > > > index acef14ad53db..bc96198df1d4 100644 > > > > > > > --- a/fs/debugfs/inode.c > > > > > > > +++ b/fs/debugfs/inode.c > > > > > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > > > > > } > > > > > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > > > > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > > > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > > > > > { > > > > > > > struct debugfs_fsdata *fsd; > > > > > > > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > > - d_delete(dentry); > > > > > > > > > > > > What happened to this call? Why no unlinking anymore? > > > > > > > > > > > > > - > > > > > > > /* > > > > > > > * Paired with the closing smp_mb() implied by a successful > > > > > > > * cmpxchg() in debugfs_file_get(): either > > > > > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > > > > > int ret = 0; > > > > > > > > > > > > > > if (simple_positive(dentry)) { > > > > > > > - dget(dentry); > > > > > > > - if (!d_is_reg(dentry)) { > > > > > > > - if (d_is_dir(dentry)) > > > > > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > > > > > - else > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > > - if (!ret) > > > > > > > - d_delete(dentry); > > > > > > > - } else { > > > > > > > - __debugfs_remove_file(dentry, parent); > > > > > > > - } > > > > > > > - dput(dentry); > > > > > > > + ret = simple_remove(d_inode(parent), dentry); > > > > > > > + if (d_is_reg(dentry)) > > > > > > > > > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > > > > > as before. > > > > > > > > > > > > What am I missing? > > > > > > > > > > > > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > > > > > After change, the helper only does the post delete stuff. > > > > > simple_unlink() is now done inside simple_remove(). > > > > > This debugfs patch depends on a patch that adds the simple_remove() helper. > > > > > sorry for not mentioning this explicitly. > > > > > > > > Right. But Greg is right that simple_remove() may have dropped last dentry > > > > reference and thus you now pass freed dentry to d_is_reg() and > > > > __debugfs_file_removed()? > > > > > > > > > > It seem so. Good spotting! > > > > Yes, that's what I was trying to say. I don't think this conversion is > > correct, so you might either have to rework your simple_rmdir(), or this > > patch, to make it work properly. > > > > To fix the correctness issue we will keep dget(dentry)/dput(dentry) > in place both in __debugfs_remove() and in simple_remove(), i.e: > > dget(dentry); > ret = simple_remove(d_inode(parent), dentry); > if (d_is_reg(dentry)) > __debugfs_file_removed(dentry); > dput(dentry); > > Will this have addressed your concern? Shouldn't you check for !d_is_reg before calling simple_remove()? > > BTW, I forwarded you the dependency patch that is needed for the > context of this review. I had dug it out of the original series when I reviewed this :) thanks, greg k-h
On Thu, May 16, 2019 at 7:52 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, May 16, 2019 at 06:38:50PM +0300, Amir Goldstein wrote: > > On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote: > > > > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote: > > > > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman > > > > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > > > > On Thu, May 16, 2019 at 01:26:33PM +0300, Amir Goldstein wrote: > > > > > > > > This will allow generating fsnotify delete events after the > > > > > > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > > > > > > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > > > --- > > > > > > > > fs/debugfs/inode.c | 20 ++++---------------- > > > > > > > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c > > > > > > > > index acef14ad53db..bc96198df1d4 100644 > > > > > > > > --- a/fs/debugfs/inode.c > > > > > > > > +++ b/fs/debugfs/inode.c > > > > > > > > @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, > > > > > > > > } > > > > > > > > EXPORT_SYMBOL_GPL(debugfs_create_symlink); > > > > > > > > > > > > > > > > -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) > > > > > > > > +static void __debugfs_file_removed(struct dentry *dentry) > > > > > > > > { > > > > > > > > struct debugfs_fsdata *fsd; > > > > > > > > > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > > > - d_delete(dentry); > > > > > > > > > > > > > > What happened to this call? Why no unlinking anymore? > > > > > > > > > > > > > > > - > > > > > > > > /* > > > > > > > > * Paired with the closing smp_mb() implied by a successful > > > > > > > > * cmpxchg() in debugfs_file_get(): either > > > > > > > > @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) > > > > > > > > int ret = 0; > > > > > > > > > > > > > > > > if (simple_positive(dentry)) { > > > > > > > > - dget(dentry); > > > > > > > > - if (!d_is_reg(dentry)) { > > > > > > > > - if (d_is_dir(dentry)) > > > > > > > > - ret = simple_rmdir(d_inode(parent), dentry); > > > > > > > > - else > > > > > > > > - simple_unlink(d_inode(parent), dentry); > > > > > > > > - if (!ret) > > > > > > > > - d_delete(dentry); > > > > > > > > - } else { > > > > > > > > - __debugfs_remove_file(dentry, parent); > > > > > > > > - } > > > > > > > > - dput(dentry); > > > > > > > > + ret = simple_remove(d_inode(parent), dentry); > > > > > > > > + if (d_is_reg(dentry)) > > > > > > > > > > > > > > Can't dentry be gone here? This doesn't seem to match the same pattern > > > > > > > as before. > > > > > > > > > > > > > > What am I missing? > > > > > > > > > > > > > > > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed() > > > > > > After change, the helper only does the post delete stuff. > > > > > > simple_unlink() is now done inside simple_remove(). > > > > > > This debugfs patch depends on a patch that adds the simple_remove() helper. > > > > > > sorry for not mentioning this explicitly. > > > > > > > > > > Right. But Greg is right that simple_remove() may have dropped last dentry > > > > > reference and thus you now pass freed dentry to d_is_reg() and > > > > > __debugfs_file_removed()? > > > > > > > > > > > > > It seem so. Good spotting! > > > > > > Yes, that's what I was trying to say. I don't think this conversion is > > > correct, so you might either have to rework your simple_rmdir(), or this > > > patch, to make it work properly. > > > > > > > To fix the correctness issue we will keep dget(dentry)/dput(dentry) > > in place both in __debugfs_remove() and in simple_remove(), i.e: > > > > dget(dentry); > > ret = simple_remove(d_inode(parent), dentry); > > if (d_is_reg(dentry)) > > __debugfs_file_removed(dentry); > > dput(dentry); > > > > Will this have addressed your concern? > > Shouldn't you check for !d_is_reg before calling simple_remove()? Current code does simple_unlink() or simple_rmdir() for !d_is_reg and simple_unlink() + stuff for d_is_reg. New helper does simple_unlink() or simple_rmdir() as appropriate for all cases, so all we are left is with + stuff only for d_is_reg. But anyway, Al is calling this patch set off. So will continue this discussion on another thread some other time. Thanks, Amir.
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index acef14ad53db..bc96198df1d4 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, } EXPORT_SYMBOL_GPL(debugfs_create_symlink); -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent) +static void __debugfs_file_removed(struct dentry *dentry) { struct debugfs_fsdata *fsd; - simple_unlink(d_inode(parent), dentry); - d_delete(dentry); - /* * Paired with the closing smp_mb() implied by a successful * cmpxchg() in debugfs_file_get(): either @@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent) int ret = 0; if (simple_positive(dentry)) { - dget(dentry); - if (!d_is_reg(dentry)) { - if (d_is_dir(dentry)) - ret = simple_rmdir(d_inode(parent), dentry); - else - simple_unlink(d_inode(parent), dentry); - if (!ret) - d_delete(dentry); - } else { - __debugfs_remove_file(dentry, parent); - } - dput(dentry); + ret = simple_remove(d_inode(parent), dentry); + if (d_is_reg(dentry)) + __debugfs_file_removed(dentry); } return ret; }
This will allow generating fsnotify delete events after the fsnotify_nameremove() hook is removed from d_delete(). Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/debugfs/inode.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)