From patchwork Tue Feb 27 02:53:02 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Eric W. Biederman" X-Patchwork-Id: 10244155 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4E11260384 for ; Tue, 27 Feb 2018 02:53:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D2B42A4D9 for ; Tue, 27 Feb 2018 02:53:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2F3422A4FA; Tue, 27 Feb 2018 02:53:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 867592A4D9 for ; Tue, 27 Feb 2018 02:53:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751742AbeB0Cxi (ORCPT ); Mon, 26 Feb 2018 21:53:38 -0500 Received: from out03.mta.xmission.com ([166.70.13.233]:54175 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751728AbeB0Cxh (ORCPT ); Mon, 26 Feb 2018 21:53:37 -0500 Received: from in01.mta.xmission.com ([166.70.13.51]) by out03.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1eqVOZ-0003SG-ND; Mon, 26 Feb 2018 19:53:35 -0700 Received: from 174-19-85-160.omah.qwest.net ([174.19.85.160] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1eqVOY-0006c3-F3; Mon, 26 Feb 2018 19:53:35 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: Miklos Szeredi , Linux Kernel Mailing List , Linux Containers , linux-fsdevel , Alban Crequy , Seth Forshee , Sargun Dhillon , Dongsu Park , "Serge E. Hallyn" References: <87po4rz4ui.fsf_-_@xmission.com> <20180226235302.12708-3-ebiederm@xmission.com> Date: Mon, 26 Feb 2018 20:53:02 -0600 In-Reply-To: (Linus Torvalds's message of "Mon, 26 Feb 2018 17:13:59 -0800") Message-ID: <87r2p7rvn5.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1eqVOY-0006c3-F3; ; ; mid=<87r2p7rvn5.fsf@xmission.com>; ; ; hst=in01.mta.xmission.com; ; ; ip=174.19.85.160; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX18xjl9mUmS2YBJNfkIp1LsqjfNYoTV8o9I= X-SA-Exim-Connect-IP: 174.19.85.160 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP So the purpose for having a patch in the first place is that 2a3a2a3f3524 ("ovl: don't cache acl on overlay layer") which addded ACL_DONT_CACHED did not result in any comment updates to get_acl. Which mean that if you read the comments in get_acl() that you don't even think of ACL_DONT_CACHED. Which means that this comment: /* * If the ACL isn't being read yet, set our sentinel. Otherwise, the * current value of the ACL will not be ACL_NOT_CACHED and so our own * sentinel will not be set; another task will update the cache. We * could wait for that other task to complete its job, but it's easier * to just call ->get_acl to fetch the ACL ourself. (This is going to * be an unlikely race.) */ Which presumes the only reason the acl could be anything other ACL_NOT_CACHED is because get_acl() is already being called upon it in another task. I wanted something to mention ACL_DONT_CACHED so someone would at least think about that case if they ever step up to modify the code. The code is perfectly clear, the comment is not. That scares me. And I had to read the code about a dozen times before I realized the ACL_DONT_CACHED case even exists. Not useful when I am need to use that to preserve historical fuse semantics. So something is missing here even if my wording does not improve things. Then we get this comment: /* * Normally, the ACL returned by ->get_acl will be cached. * A filesystem can prevent that by calling * forget_cached_acl(inode, type) in ->get_acl. */ Which was added in b8a7a3a66747 ("posix_acl: Inode acl caching fixes") That comment is and always has been rubbish. I don't have a clue what it is trying to say but it is not something a person can use to write filesystem code with. Truths: - forget_cached_acl(inode, type) can be used to invalidate the acl cache. - Calling forget_cached_acl from within the filesystems ->get_acl method won't prevent a cached value from being returend because ->get_acl will be set. - Calling forget_cached_acl from within the filesystems ->get_acl method won't prevent a returned value from being cached because it the caching happens after ->get_acl returns. - Setting inode->i_acl = ACL_DONT_CACHE is the only way to prevent a value from ->get_acl from being cached. In summary I only care about two things. 1) ACL_NOT_CACHED being mentioned somewhere in get_acl so people looking at the code, and people updating the code will have a hint that they need to consider that case. 2) That misleading completely bogus comment being removed/fixed. And yes I agree the code is clear. The comments are not. Does this look better as a comment updating patch? Eric diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 2fd0fde16fe1..5453094b8828 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -98,6 +98,11 @@ struct posix_acl *get_acl(struct inode *inode, int type) struct posix_acl **p; struct posix_acl *acl; + /* + * To avoid caching the result of ->get_acl + * set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE; + */ + /* * The sentinel is used to detect when another operation like * set_cached_acl() or forget_cached_acl() races with get_acl(). @@ -126,9 +131,7 @@ struct posix_acl *get_acl(struct inode *inode, int type) /* fall through */ ; /* - * Normally, the ACL returned by ->get_acl will be cached. - * A filesystem can prevent that by calling - * forget_cached_acl(inode, type) in ->get_acl. + * The ACL returned by ->get_acl will be cached. * * If the filesystem doesn't have a get_acl() function at all, we'll * just create the negative cache entry.