Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Jan Kara <jack@suse.cz>, Alexander Viro <viro@zeniv.linux.org.uk>
Cc:     Amir Goldstein <amir73il@gmail.com>, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Stephen Brennan <stephen.s.brennan@oracle.com>
Subject: [PATCH 1/2] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
Date:   Mon, 17 Oct 2022 21:12:32 -0700
Message-Id: <20221018041233.376977-2-stephen.s.brennan@oracle.com>
In-Reply-To: <20221018041233.376977-1-stephen.s.brennan@oracle.com>
References: <20221013222719.277923-1-stephen.s.brennan@oracle.com>
 <20221018041233.376977-1-stephen.s.brennan@oracle.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ctcxDY6qKAkd4XYhUo2FYRHdZK5EbMtzaAUpVA5QNks6F8cUPp+jdA/sELMG?=
 =?us-ascii?Q?EpCLUoVHWLDtdmvkE+1iX6jCfzM3siLVZ9c+vvZyXxoNkcroVDyFZ1RDsHBc?=
 =?us-ascii?Q?KNX1JlRiP+Tojwep+7cW3pEaT7PyEti6xmCgvpTt3Z+6fScWpWbJy5p7DMdD?=
 =?us-ascii?Q?1dWxiXt9UNz/wLKIfxTS4glNB+NqwkWWSsuWMuZ1c8bReRpTRqIyB96ZzPWK?=
 =?us-ascii?Q?EfDXyAYLKFLCL0LlbZiCtcEhOUBuiYkQGbfwLDz0p2bfp5DCcNzbaUVt64nJ?=
 =?us-ascii?Q?IV9MMucRZbBQFkAdwskY/SvKwvKmrhululo2TsFK29ce11EuphJr1sVQCWjD?=
 =?us-ascii?Q?nHE57L8CyJztAdcLqaPTX2jGIlpnkvzZBJjvHL7pLEbFuLcEKOEJRQv2gaEC?=
 =?us-ascii?Q?yYd/Im6+GoHwM5LCEdk5OqzteiKD1FXrJsDWcBmv8pAJaGeVRU3SQTlGT5Q/?=
 =?us-ascii?Q?5DMSxMW5ePjwt+GfhZgTEJVbd0kEe0r6bKfzmZ6NaoKy7oMJ5RPYTIsep9rK?=
 =?us-ascii?Q?IvfTovPOJWnmVfKGWtvFWfgAVpLBsfIv2tQB6kFqSD2aEEJ/A0C67+JF1Vow?=
 =?us-ascii?Q?DVUBFIhJbwSsEF5ixNQjC7nlKGDw3KKv87Qk1xcTDaI0F1Di9pvi59b1omlJ?=
 =?us-ascii?Q?FJuLEQB//uSQ86O1voQiDHfflGmcJVIWtkcJJgE0xC7AEJ96QzRth2PRPiDO?=
 =?us-ascii?Q?2d5l0Z8n4qBlMTiWKHQKhaqQtd+QCpWwjPf45auOkpLqULONZ1hojbaRuFVO?=
 =?us-ascii?Q?PNQQnlfFgPBtzxqgaf8A/DfcC7YjSycbGz2UGyFl4ifV2keHf2Kv7uPA7NFq?=
 =?us-ascii?Q?nxyHDbmwvhgFIdDc60LMkxq8X1lyfA253OP+4xMPoaZ83qudu4B5JGLFKGx+?=
 =?us-ascii?Q?+oDKC9PHaXZJcX1+IbFbJSBGTv+eOw6VtKFGqeZK1dFFL7YvcWdP7ZC7T9zE?=
 =?us-ascii?Q?CXtYZq9/lCRH9hapkPAXAQEyap1w04s+hxYiNWZjdbaTb3pVzuPLAnws2Ugv?=
 =?us-ascii?Q?jOOZZVhbpFrHrSGPV2SSXdEwLQWQekUOB60w3wszEGIl9zu2H+tuLyzLcSO8?=
 =?us-ascii?Q?UJkN9I1WX/gN8+ei/iD9DE4LLQ5Tq86JWXNltnW+5R1DknM92Dl3/4t2kWKz?=
 =?us-ascii?Q?IDq3yCkEV9KnIZEGq4kTEJNyruUWpeHDILVnWUyNq01TjhYidmGashbvHNYp?=
 =?us-ascii?Q?MZx36FD4J9meFbBye9Alv9TH6LBJZX5LWfdeiT5b5b6me1nfU2Xm3P1CogJL?=
 =?us-ascii?Q?DPQc7Zkwe6zU+coennt4JBd1gwcam0XPLk2fGoruEI5J6vWL0+3CDp3BgMm/?=
 =?us-ascii?Q?zoWKaiBHpk+tpxDiQcZjykR8DduCBn2gb8dSFpYb+x6Fmy3M7qjpxK/qlIVw?=
 =?us-ascii?Q?7uDRzikooGlWW77h4G3Z2zdeBr73TIsjq2ph74PLdB9Egdsbuu694+uFpEr+?=
 =?us-ascii?Q?wMagFlg6Idqtg4Up93BKWXoM03BpvPqtl/TXUBkPs0j6hDsw2+XZ08UKYBZy?=
 =?us-ascii?Q?bEh9N57Sf67qYj7CE654bHrCYIyW1YI/WD6zxjE4t3vfJ278KVMexM1zuo5g?=
 =?us-ascii?Q?9ZzORmjIMP6aEHfFkFM2sWQmRBDK2iLETZdOSZFt13HDd/msX5yR7usOxdM0?=
 =?us-ascii?Q?lg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 606e2529-1e30-42bf-ce09-08dab0befd1e
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2022 04:12:36.4525
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: l9oUkbtSLu/mnZpqiMoO5mNZhPO4hTN1SytDJFSU7+HM7x3xF+I8iA26jUinBQYpj4+FXrdmPIrrHFplw7UBgz30yphH1qLR4BIRaQ9slZA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4567
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-17_13,2022-10-17_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0 bulkscore=0
 malwarescore=0 phishscore=0 mlxlogscore=999 spamscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2210180022
