From patchwork Wed Nov 11 13:59:21 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Gruenbacher X-Patchwork-Id: 7595881 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A1DAC9F1C2 for ; Wed, 11 Nov 2015 13:59:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9982220642 for ; Wed, 11 Nov 2015 13:59:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8028F20523 for ; Wed, 11 Nov 2015 13:59:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752291AbbKKN7e (ORCPT ); Wed, 11 Nov 2015 08:59:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34988 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750869AbbKKN7b (ORCPT ); Wed, 11 Nov 2015 08:59:31 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id C864719D054; Wed, 11 Nov 2015 13:59:30 +0000 (UTC) Received: from nux.redhat.com (vpn1-6-232.ams2.redhat.com [10.36.6.232]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tABDxMYC020889; Wed, 11 Nov 2015 08:59:23 -0500 From: Andreas Gruenbacher To: Christoph Hellwig Cc: Andreas Gruenbacher , Alexander Viro , "Theodore Ts'o" , Andreas Dilger , "J. Bruce Fields" , Jeff Layton , Trond Myklebust , Anna Schumaker , Dave Chinner , linux-ext4 , XFS Developers , LKML , linux-fsdevel , Linux NFS Mailing List , linux-cifs@vger.kernel.org, Linux API Subject: Re: [PATCH v15 00/22] Richacls (Core and Ext4) Date: Wed, 11 Nov 2015 14:59:21 +0100 Message-Id: <1447250361-5561-1-git-send-email-agruenba@redhat.com> References: <1447067343-31479-1-git-send-email-agruenba@redhat.com> <20151110112943.GA17038@infradead.org> <20151111075707.GA23752@infradead.org> In-Reply-To: <20151111075707.GA23752@infradead.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Nov 11, 2015 at 8:57 AM, Christoph Hellwig wrote: > On Tue, Nov 10, 2015 at 01:39:52PM +0100, Andreas Gruenbacher wrote: >> > It still has the same crappy fs interfaces with lots of boilerplate >> > code >> >> Could you please be more specific so that I can trace this complaint >> to some actual code? > > if (IS_RICHACL()) > richacl_foo() > else > posix_acl_foo() > > for every call from the filesystem is the major one that came to mind. There are two such places in ext4, ext4_new_acl and ext4_acl_chmod. Those could be replaced by function pointers as below, I'm not sure we seriously want to consider that. In xfs, we have xfs_acl_chmod which is similar. xfs_generic_create doesn't quite follow this pattern. This seems to be about it though. Thanks, Andreas --- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/ialloc.c | 11 +---------- fs/ext4/inode.c | 11 +---------- fs/ext4/super.c | 30 ++++++++++++++++++++++++++++++ include/linux/fs.h | 1 + 5 files changed, 43 insertions(+), 20 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index b97a3b1..5ff4556 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -3049,6 +3049,16 @@ extern struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ]; extern int ext4_resize_begin(struct super_block *sb); extern void ext4_resize_end(struct super_block *sb); +struct ext4_acl_ops { + int (*chmod)(struct inode *, umode_t); + int (*init_acl)(handle_t *, struct inode *, struct inode *); +}; + +static inline const struct ext4_acl_ops *ACL_OPS(struct inode *inode) +{ + return inode->i_sb->s_private; +} + #endif /* __KERNEL__ */ #endif /* _EXT4_H */ diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index 9657b3a..e33646f 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -27,7 +27,6 @@ #include "ext4_jbd2.h" #include "xattr.h" #include "acl.h" -#include "richacl.h" #include @@ -698,14 +697,6 @@ out: return ret; } -static inline int -ext4_new_acl(handle_t *handle, struct inode *inode, struct inode *dir) -{ - if (IS_RICHACL(dir)) - return ext4_init_richacl(handle, inode, dir); - return ext4_init_acl(handle, inode, dir); -} - /* * There are two policies for allocating an inode. If the new inode is * a directory, then a forward search is made for a block group with both @@ -1061,7 +1052,7 @@ got: if (err) goto fail_drop; - err = ext4_new_acl(handle, inode, dir); + err = ACL_OPS(inode)->init_acl(handle, inode, dir); if (err) goto fail_free_drop; diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 647f3c3..9f179ee 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -42,7 +42,6 @@ #include "xattr.h" #include "acl.h" #include "truncate.h" -#include "richacl.h" #include @@ -4639,14 +4638,6 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode) } } -static inline int -ext4_acl_chmod(struct inode *inode, umode_t mode) -{ - if (IS_RICHACL(inode)) - return richacl_chmod(inode, inode->i_mode); - return posix_acl_chmod(inode, inode->i_mode); -} - /* * ext4_setattr() * @@ -4815,7 +4806,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr) ext4_orphan_del(NULL, inode); if (!rc && (ia_valid & ATTR_MODE)) - rc = ext4_acl_chmod(inode, inode->i_mode); + rc = ACL_OPS(inode)->chmod(inode, inode->i_mode); err_out: ext4_std_error(inode->i_sb, error); if (!error) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 7457ea8..879bc2c 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -49,6 +49,7 @@ #include "ext4_jbd2.h" #include "xattr.h" #include "acl.h" +#include "richacl.h" #include "mballoc.h" #define CREATE_TRACE_POINTS @@ -1270,25 +1271,54 @@ static ext4_fsblk_t get_sb_block(void **data) return sb_block; } +static int no_chmod_acl(struct inode *inode, umode_t mode) +{ + return 0; +} + +static int no_init_acl(handle_t *handle, struct inode *inode, struct inode *dir) +{ + return 0; +} + +struct ext4_acl_ops no_acl_ops = { + .chmod = no_chmod_acl, + .init_acl = no_init_acl, +}; + +struct ext4_acl_ops ext4_posix_acl_ops = { + .chmod = posix_acl_chmod, + .init_acl = ext4_init_acl, +}; + +struct ext4_acl_ops ext4_richacl_ops = { + .chmod = richacl_chmod, + .init_acl = ext4_init_richacl, +}; + static int enable_acl(struct super_block *sb) { sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL); + sb->s_private = &no_acl_ops; if (test_opt(sb, ACL)) { if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RICHACL)) { #ifdef CONFIG_EXT4_FS_RICHACL sb->s_flags |= MS_RICHACL; + sb->s_private = &ext4_richacl_ops; #else return -EOPNOTSUPP; #endif } else { #ifdef CONFIG_EXT4_FS_POSIX_ACL sb->s_flags |= MS_POSIXACL; + sb->s_private = &ext4_posix_acl_ops; #else return -EOPNOTSUPP; #endif } } + return 0; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 05fb943..5803bf6 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1324,6 +1324,7 @@ struct super_block { void *s_security; #endif const struct xattr_handler **s_xattr; + const void *s_private; struct hlist_bl_head s_anon; /* anonymous dentries for (nfs) exporting */ struct list_head s_mounts; /* list of mounts; _not_ for fs use */