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: <a0a311d6-bf23-4b62-e2ed-874c116bda6b@oracle.com>
Date:   Thu, 31 Mar 2022 13:37:01 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC PATCH 2/2] fs/dcache: Add negative-dentry-ratio config
Content-Language: en-US
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Matthew Wilcox <willy@infradead.org>,
        James Bottomley <James.Bottomley@hansenpartnership.com>,
        Gao Xiang <hsiangkao@linux.alibaba.com>,
        Dave Chinner <david@fromorbit.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Colin Walters <walters@verbum.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220331190827.48241-1-stephen.s.brennan@oracle.com>
 <20220331190827.48241-3-stephen.s.brennan@oracle.com>
 <YkYE7RXgP8hL9aLd@zeniv-ca.linux.org.uk>
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
In-Reply-To: <YkYE7RXgP8hL9aLd@zeniv-ca.linux.org.uk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NC9sSTYxQUdnZndKT0tNaEFZb3VCU1lBVEc0aEJDb3NXQmw3U0llSnNnaDBO?=
 =?utf-8?B?eVpKNXgzM3VqRzhva0lTUEZkWUR5c0x2cjlyMTU2WXFrMm1JdTMvZmhOMFor?=
 =?utf-8?B?L1ZaSW9CQnpUcDRlVXRYazdmb09zck9wYk1nRzZkOTdNS1cyclhIUzA2Sjh1?=
 =?utf-8?B?Q2hwdGNzcVRrQjk5TVpIb2hyNU5pa3J1K2dHek85NTdCTm5NMzZnMFBZSlFH?=
 =?utf-8?B?cEM5MVBmNUpUTVhOZzF0TTJaYm9vN00yVXlYaFQrck1YcUZTeUpWbFp1Vy9E?=
 =?utf-8?B?dnQ4dzJKU3dSM1REbldHQ0NRSDVEWlY0cUQwZG5mb2s4czFNN2MwZXA4TTdz?=
 =?utf-8?B?YTY0SUNTN0w2MVlFR1JFR2pQb2owdnp4dytrU0U1MlZCOVRQSlVZdWFnVW1t?=
 =?utf-8?B?enYvb0g1cEpIUEhzRE1YSExEc1RCUkdnMXpLQS9NY3J3S1U3SGVyNTMrRWpX?=
 =?utf-8?B?YW01bklrZzR6Q2tlTVNaT3dpUFdKcEdmcmFXUDlFakFhUkdFNGsweFdmb3Fz?=
 =?utf-8?B?eGhLZFVjMTBXQnpsVWZ6aGR4aGRyRmpQaTdSbzl3ZWdWWWZpMHloVmlkeEh3?=
 =?utf-8?B?R0dzTStzcVhxakc1YkphUEQ5V3ZzQXIxVEdXdlBUMU52NUpabWEzR3dtSlRJ?=
 =?utf-8?B?YWNTcFpFTVRMeno1ekh6S1czWkZ5VHQzT2k2M1NrTTVTczlaSy91eGdDdGh1?=
 =?utf-8?B?ajZGS1RiT1lFdzZ2YnMvVXJwMmVhRm4zNXpzdCtJcTFKNGVPZ0pZL0ZIZC9G?=
 =?utf-8?B?R0pJUEdWdXMxbFdjMkhWa25jTVRBV2t0aCtyNWgwMy8ydWV0YzUzNUNqa0NV?=
 =?utf-8?B?endRaDdpUnB4bmgzQkgyRnFob1JiVmVFWkhycWoyRkRNMWthSG9oMGgwUDJi?=
 =?utf-8?B?RVBOMmdOMW5PM3ZkUlM2VFkzajRDbDZyRXo3UXVMZVRYMnFpSHN3L2xyV3pT?=
 =?utf-8?B?SkxCdk9VNmZLUTlhaGRYNnVLeVhCNnF2cjc5MjBtby9xM3dML2I5THdreGYv?=
 =?utf-8?B?N3BhVmoxR3RzYUx4aUdQMzRrdUk0THpiZElqaVN0VGJJVWs0TWZHMkROengr?=
 =?utf-8?B?T1pNYjduL0tmTVJ0UlNFeGkwT0VPYTlqNFRYL2VuUTZaL2JjV2x1VVJTaThR?=
 =?utf-8?B?QXJDU2lwOUFXc25Ddmx3ZUVwSW1ZRnp3MXhZSHZvTXRvSnlRRVhjdHlxYURZ?=
 =?utf-8?B?VlloUm1ET2pFUnQvalYwMnN3eWhHOXUvemNvSDEvY1NaYkg4eldFMHdjd3hY?=
 =?utf-8?B?cjZsNDhqWE9rSEo1clJoekliWURZbGpJM2tTL29ZV1dOaDhyc2N0Y1NKTEFJ?=
 =?utf-8?B?cERWelZhR0RndmFZaW9BOE5aOUhOanhkRElTd2ZvQUtqdUs0ZVdaWlI3dWNl?=
 =?utf-8?B?T1R3aU5tenNXeG92SmNyeFhaK2IvOUVlNERBR0xQMlgxQ1Z3bXVyOXlPcXNx?=
 =?utf-8?B?MS95SGxzV3JSc1cvdWdkWGNwT0ZhUDRhZGRQMlVtcEFERzEraTlBOXZJRnNq?=
 =?utf-8?B?UUFyRUJ5S1RaWVVIT095cE5xOWE0TnB3M0QrcmtYeDZCdW1IOUY5UzJqdFdW?=
 =?utf-8?B?Sk9KbDJWMmIycnhaV0swVFNxTWc5eC9TRXUzMElVRFR6M09NZmpQSG5ObnlX?=
 =?utf-8?B?bDBqQUNkTEVxeTBUQVNmK0FQVy9iT3diSWdnV0FVTVd2U2lSRGgwemk3Z3dO?=
 =?utf-8?B?bTZKZGswL0V0Ny9ZZWFlM29xdjVlS2g1bVVlcTMxSzVoa0VxVE1xOXViUGRJ?=
 =?utf-8?B?TlRMV0MyVC9HWno2MGhWckNCQUFjYldWMWpaRWlBaEw0b0ZLYWl4cHRGVFl2?=
 =?utf-8?B?SDJHVFBMNys4YS80VE1XMjQ5R21kcmRCL1BGVHBzUTRjS2ZlNERlYnlvWDFt?=
 =?utf-8?B?aktnZ1ZaUG1INGNrbGdjcnlXeHhVcnEyRk9tL292TVB0QVRZOFZhWHdwR2J5?=
 =?utf-8?B?T1ZqVHdEeTN6YU55R0xCZjJsUUpqcEk5TkVNS3dUUHh2WmFDcm1ORUN6V1Ro?=
 =?utf-8?B?MGtqVWpjd3lQdU9CWGVTSEJQTHZmSHc2bUlaQzdjUjR0VnlyZnhuSkxhdGpt?=
 =?utf-8?B?YXBkYmQwNWxiUGpwTnNQazRRVS9ldXhRbDVyV2FvRE5XaVF1ZGFNRlM2UU52?=
 =?utf-8?B?U2pkTVBTVTFXQmVydi9HalhvVnAzaWxWOCtHQlArU2lpZFU3T2IzQW5LU1lv?=
 =?utf-8?B?RklnMVAranBrelBCZ1NuYit3cWtDY3RXTk83bUtDTUhZT3pUZC9iYkkyNHk2?=
 =?utf-8?B?WTJOcm91azl6ZS9nYXZidTRvMVNBbVpJcmZZMU8zT0VOaUllK0FkSFhvQ3FW?=
 =?utf-8?B?QnNWUFJtelJ5OXU3M2dPZ2JoZ1hhOHAxaXlkMzQ2aXU5MFVCL2JIdHNHQjRW?=
 =?utf-8?Q?XtvvIQOi/MJu0zAo=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1d616e19-6e57-4414-cef9-08da13563836
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2022 20:37:05.9452
 (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: BQfXo6oDqqY+6oASUXPMG6yyxEIjmCucrkRGiVqZ9x5AYSPc7rCWgLG/dSGSX3jeOTtk0D6yseEiNrU66sxZUdZlZnftRBfqB8a5vcw+Gu0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1987
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.850
 definitions=2022-03-31_06:2022-03-30,2022-03-31 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=816
 phishscore=0 suspectscore=0 malwarescore=0 adultscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2203310109
X-Proofpoint-ORIG-GUID: NXueq_5pyf7j__4u_Tsxkq3GlbdwP1Sx
X-Proofpoint-GUID: NXueq_5pyf7j__4u_Tsxkq3GlbdwP1Sx
X-Spam-Status: No, score=-2.8 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

On 3/31/22 12:45, Al Viro wrote:
> On Thu, Mar 31, 2022 at 12:08:27PM -0700, Stephen Brennan wrote:
>> Negative dentry bloat is a well-known problem. For systems without
>> memory pressure, some workloads (like repeated stat calls) can create an
>> unbounded amount of negative dentries quite quickly. In the best case,
>> these dentries could speed up a subsequent name lookup, but in the worst
>> case, they are never used and their memory never freed.
>>
>> While systems without memory pressure may not need that memory for other
>> purposes, negative dentry bloat can have other side-effects, such as
>> soft lockups when traversing the d_subdirs list or general slowness with
>> managing them. It is a good idea to have some sort of mechanism for
>> controlling negative dentries, even outside memory pressure.
>>
>> This patch attempts to do so in a fair way. Workloads which create many
>> negative dentries must create many dentries, or convert dentries from
>> positive to negative. Thus, negative dentry management is best done
>> during these same operations, as it will amortize its cost, and
>> distribute the cost to the perpetrators of the dentry bloat. We
>> introduce a sysctl "negative-dentry-ratio" which sets a maximum number
>> of negative dentries per positive dentry, N:1. When a dentry is created
>> or unlinked, the next N+1 dentries of the parent are scanned. If no
>> positive dentries are found, then a candidate negative dentry is killed.
> 
> Er...  So what's to stop d_move() from leaving you with your cursor
> pointer poiting into the list of children of another parent?
> 
> What's more, your dentry_unlist() logics will be defeated by that -
> if victim used to have a different parent, got moved, then evicted,
> it looks like you could end up with old parent cursor pointing
> to the victim and left unmodified by dentry_unlist() (since it looks
> only at the current parent's cursor).  Wait for it to be freed and
> voila - access to old parent's cursor will do unpleasant things.
> 
> What am I missing here?

Thanks for this catch. Since d_move holds the parent's lock, it should
be possible to include the same condition as dentry_unlist() to ensure
the cursor gets advanced if necessary. I could make it a small inline
helper to make things easier to read. I will go ahead and fix that.

Thanks,
Stephen