>
On Thu, Mar 17, 2022 at 5:27 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 07-03-22 17:57:39, Amir Goldstein wrote:
> > fsnotify_add_mark() and variants implicitly take a reference on inode
> > when attaching a mark to an inode.
> >
> > Make that behavior opt-out with the flag FSNOTIFY_ADD_MARK_NO_IREF.
> >
> > Instead of taking the inode reference when attaching connector to inode
> > and dropping the inode reference when detaching connector from inode,
> > take the inode reference on attach of the first mark that wants to hold
> > an inode reference and drop the inode reference on detach of the last
> > mark that wants to hold an inode reference.
> >
> > This leaves the choice to the backend whether or not to pin the inode
> > when adding an inode mark.
> >
> > This is intended to be used when adding a mark with ignored mask that is
> > used for optimization in cases where group can afford getting unneeded
> > events and reinstate the mark with ignored mask when inode is accessed
> > again after being evicted.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Couple of notes below.
>
> > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> > index 190df435919f..f71b6814bfa7 100644
> > --- a/fs/notify/mark.c
> > +++ b/fs/notify/mark.c
> > @@ -213,6 +213,17 @@ static void *fsnotify_detach_connector_from_object(
> > if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
> > inode = fsnotify_conn_inode(conn);
> > inode->i_fsnotify_mask = 0;
> > +
> > + pr_debug("%s: inode=%p iref=%u sb_connectors=%lu icount=%u\n",
> > + __func__, inode, atomic_read(&conn->proxy_iref),
> > + atomic_long_read(&inode->i_sb->s_fsnotify_connectors),
> > + atomic_read(&inode->i_count));
>
> Are these pr_debug() prints that useful? My experience is they get rarely used
> after the code is debugged... If you think some places are useful longer
> term, tracepoints are probably easier to use these days?
>
Well in this case, the debug prints didn't even help me find the refcount bug
I had in the code, so not useful.
> > +
> > + /* Unpin inode when detaching from connector */
> > + if (atomic_read(&conn->proxy_iref))
> > + atomic_set(&conn->proxy_iref, 0);
> > + else
> > + inode = NULL;
>
> proxy_iref is always manipulated under conn->lock so there's no need for
> atomic operations here.
Of course. Much simpler!
>
> > } else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
> > fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
> > } else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) {
> > @@ -240,12 +251,43 @@ static void fsnotify_final_mark_destroy(struct fsnotify_mark *mark)
> > /* Drop object reference originally held by a connector */
> > static void fsnotify_drop_object(unsigned int type, void *objp)
> > {
> > + struct inode *inode = objp;
> > +
> > if (!objp)
> > return;
> > /* Currently only inode references are passed to be dropped */
> > if (WARN_ON_ONCE(type != FSNOTIFY_OBJ_TYPE_INODE))
> > return;
> > - fsnotify_put_inode_ref(objp);
> > +
> > + pr_debug("%s: inode=%p sb_connectors=%lu, icount=%u\n", __func__,
> > + inode, atomic_long_read(&inode->i_sb->s_fsnotify_connectors),
> > + atomic_read(&inode->i_count));
> > +
> > + fsnotify_put_inode_ref(inode);
> > +}
> > +
> > +/* Drop the proxy refcount on inode maintainted by connector */
> > +static struct inode *fsnotify_drop_iref(struct fsnotify_mark_connector *conn,
> > + unsigned int *type)
> > +{
> > + struct inode *inode = fsnotify_conn_inode(conn);
> > +
> > + if (WARN_ON_ONCE(!inode || conn->type != FSNOTIFY_OBJ_TYPE_INODE))
> > + return NULL;
> > +
> > + pr_debug("%s: inode=%p iref=%u sb_connectors=%lu icount=%u\n",
> > + __func__, inode, atomic_read(&conn->proxy_iref),
> > + atomic_long_read(&inode->i_sb->s_fsnotify_connectors),
> > + atomic_read(&inode->i_count));
> > +
> > + if (WARN_ON_ONCE(!atomic_read(&conn->proxy_iref)) ||
> > + !atomic_dec_and_test(&conn->proxy_iref))
> > + return NULL;
> > +
> > + fsnotify_put_inode_ref(inode);
>
> You cannot call fsnotify_put_inode_ref() here because the function is
> called under conn->lock and iput() can sleep... You need to play similar
> game with passing inode pointer like
> fsnotify_detach_connector_from_object() does.
That was a plain bug.
That game is already being played and fsnotify_drop_object()
is responsible of iput().
BTW, I realized that incrementing/decrementing s_fsnotify_connectors
along with ihold/iput is completely useless, so I will remove the
fsnotify_{put,get}_inode_ref() helpers.
> > + *type = FSNOTIFY_OBJ_TYPE_INODE;
> > +
> > + return inode;
> > }
> >
> > void fsnotify_put_mark(struct fsnotify_mark *mark)
> > @@ -275,6 +317,9 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
> > free_conn = true;
> > } else {
> > __fsnotify_recalc_mask(conn);
> > + /* Unpin inode on last mark that wants inode refcount held */
> > + if (mark->flags & FSNOTIFY_MARK_FLAG_HAS_IREF)
> > + objp = fsnotify_drop_iref(conn, &type);
> > }
>
> This is going to be interesting. What if the connector got detached from
> the inode before fsnotify_put_mark() was called? Then iref_proxy would be
> already 0 and we would barf? I think
> fsnotify_detach_connector_from_object() needs to drop inode reference but
> leave iref_proxy alone for this to work. fsnotify_drop_iref() would then
> drop inode reference only if iref_proxy reaches 0 and conn->objp != NULL...
>
Good catch! but solution I think the is way simpler:
+ /* Unpin inode on last mark that wants inode refcount held */
+ if (conn->type == FSNOTIFY_OBJ_TYPE_INODE &&
+ mark->flags & FSNOTIFY_MARK_FLAG_HAS_IREF)
+ objp = fsnotify_drop_iref(conn, &type);
(iref_proxy > 0) always infers a single i_count reference, so it makes
fsnotify_detach_connector_from_object() sets iref_proxy = 0 and
conn->type = FSNOTIFY_OBJ_TYPE_DETACHED, so we should be good here.
Thanks,
Amir.
