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;
Message-ID: <3505769d-9e7a-e76d-3aa7-286d689345b6@oracle.com>
Date:   Thu, 27 Jul 2023 14:30:42 +1000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.13.0
Subject: Re: [PATCH 1/2] kernfs: dont take i_lock on inode attr read
To:     Ian Kent <raven@themaw.net>,
        Anders Roxell <anders.roxell@linaro.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Tejun Heo <tj@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Minchan Kim <minchan@kernel.org>,
        Eric Sandeen <sandeen@sandeen.net>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Rick Lindsley <ricklind@linux.vnet.ibm.com>,
        David Howells <dhowells@redhat.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Carlos Maiolino <cmaiolino@redhat.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>,
        elver@google.com
References: <166606025456.13363.3829702374064563472.stgit@donald.themaw.net>
 <166606036215.13363.1288735296954908554.stgit@donald.themaw.net>
 <Y2BMonmS0SdOn5yh@slm.duckdns.org> <20221221133428.GE69385@mutt>
 <7815c8da-7d5f-c2c5-9dfd-7a77ac37c7f7@themaw.net>
 <e25ee08c-7692-4042-9961-a499600f0a49@app.fastmail.com>
 <9e35cf66-79ef-1f13-dc6b-b013c73a9fc6@themaw.net>
 <db933d76-1432-f671-8712-d94de35277d8@themaw.net> <20230718190009.GC411@mutt>
 <76fcd1fe-b5f5-dd6b-c74d-30c2300f3963@themaw.net>
 <ce407424e98bf5f2b186df5d28dd5749a6cbfa45.camel@themaw.net>
 <15eddad0-e73b-2686-b5ba-eaacc57b8947@themaw.net>