X-Proofpoint-ORIG-GUID: Wej28yRvfJB63sqGzOKOsKimxwAjcCi-
X-Proofpoint-GUID: Wej28yRvfJB63sqGzOKOsKimxwAjcCi-
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

When an inode is interested in events on its children, it must set
DCACHE_FSNOTIFY_PARENT_WATCHED flag on all its children. Currently, when
the fsnotify connector is removed and i_fsnotify_mask becomes zero, we
lazily allow __fsnotify_parent() to do this the next time we see an
event on a child.

However, if the list of children is very long (e.g., in the millions),
and lots of activity is occurring on the directory, then it's possible
for many CPUs to end up blocked on the inode spinlock in
__fsnotify_update_child_flags(). Each CPU will then redundantly iterate
over the very long list of children. This situation can cause soft
lockups.

To avoid this, stop lazily updating child flags in __fsnotify_parent().
Protect the child flag update with i_rwsem held exclusive, to ensure
that we only iterate over the child list when it's absolutely necessary,
and even then, only once.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---

Notes:

It seems that there are two implementation options for this, regarding
what i_rwsem protects:

1. Both updates to i_fsnotify_mask, and the child dentry flags, or
2. Only updates to the child dentry flags

I wanted to do #1, but it got really tricky with fsnotify_put_mark(). We
don't want to hold the inode lock whenever we decrement the refcount,
but if we don't, then we're stuck holding a spinlock when the refcount
goes to zero, and we need to grab the inode rwsem to synchronize the
update to the child flags. I'm sure there's a way around this, but I
didn't keep going with it.

With #1, as currently implemented, we have the unfortunate effect of
that a mark can be added, can see that no update is required, and
return, despite the fact that the flag update is still in progress on a
different CPU/thread. From our discussion, that seems to be the current
status quo, but I wanted to explicitly point that out. If we want to
move to #1, it should be possible with some work.

 fs/notify/fsnotify.c | 12 ++++++++--
 fs/notify/mark.c     | 55 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 7974e91ffe13..e887a195983b 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -207,8 +207,16 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
 	p_mask = fsnotify_inode_watches_children(p_inode);
