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?
> +
> + /* 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.
> } 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.
> + *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...
> WRITE_ONCE(mark->connector, NULL);
> spin_unlock(&conn->lock);
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