Content-Language: en-US
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <15eddad0-e73b-2686-b5ba-eaacc57b8947@themaw.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UW54MGM1SkxYTjVxdUI5R2REcGdxWWR0cGpFcXExdlN4bks1WWJITXZteXM5?=
 =?utf-8?B?QU1pRmx5TXFoZ1RrSjE1dnVFcG4zR3JBd0xISVdqRklaVUVQN1hrSklUOFMz?=
 =?utf-8?B?cThCcy9TVU1JOVhsZ2N4Qk1ITG50ejBXUStEaGsxOTV5QnFhYko4Z1FxL2wy?=
 =?utf-8?B?Q3hqVy9jbGJWdzM5akFBNEo1Y0Y0Vjg3TjJBMUx0dmVESXNlM3A4Q1YvRy9a?=
 =?utf-8?B?U2dYZVc0Q2JjOXl4RklkelFMdFZkd2VKOEgweG44U1N6MUpMclI3NXNUOFZH?=
 =?utf-8?B?Nm04MnVBVVZkZTlLTGNvZjBTRlVzcndHUmt6VlBxWCsxdCs4NjQ1dXc3RUFs?=
 =?utf-8?B?d2lIWW9rbGpPT0lkSnpFUlRCZ2VkYjF2d0dKa0pGQXR6S2xERmJoU1FjSWlu?=
 =?utf-8?B?NW4vV08yaTZvUjJtQUdBdllRS3A4d3h3eFgwUER2cW1ncGg1Wng4ZDcrQ3JC?=
 =?utf-8?B?bWdTY3h3SWR2OEp1bzJjL05sb3pYSGYzeENPRWtodXhkR1hmaHVMZUNkWFly?=
 =?utf-8?B?WGd5enZpMUtiOFRyT3BNM1hjUkpTTXk2cFZHZWRqbDM1N2pZNm5ScEROZzc3?=
 =?utf-8?B?UXo1WDlzdjNuTmlJQ0tOK0VQMHgxSFFpS3R0cDBRWlQ2SWo1dkoxK1Bzb3Fh?=
 =?utf-8?B?NmM5Y1laUXUwdnVLMEhHWmRJVkNscDZ2S2VTVWhUaitZL2JaVGhtOE96SFA5?=
 =?utf-8?B?Q0JsS0VWQ0VocXpJT3BoM0NxNXpMZDRjMFdZOFVyRUNrb0h0bmUyR0pvVXl3?=
 =?utf-8?B?b1pqaVNteG12RlN3dnJOYzFOR1JXSXV5ZjlJSStWTlQxNjBMOVJsYlliTVNC?=
 =?utf-8?B?SVMyL05oZFhrQ2h3MXZiR2Nhc2RDaFFmaTJaSDdjcVJwNFhnbWZTR1VmQmZR?=
 =?utf-8?B?L3c5R1NPTnU3RGJvQkd0QXR0UVZyZXQycWI3emJ6WjhQSzVQdm5LRzFmNDlz?=
 =?utf-8?B?dTUxZHFVeVcrVXlzMjBZMXh3ZWQxdjZVek91ZjhDd3lMU080YkNaaXNqM3Ni?=
 =?utf-8?B?MU1jWXNQMlF6YjJuSWkycklxb293c3pWMVRBZld6WDBCY0c0MWVhZG8zVVUw?=
 =?utf-8?B?OVNWL0VXMEIvQUFZR2NGYmNMTTJmbmE4ZDdGQTVpblY2QmZVSmJjOW4zdkg2?=
 =?utf-8?B?UmQ1a3c0UGdpQVdPaWhkeCtnUTc1dG9HMjU1UTFmY3pneE9tcUhsYm5JUkZq?=
 =?utf-8?B?WVBnT3RYWENwd3d1T1J5SUswRk01VVZpSVBxcHZuZEozaWZWNkt2S1NNT1dH?=
 =?utf-8?B?UzhBQmtNNGRuTUFhMFJ3bG44aDFmQXBwc3lGZHdFSlR1OVVVRUN4MlEraCtM?=
 =?utf-8?B?cHI1SzVhOTJoeXZCbWdFVDhTSldwbW5oN2N3SXluV0FCK3dRaXE3Qkd5YjZh?=
 =?utf-8?B?WXVIM0RWTksramFNNmMyN0g5TDkrT0JCa0hiaHkzTGRvZTVNeGVSdVJaczJ1?=
 =?utf-8?B?Y0Q4YzlVMlJwa3p1dFVVTjhrZDMzcjl1eVN5YXFYZUt1Zkt0Q25yWWdLd2Mr?=
 =?utf-8?B?V3U3eVRyNDlpUDlDUytjcWZJcVpndDA5ZGVSRjg1VWZ6VU5GZGpoTjJOaU1s?=
 =?utf-8?B?RWdNd3puZE1oOGFqWjRlY1c4RWN0Q0hqUUxFbmJ5bWhBR0R2c0N6U25DNXNK?=
 =?utf-8?B?ODNnVGlldHhWSnVUQ0d2K2FWWEk3cjY3ZERwREZ1MHNreCtqNndXYjJRZFRF?=
 =?utf-8?B?dHlZbXBFdmxETkEvV1M1bDFTMlpVK2tmWWlyaDliSTNrVXIxREdnWmN2d0RQ?=
 =?utf-8?B?RDI3TGdra3Q4NFcrN2RmNk5YYjAzT2hoR0RWNzk3SE41c3lQd1kwREVMbjYz?=
 =?utf-8?B?SEYzNFlGeER6SkVVaDM0d2lnVjAzcU5OVzlLcklITEJvTVFpQ2hSYWJNNGFQ?=
 =?utf-8?B?MmJINElaMFQwaFgxbk5hbndob2M4VWd2L1FBTk1FMDZwbUNWNkNZZXZqU3Zi?=
 =?utf-8?B?L1FjeWNVQThsSVppZlRPTkVwL0JSR1ZtQUh5QXpIMzVIT2NocG56K0FTZ1Vl?=
 =?utf-8?B?WUJYVWJRYmRIditQTkV6YVdQaVBWQmZ1d2R6Yi84cFZ0eTNSOWVnYW0rS2k2?=
 =?utf-8?B?NEh2bWhxL0QxVW9CWklRY3JMSXNWdU5MOXN5cU1vTVU4dUNCRlJ3MEFxZzJq?=
 =?utf-8?B?cGh3UGFkVjJ1TDhWMEx2a2Q1cGtXeFBOLzZiUm95UVdZZU9LSmNMMnIxMSty?=
 =?utf-8?B?UGc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?S0NONEhYUi9lOVowTms0UjZyaG5VMGVieG56bnFGeWppUWtTbExtL0x5cGdM?=
 =?utf-8?B?cGNobE5oN1FqZXovOHlORXFXS3hLYVlsSVovY1Rha1ZsYVNGY2g4OElHZnpV?=
 =?utf-8?B?QU1EaTd2K3pXdFlEYzBFRGNTMk9wSlFFc1VPV2diaURQeExmQU1zdC9scWJX?=
 =?utf-8?B?UHZxSkxyUlJmVTUxZStLcWcrZ3Z6R3BYM21KVVk5R3E1a2RQRm5HYzJEQnFk?=
 =?utf-8?B?dlBXTGFDZGVLNGtJeE9DTFl4aktNK1pwYzFnZHpTcDRncytmQXFMMjEyYjFK?=
 =?utf-8?B?RkZJSTluK1FaUFVMYzBKc2Zuc3J6b0JFaVI4ZUVFcGI3RFZPaTdpWk1UME9B?=
 =?utf-8?B?NWQ5RUI3VVBCWEdmLzVnMDdIQVQ0SWwrVGFEeTBCVEtaMmZHSTR3QytzYzhr?=
 =?utf-8?B?cGRTWi9va1Z2RWNjSFJ1SXhPZEZuQ1plbnhva3pQS1FhdzRzVE1WcEVDbmNk?=
 =?utf-8?B?U1MwODBOczk5T0hiUVRvbFJvNGgrWm4wZy9XbzhVZHJnV3lmS242SlZadXJT?=
 =?utf-8?B?cXhqU2UyVmpMZTZXejZiM1Z4MS82WEtKM2ZpSXJOdmpKYitHVnNOV2hvdG45?=
 =?utf-8?B?bzlEM3V0VE1OZlNrRE13NkNpcGpiSTd4RTRxa0Q0dXZzeHI3Q3YyeUlLSklK?=
 =?utf-8?B?cWJvMFZ1ejhhVXkvQmQrTWppS3BXak5wODIzbjJLY2hCbk8ySHk2QlYzdGpk?=
 =?utf-8?B?NDZxR3NBUTl6Y1dsYmo0UUFxNS83YndvaFpxbGdJeFBJS1dFdEN3Y01nSEdS?=
 =?utf-8?B?Vko3TjZta0o4TTRIRG1WenJ6NVlrRW8wZ2htMldjU2htQVg1S2pqaHJsRjRO?=
 =?utf-8?B?YlNVQkhXdWZITDBLd0l4OXhTU0ZqV3cxcUl6V3hlWTlsQk9uOEV0VUJ1SUF3?=
 =?utf-8?B?UnRnMkxvRyszaDdaTVBWNU5mRGtMV1pNSEtMUytMeUZ2cllYVUh1UUxnQVNZ?=
 =?utf-8?B?TWRONWtET3dCVVRSMXZoSm5aUm5JUTJpMHc4SVVycFhobnczS3Z4VnRtLzRK?=
 =?utf-8?B?N014U09jRHJiM1gway9OQ2Z3WGlsc2lLUCsvQXhZcXNhV3NvbEwwWWRtS0pT?=
 =?utf-8?B?S1pZbVJRYTJ3TkozR3lhYUI0dmI3TmRVOE9aaUoybUdnMG5nZlNFazd4RlNl?=
 =?utf-8?B?am5NdUxEVjJNMFdxM2Q5amE2VUw5SFlTYWhhTzZVTmF1M0dBYW1rTHR4TFA3?=
 =?utf-8?B?RUZNV1dwMlh2bTdjZzNSY1RSVmpsU0doZFRZUHd6U3M2N3RtK1dZbjg1VFhx?=
 =?utf-8?B?N0FjOGFsR1hVQUhxN1JnblMyT1N1Y3hsMzFRdmJRT0pKbW91dVFOV1kwTVhx?=
 =?utf-8?B?UWV5M3dBbmIvaStITVRnOGczb2h2WWYyaFAvQVFVK3FKbWVMclNsUFJzaUNi?=
 =?utf-8?B?OVpuYlZFU3JzR0ZNcFhvdzVtcDdtQ1JOMjVJUE9sS2E2QytzUlJPdkRrR24r?=
 =?utf-8?B?QWFEd3huKzNwdG43QUNCUHl2b0p5Wi9KUmhneTUyMG5JVkRzd1F0NW1iUFVU?=
 =?utf-8?B?Tlk5TnVvbTc2RWhpckJPd1hWWEZtZlJGQUgwTVlRYzFabG5jZ3VWSkRlOUE1?=
 =?utf-8?B?ZlBoUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a5e17fd4-f840-428a-2167-08db8e5a4358
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jul 2023 04:30:53.3496
 (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: FkZrK8+RSD23NEZo0LkzGAbu+SQHXS8VtBYarsmDTwrf9Y/oLd4b2lWnqzxL8uFEEc9sIIP1LBr83We5CaPtfg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4730
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-26_08,2023-07-26_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0
 adultscore=0 suspectscore=0 phishscore=0 malwarescore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2306200000 definitions=main-2307270040
X-Proofpoint-GUID: gy3kJRgwFeB4AHZASOBGTkVGtyMdKzb2
X-Proofpoint-ORIG-GUID: gy3kJRgwFeB4AHZASOBGTkVGtyMdKzb2
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE 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

Hello Ian,
Sorry for late reply. I was about to reply this week.

On 27/7/2023 10:38 am, Ian Kent wrote:
> On 20/7/23 10:03, Ian Kent wrote:
>> On Wed, 2023-07-19 at 12:23 +0800, Ian Kent wrote:

[...]
>> I do see a problem with recent changes.
>>
>> I'll send this off to Greg after I've done some testing (primarily just
>> compile and function).
>>
>> Here's a patch which describes what I found.
>>
>> Comments are, of course, welcome, ;)
> 
> Anders I was hoping you would check if/what lockdep trace
> 
> you get with this patch.
> 
> 
> Imran, I was hoping you would comment on my change as it
> 
> relates to the kernfs_iattr_rwsem changes.
> 
> 
> Ian
> 
>>
>> kernfs: fix missing kernfs_iattr_rwsem locking
>>
>> From: Ian Kent <raven@themaw.net>
>>
>> When the kernfs_iattr_rwsem was introduced a case was missed.
>>
>> The update of the kernfs directory node child count was also protected
>> by the kernfs_rwsem and needs to be included in the change so that the
>> child count (and so the inode n_link attribute) does not change while
>> holding the rwsem for read.
>>

