From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754908AbcBWTCI (ORCPT ); Tue, 23 Feb 2016 14:02:08 -0500 Received: from mail-by2on0115.outbound.protection.outlook.com ([207.46.100.115]:21343 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752204AbcBWTCG (ORCPT ); Tue, 23 Feb 2016 14:02:06 -0500 Authentication-Results: suse.cz; dkim=none (message not signed) header.d=none;suse.cz; dmarc=none action=none header.from=hpe.com; Message-ID: <56CCACA3.1000905@hpe.com> Date: Tue, 23 Feb 2016 14:01:55 -0500 From: Waiman Long User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12 MIME-Version: 1.0 To: Jan Kara CC: Peter Zijlstra , Dave Chinner , Alexander Viro , Jan Kara , Jeff Layton , "J. Bruce Fields" , Tejun Heo , Christoph Lameter , , , Ingo Molnar , Andi Kleen , Dave Chinner , Scott J Norton , Douglas Hatch Subject: Re: [PATCH v2 3/3] vfs: Use per-cpu list for superblock's inode list References: <1455916245-32707-1-git-send-email-Waiman.Long@hpe.com> <1455916245-32707-4-git-send-email-Waiman.Long@hpe.com> <20160221213419.GB25832@dastard> <20160222091844.GZ6357@twins.programming.kicks-ass.net> <20160222115435.GI7791@quack.suse.cz> <20160222121222.GF6357@twins.programming.kicks-ass.net> <20160222130435.GM7791@quack.suse.cz> In-Reply-To: <20160222130435.GM7791@quack.suse.cz> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [71.168.64.29] X-ClientProxiedBy: BLUPR01CA039.prod.exchangelabs.com (25.160.23.29) To DF4PR84MB0315.NAMPRD84.PROD.OUTLOOK.COM (25.162.193.29) X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0315;2:HQtHNE4CDaqflZ7uE75HYSJnLG61zGj+wFD09r60aC7Vpv1GglcObuUtzTiDvM7RmjNasRNOaKu5CwNsEjp/XIuEL+kxjmb+a1EiAVN5GQsJbfh9j9JbCU+Mdyhs0/YWTPzsKAUBWV2BXxvEj7UvzA==;3:gjqKxamfaYHvIghVujOAgrn56D2JhTMa9ZJqXGZGehucX3dtbi62v3QzH0459ZTV+UVLbZ6d7DRxJyS/FaOIZt1H4xSF3u9UA7rhlDxDB0/CDb6pTHhi0fEBEZk1Degc;25:/nEBcgZSgpT/O3mo4cCV8TrndZJXmInaCkshk69vWDycmLkENg/tBbAiRyIq05dyHjUxm5MdNbz6NrvSpcl/aemvuA8SaW+VHWNkq+3gMRRr/q6x7ibK6GroIpucgrrru+wKW4MeUE2tpkzo8Qe6opGrQblaCnpnCcdRzDTBy3cCLSHlb0p9ertIGE+eUzagF4jCoiaQqn+7uj/3U9u11+Bv8sQVTO5RTOntxQaxQke45BuX0jyQUBz3UPp35YFobVQxquweMQZCAWUopvDbzPU6ELBlUa4kTUu4Mktt88VzaovItGe+uFvcxUdgQyTcQNDsv0EcBMTN9h+oLmUJNx2NY62MwRmbumhyIp6NtpI=;20:lRynbj8PC/gYn/tRlFvwc8WeFTTN/2xgjbTv3VjI1PkceN2FEBd43W+bysCQqSsuuEWa1WHnv7no5IRokUVMeZ1vQkUF1+FVux0tefAUQCOgcKjASYnKhRFCR0bqZA5n7tZDQthgWx7w4msCmHZlil06OSHq2yHdQw3bCrkQB6r6YTDS3bHwh/SPDPuUvRufkawjX14qFNT6rnOFc4Ya25XxSyMtwWkDqhibvBiTc776DGvy69VOY23obVn54kpP X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0315; X-MS-Office365-Filtering-Correlation-Id: 6f6751d3-c5fa-4433-feb0-08d33c83d096 X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001);SRVR:DF4PR84MB0315;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0315; X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0315;4:6oZbsmr2twKGcpoFaOv+Cv9orBXwr2D7P2Z7JsDrXvP91QT+G7CAu0yDe8aQzvKUpQN4T6EP9Kq1yzKXbkQmPTUZCpm8y/zNfy9nMO2KODGSmxMtWQASfLrdnbew09yWq42XbFz6apFdmsYvzvt8UbU+m+IEivF6moKaSb9oHib4dFgYalhci+OV0Evj9DZ7lRIuGuBmuemFFJx5ydcT8+jWVrjL1hWloUW+/3pczy0SiFMYL6C/dQK+aGyeLLE8IJ4aXx4iLK5Fbq9DMOBr+iK7HNQ3OJeuiN0COb3ESz+SyIPQAQVjvSTgfE7BM61COQehKTFW/CyId/vtVToyyeeOQ9NdWgiUDNukcl73OHH5FLLUaFMqerH9VD3LJk9u X-Forefront-PRVS: 08617F610C X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(24454002)(377454003)(377424004)(479174004)(23756003)(92566002)(83506001)(6116002)(50466002)(77096005)(40100003)(80316001)(5890100001)(64126003)(2950100001)(4001350100001)(33656002)(54356999)(87266999)(50986999)(65816999)(76176999)(189998001)(5001960100002)(87976001)(42186005)(3846002)(230700001)(4326007)(1096002)(47776003)(2906002)(5008740100001)(5004730100002)(586003)(117156001)(93886004)(36756003)(66066001)(65956001)(86362001)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:DF4PR84MB0315;H:[192.168.142.191];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;DF4PR84MB0315;23:SJlVmzyZSdm2hXmXvL5GXAbIAd7b6DDjHmQXK77?= =?iso-8859-1?Q?NdKKuBg9QD8EEmluVDzB6+TuQDf8VBtDp9RPl5kbG3TAbTDGKdECE9n82J?= =?iso-8859-1?Q?6yofTyD6SUsZcUZkHLaAIYYASQ/CNZjohw3lRElEoOo5qj3LVFRz6jDwH3?= =?iso-8859-1?Q?ENrn+8gnl12xaRsP2ZEfUossL5oCSE4yjvdF7hN2kkWyaUDrfZK0n8EHRQ?= =?iso-8859-1?Q?v4R+VVJJjmEK1iOK0dQh17FIt1UK/v52KTrROtWl8oH04HPegrvQ0BQ2p8?= =?iso-8859-1?Q?mMWh2qM+pFmYub4Yu58Kr2ZxQTnxL4WafOnWMwBRPjfKun1VuggOm/kVp4?= =?iso-8859-1?Q?PgyDC4VQtE3AETj/yJzqEGsFjGzwG0ytchwQkkcrGEzL4IXezjOHEpRknp?= =?iso-8859-1?Q?Re/3UlqlHhF2vdbYL9n4CtK6qdyAoxiMtL0gW2JenO1NWbuud8HEkFQV7z?= =?iso-8859-1?Q?tnUhnGbFRW1pH3rNYvK5U+doys5NhsvgCSraqhgpnXW0TgPWmHCXKa5xc9?= =?iso-8859-1?Q?hIHPpbgZYJSVDNBbocnJEhrF6XO6PjZS/xaM980BeE6zkD6Ke/oGRJs2l2?= =?iso-8859-1?Q?NWlA0L2I3tQflyyQw9IwdBZdWIKUvpICWnRfnBIORCoetF2x1Hs1CGnwcr?= =?iso-8859-1?Q?qdgONbZc9N4b5ClZnPAVySYO7e/fgJbfJYx+kmAYCH4l39tAv2i7hLPhHb?= =?iso-8859-1?Q?HDMcml+xP+BuCg5TzB4ctFD9fliwvVz5KfZOYoCeFSXu4cfX/QTfubY9+m?= =?iso-8859-1?Q?Nx4o2bDvSL6v7krrDuvSrBw397/payU0tICS+82RTzIv3+cKxakdzkWn9Q?= =?iso-8859-1?Q?lqyZi99WDSbMyK0ME+K9/ZCYfVBV6ZCyERo22N634wccw82+PvIjydljQ7?= =?iso-8859-1?Q?A/pVziCkQ4lLQo+KALY1GnvH7UVGyUYg6WbkH/YEuu5pwTJnuHNEl2HhB9?= =?iso-8859-1?Q?uXV51pWaTsjMD4X6PgRK4E8/mcwmrYdbaHONDN521q0SMA4opmJmmfYUe+?= =?iso-8859-1?Q?LeHaRlvWRK7jaJEgIuM1aqIYn4k0j0+TOyT0GPlSiBaxbkplrf4V4r+SYL?= =?iso-8859-1?Q?91S6OMso1EQKvompuKI3QaMRBtPSv12WdSkcL8Q5BwTaQq11xbVOc+DX0b?= =?iso-8859-1?Q?AGsRT4Q862B6REFAPTW0TyAnka8ApuJFfdt1c09loFAwvvByy564s/7z0d?= =?iso-8859-1?Q?Gv3x4nLquH3T5DLug0CToHm/sKLFXGDDSour3420UdSPVMTK8//Psw=3D?= X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0315;5:/BGTB+e3QD+HnwCdNGRiipfYqPKcFK3qktj02mu1KwnHoyXlmw4eGZBBZsJcWshNgKS3TPBTauys46ViBe50IykdtMG9EYBFBn0PlkP7vm6Q87W+WMPuOUrqYYO3ntFLT2Id0KJM2J6WI0Ye/IRWeg==;24:blx7ueza6KaSYPUbQSec0wP+bcolcHgq/GRr6G/9mK7wwOI1D3uW1duBH3V0isjp/LjMvKls4QoX+sp99oFyQjUBntINlQTsAV8dtpjUxwA= SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2016 19:02:01.4653 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0315 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/22/2016 08:04 AM, Jan Kara wrote: > On Mon 22-02-16 13:12:22, Peter Zijlstra wrote: >> On Mon, Feb 22, 2016 at 12:54:35PM +0100, Jan Kara wrote: >>>> Also, I think fsnotify_unmount_inodes() (as per mainline) is missing a >>>> final iput(need_iput) at the very end, but I could be mistaken, that >>>> code hurts my brain. >>> I think the code is actually correct since need_iput contains "inode >>> further in the list than the current inode". Thus we will always go though >>> another iteration of the loop which will drop the reference. And inode >>> cannot change state to I_FREEING or I_WILL_FREE because we hold inode >>> reference. But it is subtle as hell so I agree that code needs rewrite. >> So while talking to dchinner, he doubted fsnotify will actually remove >> inodes from the sb-list, but wasn't sure and too tired to check now. >> >> (I got lost in the fsnotify code real quick and gave up, for I was >> mostly trying to make a point that we don't need the CPP magic and can >> do with 'readable' code). >> >> If it doesn't, it doesn't need to do this extra special magic dance and >> can use the 'normal' iterator pattern used in all the other functions, >> greatly reducing complexity. > Yeah, that would be nice. But fsnotify code needs to iterate over all > inodes, drop sb_list_lock and do some fsnotify magic with the inode which > is not substantial for our discussion. Now that fsnotify magic may actually > drop all the remaining inode references so once we drop our reference > pinning the inode, it can just disappear. We don't want to restart the scan > for each inode we have to process so that is the reason why we play ugly > tricks with pinning the next inode in the list. > > But I agree it should be possible to just use list_for_each_entry() instead > of list_for_each_entry_safe() and keep current inode pinned till the next > iteration to make it stick in the sb->s_inodes list. That would make the > iteration more standard. Lightly tested patch attached. > > Honza Your patch looks good to me. I would like to put your patch into my per-cpu list patchset if you don't mind. Cheers, Longman