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>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Amir Goldstein <amir73il@gmail.com>,
        Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v3 1/3] fsnotify: Use d_find_any_alias to get dentry
 associated with inode
In-Reply-To: <20221028001016.332663-2-stephen.s.brennan@oracle.com>
References: <20221021010310.29521-1-stephen.s.brennan@oracle.com>
 <20221028001016.332663-1-stephen.s.brennan@oracle.com>
 <20221028001016.332663-2-stephen.s.brennan@oracle.com>
Date:   Wed, 09 Nov 2022 17:12:58 -0800
Message-ID: <87pmdvfw85.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?RbOii+jQbvakmj6u8Kz3ZRERylSO1CRGFJ/WqioXG6qOPSuUSexa4oJGEUvH?=
 =?us-ascii?Q?2/ZlOh8SbsWSOCeHqn8mPLg/DYs0s8y7pYmLPwud9nHmAERLYVAbLQWmKHBZ?=
 =?us-ascii?Q?4J38yjqpccqg9Ce/bn2uW9jXOhzx6KsXGXUpeF5jvD91tL4b3gX7k2v7psN5?=
 =?us-ascii?Q?FxOIA1UJ5X+7TElAJQHfpwo3gIcKdEJDvRgH5h+7r0pNS9bk3FpMvrI4u9m/?=
 =?us-ascii?Q?adTGDwFaA44C3xdjmYIA1/gHcirSDax0hlJ2NFLFAvN1TMcCA0Oe8xbhjIwT?=
 =?us-ascii?Q?aZaHDoa/fXPGJqRXEVzOKnM+3nsBbAPvB/CQFt1PgGj1t3YFgWBbevEWu15u?=
 =?us-ascii?Q?1rtUmWHEJ9C/zflaXGfGcHQ2IuRjOc6NVhU0n8hfgjwSUB20GpcskvilP/W5?=
 =?us-ascii?Q?YOs3dYwBZynDlXxVii7cDw53TmW/JkVNKYU7Fzb0xZjHGwlsqk6rlM7X5wKt?=
 =?us-ascii?Q?g9VvP1Nl7HsZ6nGFdkuFKKJavz5KlONr57rMGvlqNowEbNuZlD4lTtIHykg5?=
 =?us-ascii?Q?3clthh/9/CzyhY2DgGt7kdxmtaH0N6sHOB0HuZHXPQsfCRy9gq+nZeATNTtx?=
 =?us-ascii?Q?v3YUtf09/dnBet7Y5y/GDZIPARiStauZvhjXuENi7HUP1AhjaluOnsAcHeAn?=
 =?us-ascii?Q?avzHzvup3zIMRlgvq+9y44yPQJWQczSQxlUvwgoRLYNSs61+wQ4chwIXT6PK?=
 =?us-ascii?Q?wtWbol8IP9yi7TRAIBv0aDO7KY9tk0k8+7qAjPAgFwGSDEPrzFkWrlfa/FjF?=
 =?us-ascii?Q?ToW5smpmkYSaKAiGfUoFFXCVpZ4STctv3ilCE7N8kgGV+xgKIvoO+N/aPr34?=
 =?us-ascii?Q?2my7lN1IDMeUhoFdSI/VmtS9L90wyVWmLMDkeiapepQZgMG/IF55wTlLMgNq?=
 =?us-ascii?Q?ffAWq9NDPuBx85cdgbeMOWLbjGEjyQjDuozIGGbG1nJ2rkh1j09W3Bg9iEu1?=
 =?us-ascii?Q?EgovsZ44uwqyMMxOKAYWzmxw0574qdFhGlxBdrNrhqsGlDsEKfG1b2mN9iUh?=
 =?us-ascii?Q?7PVo1bjPqd7SiS5AsNEFLk32eBdOENbLCjg4MMIswgcklHDtYBl6rS2W3Djv?=
 =?us-ascii?Q?tqmHLtOpwveKyXVtViVQQKK96mcO3ecWUJunxn7FeWb9yy4R3YUe5wFgGPLo?=
 =?us-ascii?Q?M8z+nWICW+kM8Jv/vCSyISbS21qmF+vJzbzP3f8u6GZrhqT+rVERFgtYBIiS?=
 =?us-ascii?Q?f/pb+6iRcEHbZFu7DlNZQxwOMw298vOLzY3ioI+5vsp4PY9n5x6qEfj1nbm1?=
 =?us-ascii?Q?RAwYcVO6MeZB1zE/N6yQqeeQs8NMQCHso7zSS4tpY11Q19cRWnR/64+Zy+hN?=
 =?us-ascii?Q?4JtZERICSKXMm+g/D6mFp0LXXEq13trG5gJma0WeoVyBX620GC/8F04WoY5u?=
 =?us-ascii?Q?4XDRQJTN5YWBJfiTym8y1peeCdNV7L5PFFWgj+ANKxZC+LLFj0snCd28hlqY?=
 =?us-ascii?Q?OguVT6W17DftyzK6d9wca0V0DwrImI92WGdGcPOq9HxZqLX2LzuNv1J8U3yD?=
 =?us-ascii?Q?LW+sirTkaQTdxmjQXhuCU9/iC96mcSrUHprLhQ6M7RSzeY282Pb3DM+mTUrh?=
 =?us-ascii?Q?eUHhiXHxTiq1hdtymuOrHH1Eziw2Pg7xibhlALOx7/cFG7B7wQNr4b6PLAh9?=
 =?us-ascii?Q?MA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 30f93874-b5e3-4940-82f8-08dac2b8b5ac
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Nov 2022 01:13:00.5350
 (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: Dd9Ngg498uilrTxmBXX4wsSaPlwKn06Z5SBK+JZh2nNGFAPNyZ9vU5wQbdmDU4UUnAs6mm0N++3m/+FgJ/l4EkPzOWPVSf5gOZ51MmtcSlE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR10MB7000
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-09_06,2022-11-09_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 adultscore=0
 malwarescore=0 mlxlogscore=999 mlxscore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211100007
X-Proofpoint-ORIG-GUID: ITgrbC2ReEoRXHn4QH649BLWlSWq5jeO
X-Proofpoint-GUID: ITgrbC2ReEoRXHn4QH649BLWlSWq5jeO
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

Stephen Brennan <stephen.s.brennan@oracle.com> writes:

> Rather than iterating over the inode's i_dentry (requiring holding the
> i_lock for the entire duration of the function), we know that there
> should be only one item in the list. Use d_find_any_alias() and no
> longer hold i_lock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Notes:
>     Changes since v2:
>     - Add newlines in block comment
>     - d_find_any_alias() returns a reference, which I was leaking. Add
>       a dput(alias) at the end.
>     - Add Amir's R-b
>
>  fs/notify/fsnotify.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 7974e91ffe13..7939aa911931 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -105,7 +105,7 @@ void fsnotify_sb_delete(struct super_block *sb)
>   */
>  void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  {
> -	struct dentry *alias;
> +	struct dentry *alias, *child;
>  	int watched;
>  
>  	if (!S_ISDIR(inode->i_mode))
> @@ -114,30 +114,28 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  	/* determine if the children should tell inode about their events */
>  	watched = fsnotify_inode_watches_children(inode);
>  
> -	spin_lock(&inode->i_lock);
> -	/* run all of the dentries associated with this inode.  Since this is a
> -	 * directory, there damn well better only be one item on this list */
> -	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> -		struct dentry *child;
> -
> -		/* run all of the children of the original inode and fix their
> -		 * d_flags to indicate parental interest (their parent is the
> -		 * original inode) */
> -		spin_lock(&alias->d_lock);
> -		list_for_each_entry(child, &alias->d_subdirs, d_child) {
> -			if (!child->d_inode)
> -				continue;
> +	/* Since this is a directory, there damn well better only be one child */
> +	alias = d_find_any_alias(inode);

Unfortunately, even this patch is not as simple as we would have liked
to believe. Running LTP, fanotify10 test, I reliably panic here because
d_find_any_alias(inode) returns NULL. I've fixed this in my branch
already with a simple if (alias) return watched;

Will continue to work through LTP tests (with lockdep enabled) and work
on the existing dnotify lock ordering issue.

>  
> -			spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> -			if (watched)
> -				child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> -			else
> -				child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> -			spin_unlock(&child->d_lock);
> -		}
> -		spin_unlock(&alias->d_lock);
> +	/*
> +	 * run all of the children of the original inode and fix their
> +	 * d_flags to indicate parental interest (their parent is the
> +	 * original inode)
> +	 */
> +	spin_lock(&alias->d_lock);
> +	list_for_each_entry(child, &alias->d_subdirs, d_child) {
> +		if (!child->d_inode)
> +			continue;
> +
> +		spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> +		if (watched)
> +			child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> +		else
> +			child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> +		spin_unlock(&child->d_lock);
>  	}
> -	spin_unlock(&inode->i_lock);
> +	spin_unlock(&alias->d_lock);
> +	dput(alias);
>  }
>  
>  /* Are inode/sb/mount interested in parent and name info with this event? */
> -- 
> 2.34.1