kernfs direcytory node's child count changes in kernfs_(un)link_sibling and
these are getting invoked while adding (kernfs_add_one),
removing(__kernfs_remove) or moving (kernfs_rename_ns)a node. Each of these
operations proceed under kernfs_rwsem and I see each invocation of
kernfs_link/unlink_sibling during the above mentioned operations is happening
under kernfs_rwsem.
So the child count should still be protected by kernfs_rwsem and we should not
need to acquire kernfs_iattr_rwsem in kernfs_link/unlink_sibling.

Kindly let me know your thoughts. I would still like to see new lockdep traces
with this change.

Thanks,
Imran

>> Fixes: 9caf696142 (kernfs: Introduce separate rwsem to protect inode
>> attributes)
>>
>> Signed-off-by: Ian Kent <raven@themaw.net>
>> Cc: Anders Roxell <anders.roxell@linaro.org>
>> Cc: Imran Khan <imran.f.khan@oracle.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>   fs/kernfs/dir.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>> index 45b6919903e6..6e84bb69602e 100644
>> --- a/fs/kernfs/dir.c
>> +++ b/fs/kernfs/dir.c
>> @@ -383,9 +383,11 @@ static int kernfs_link_sibling(struct kernfs_node
>> *kn)
>>       rb_insert_color(&kn->rb, &kn->parent->dir.children);
>>         /* successfully added, account subdir number */
>> +    down_write(&kernfs_root(kn)->kernfs_iattr_rwsem);
>>       if (kernfs_type(kn) == KERNFS_DIR)
>>           kn->parent->dir.subdirs++;
>>       kernfs_inc_rev(kn->parent);
>> +    up_write(&kernfs_root(kn)->kernfs_iattr_rwsem);
>>         return 0;
>>   }
>> @@ -408,9 +410,11 @@ static bool kernfs_unlink_sibling(struct
>> kernfs_node *kn)
>>       if (RB_EMPTY_NODE(&kn->rb))
>>           return false;
>>   +    down_write(&kernfs_root(kn)->kernfs_iattr_rwsem);
>>       if (kernfs_type(kn) == KERNFS_DIR)
>>           kn->parent->dir.subdirs--;
>>       kernfs_inc_rev(kn->parent);
>> +    up_write(&kernfs_root(kn)->kernfs_iattr_rwsem);
>>         rb_erase(&kn->rb, &kn->parent->dir.children);
>>       RB_CLEAR_NODE(&kn->rb);
>>