| Message ID | 20220329074904.2980320-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Evictable fanotify marks | expand |
On Tue 29-03-22 10:48:52, Amir Goldstein wrote: > s_fsnotify_connectors is elevated for every inode mark in addition to > the refcount already taken by the inode connector. > > This is a relic from s_fsnotify_inode_refs pre connector era. > Remove those unneeded recounts. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> I disagree it is a relict. fsnotify_sb_delete() relies on s_fsnotify_connectors to wait for all connectors to be properly torn down on unmount so that we don't get "Busy inodes after unmount" error messages (and use-after-free issues). Am I missing something? Honza > --- > fs/notify/mark.c | 21 +++------------------ > 1 file changed, 3 insertions(+), 18 deletions(-) > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index b1443e66ba26..698ed0a1a47e 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -169,21 +169,6 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work) > } > } > > -static void fsnotify_get_inode_ref(struct inode *inode) > -{ > - ihold(inode); > - atomic_long_inc(&inode->i_sb->s_fsnotify_connectors); > -} > - > -static void fsnotify_put_inode_ref(struct inode *inode) > -{ > - struct super_block *sb = inode->i_sb; > - > - iput(inode); > - if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) > - wake_up_var(&sb->s_fsnotify_connectors); > -} > - > static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn) > { > struct super_block *sb = fsnotify_connector_sb(conn); > @@ -245,7 +230,7 @@ static void fsnotify_drop_object(unsigned int type, void *objp) > /* Currently only inode references are passed to be dropped */ > if (WARN_ON_ONCE(type != FSNOTIFY_OBJ_TYPE_INODE)) > return; > - fsnotify_put_inode_ref(objp); > + iput(objp); > } > > void fsnotify_put_mark(struct fsnotify_mark *mark) > @@ -519,7 +504,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, > } > if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) { > inode = fsnotify_conn_inode(conn); > - fsnotify_get_inode_ref(inode); > + ihold(inode); > } > fsnotify_get_sb_connectors(conn); > > @@ -530,7 +515,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, > if (cmpxchg(connp, NULL, conn)) { > /* Someone else created list structure for us */ > if (inode) > - fsnotify_put_inode_ref(inode); > + iput(inode); > fsnotify_put_sb_connectors(conn); > kmem_cache_free(fsnotify_mark_connector_cachep, conn); > } > -- > 2.25.1 >
On Tue, Apr 5, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 29-03-22 10:48:52, Amir Goldstein wrote: > > s_fsnotify_connectors is elevated for every inode mark in addition to > > the refcount already taken by the inode connector. > > > > This is a relic from s_fsnotify_inode_refs pre connector era. > > Remove those unneeded recounts. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > I disagree it is a relict. fsnotify_sb_delete() relies on > s_fsnotify_connectors to wait for all connectors to be properly torn down > on unmount so that we don't get "Busy inodes after unmount" error messages > (and use-after-free issues). Am I missing something? > I meant it is a relic from the time before s_fsnotify_inode_refs became s_fsnotify_connectors. Nowadays, one s_fsnotify_connectors refcount per connector is enough. No need for one refcount per inode. Open code the the sequence: if (inode) fsnotify_put_inode_ref(inode); fsnotify_put_sb_connectors(conn); To see how silly it is. Thanks, Amir.
On Tue 05-04-22 16:09:00, Amir Goldstein wrote: > On Tue, Apr 5, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > On Tue 29-03-22 10:48:52, Amir Goldstein wrote: > > > s_fsnotify_connectors is elevated for every inode mark in addition to > > > the refcount already taken by the inode connector. > > > > > > This is a relic from s_fsnotify_inode_refs pre connector era. > > > Remove those unneeded recounts. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > I disagree it is a relict. fsnotify_sb_delete() relies on > > s_fsnotify_connectors to wait for all connectors to be properly torn down > > on unmount so that we don't get "Busy inodes after unmount" error messages > > (and use-after-free issues). Am I missing something? > > > > I meant it is a relic from the time before s_fsnotify_inode_refs became > s_fsnotify_connectors. > > Nowadays, one s_fsnotify_connectors refcount per connector is enough. > No need for one refcount per inode. > > Open code the the sequence: > if (inode) > fsnotify_put_inode_ref(inode); > fsnotify_put_sb_connectors(conn); > > To see how silly it is. I see your point and I agree the general direction makes sense but technically I think your patch is buggy. Because notice that we do fsnotify_put_sb_connectors() in fsnotify_detach_connector_from_object() so after this call there's nothing blocking umount while we can be still holding inode references from some marks attached to this connector. So racing inode mark destruction & umount can lead to "Busy inodes after umount" messages. Honza
On Wed, Apr 6, 2022 at 8:47 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 05-04-22 16:09:00, Amir Goldstein wrote: > > On Tue, Apr 5, 2022 at 3:54 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Tue 29-03-22 10:48:52, Amir Goldstein wrote: > > > > s_fsnotify_connectors is elevated for every inode mark in addition to > > > > the refcount already taken by the inode connector. > > > > > > > > This is a relic from s_fsnotify_inode_refs pre connector era. > > > > Remove those unneeded recounts. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > > > I disagree it is a relict. fsnotify_sb_delete() relies on > > > s_fsnotify_connectors to wait for all connectors to be properly torn down > > > on unmount so that we don't get "Busy inodes after unmount" error messages > > > (and use-after-free issues). Am I missing something? > > > > > > > I meant it is a relic from the time before s_fsnotify_inode_refs became > > s_fsnotify_connectors. > > > > Nowadays, one s_fsnotify_connectors refcount per connector is enough. > > No need for one refcount per inode. > > > > Open code the the sequence: > > if (inode) > > fsnotify_put_inode_ref(inode); > > fsnotify_put_sb_connectors(conn); > > > > To see how silly it is. > > I see your point and I agree the general direction makes sense but > technically I think your patch is buggy. Because notice that we do > fsnotify_put_sb_connectors() in fsnotify_detach_connector_from_object() so > after this call there's nothing blocking umount while we can be still > holding inode references from some marks attached to this connector. So > racing inode mark destruction & umount can lead to "Busy inodes after > umount" messages. > Yes, I see it now. IIRC, it was just a cleanup patch, I can remove it and amend the rest of the series. Thanks, Amir.
diff --git a/fs/notify/mark.c b/fs/notify/mark.c index b1443e66ba26..698ed0a1a47e 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -169,21 +169,6 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work) } } -static void fsnotify_get_inode_ref(struct inode *inode) -{ - ihold(inode); - atomic_long_inc(&inode->i_sb->s_fsnotify_connectors); -} - -static void fsnotify_put_inode_ref(struct inode *inode) -{ - struct super_block *sb = inode->i_sb; - - iput(inode); - if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) - wake_up_var(&sb->s_fsnotify_connectors); -} - static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn) { struct super_block *sb = fsnotify_connector_sb(conn); @@ -245,7 +230,7 @@ static void fsnotify_drop_object(unsigned int type, void *objp) /* Currently only inode references are passed to be dropped */ if (WARN_ON_ONCE(type != FSNOTIFY_OBJ_TYPE_INODE)) return; - fsnotify_put_inode_ref(objp); + iput(objp); } void fsnotify_put_mark(struct fsnotify_mark *mark) @@ -519,7 +504,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, } if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) { inode = fsnotify_conn_inode(conn); - fsnotify_get_inode_ref(inode); + ihold(inode); } fsnotify_get_sb_connectors(conn); @@ -530,7 +515,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, if (cmpxchg(connp, NULL, conn)) { /* Someone else created list structure for us */ if (inode) - fsnotify_put_inode_ref(inode); + iput(inode); fsnotify_put_sb_connectors(conn); kmem_cache_free(fsnotify_mark_connector_cachep, conn); }
s_fsnotify_connectors is elevated for every inode mark in addition to the refcount already taken by the inode connector. This is a relic from s_fsnotify_inode_refs pre connector era. Remove those unneeded recounts. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/mark.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-)