-	if (unlikely(parent_watched && !p_mask))
-		__fsnotify_update_child_dentry_flags(p_inode);
+	if (unlikely(parent_watched && !p_mask)) {
+		/*
+		 * Flag would be cleared soon by
+		 * __fsnotify_update_child_dentry_flags(), but as an
+		 * optimization, clear it now.
+		 */
+		spin_lock(&dentry->d_lock);
+		dentry->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
+		spin_unlock(&dentry->d_lock);
+	}
 
 	/*
 	 * Include parent/name in notification either if some notification
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index c74ef947447d..da9f944fcbbb 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -184,15 +184,36 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
  */
 void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 {
+	struct inode *inode = NULL;
+	int watched_before, watched_after;
+
 	if (!conn)
 		return;
 
-	spin_lock(&conn->lock);
-	__fsnotify_recalc_mask(conn);
-	spin_unlock(&conn->lock);
-	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		__fsnotify_update_child_dentry_flags(
-					fsnotify_conn_inode(conn));
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
+		/*
+		 * For inodes, we may need to update flags on the child
+		 * dentries. To ensure these updates occur exactly once,
+		 * synchronize the recalculation with the inode mutex.
+		 */
+		inode = fsnotify_conn_inode(conn);
+		spin_lock(&conn->lock);
+		watched_before = fsnotify_inode_watches_children(inode);
+		__fsnotify_recalc_mask(conn);
+		watched_after = fsnotify_inode_watches_children(inode);
+		spin_unlock(&conn->lock);
+
+		inode_lock(inode);
+		if ((watched_before && !watched_after) ||
+		    (!watched_before && watched_after)) {
+			__fsnotify_update_child_dentry_flags(inode);
+		}
+		inode_unlock(inode);
+	} else {
+		spin_lock(&conn->lock);
+		__fsnotify_recalc_mask(conn);
+		spin_unlock(&conn->lock);
+	}
 }
 
 /* Free all connectors queued for freeing once SRCU period ends */
@@ -295,6 +316,8 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 	struct fsnotify_mark_connector *conn = READ_ONCE(mark->connector);
 	void *objp = NULL;
 	unsigned int type = FSNOTIFY_OBJ_TYPE_DETACHED;
+	struct inode *inode = NULL;
+	int watched_before, watched_after;
 	bool free_conn = false;
 
 	/* Catch marks that were actually never attached to object */
@@ -311,17 +334,31 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 	if (!refcount_dec_and_lock(&mark->refcnt, &conn->lock))
 		return;
 
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
+		inode = fsnotify_conn_inode(conn);
+		watched_before = fsnotify_inode_watches_children(inode);
+	}
+
 	hlist_del_init_rcu(&mark->obj_list);
 	if (hlist_empty(&conn->list)) {
 		objp = fsnotify_detach_connector_from_object(conn, &type);
 		free_conn = true;
+		watched_after = 0;
 	} else {
 		objp = __fsnotify_recalc_mask(conn);
 		type = conn->type;
+		watched_after = fsnotify_inode_watches_children(inode);
 	}
 	WRITE_ONCE(mark->connector, NULL);
 	spin_unlock(&conn->lock);
 
+	if (inode) {
+		inode_lock(inode);
+		if (watched_before && !watched_after)
+			__fsnotify_update_child_dentry_flags(inode);
+		inode_unlock(inode);
+	}
+
 	fsnotify_drop_object(type, objp);
 
 	if (free_conn) {
@@ -331,12 +368,6 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 		spin_unlock(&destroy_lock);
 		queue_work(system_unbound_wq, &connector_reaper_work);
 	}
-	/*
-	 * Note that we didn't update flags telling whether inode cares about
-	 * what's happening with children. We update these flags from
-	 * __fsnotify_parent() lazily when next event happens on one of our
-	 * children.
-	 */
 	spin_lock(&destroy_lock);
 	list_add(&mark->g_list, &destroy_list);
 	spin_unlock(&destroy_lock);
-- 
2.34.1