From patchwork Wed Jan 29 19:09:18 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 3554091 Return-Path: X-Original-To: patchwork-ceph-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id E0BFAC02DC for ; Wed, 29 Jan 2014 19:09:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 21CCB2018B for ; Wed, 29 Jan 2014 19:09:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2C0262017D for ; Wed, 29 Jan 2014 19:09:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752954AbaA2TJY (ORCPT ); Wed, 29 Jan 2014 14:09:24 -0500 Received: from mail-vb0-f53.google.com ([209.85.212.53]:39196 "EHLO mail-vb0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752064AbaA2TJV (ORCPT ); Wed, 29 Jan 2014 14:09:21 -0500 Received: by mail-vb0-f53.google.com with SMTP id p17so1449434vbe.12 for ; Wed, 29 Jan 2014 11:09:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=bJQ3w5cdbTbMXiZ+j8jCV9HGIUSrmJPIaEPA85y90HA=; b=BQWS+BxLyCNm63TyVxb8nGV2ndPlMYEo/a2yfWZojT71uVDl/R/SbsdcMXYulsZNvf G2S55kUU7DGzD0DHc+oPArWa4U6NiCvHjbSbABQp1OV8hK7Hbswf6xgGJMc1Gh821hxE Ov5mmBB4guB1huN+A44d3q+XzqqIfQu95VaHmKmYE5TFtkLxuVTN82+pEtSnhMOp2U7f RvIO0mRJRH2QTrf7N0TGoQvoRECCB7Dw5WpvK+3dsa6iGGVuWf05mVpD5GIQHLSsW2sY g9CYJUDwFyTBoDq77vWaKz3KQESXyuXc4O2pRBH8JmpqiKOl8n8TA4qyVbDmAbJpo1MP Zl7w== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=bJQ3w5cdbTbMXiZ+j8jCV9HGIUSrmJPIaEPA85y90HA=; b=LXVYBNQ7140mvL2sntlQ1YYUXgevJpK6rrROXaYl7sePOIy1oZ7tT3bDMp9jq/YSic SbIevC+Wc0STcj/M8LUygsnz1wv2su+M+d5DInDZQY4CrZ8e+0djbaHqpNfZXZCVeBil 5k9p6riktI5WZyUH7f2Q8QU2WQxZNTlZBazO8= MIME-Version: 1.0 X-Received: by 10.220.164.80 with SMTP id d16mr7791486vcy.15.1391022558757; Wed, 29 Jan 2014 11:09:18 -0800 (PST) Received: by 10.221.8.73 with HTTP; Wed, 29 Jan 2014 11:09:18 -0800 (PST) In-Reply-To: <1391013467-7598-1-git-send-email-ilya.dryomov@inktank.com> References: <1391013467-7598-1-git-send-email-ilya.dryomov@inktank.com> Date: Wed, 29 Jan 2014 11:09:18 -0800 X-Google-Sender-Auth: wt6UdAG_aPgYQbUbjg49lSQ5MH8 Message-ID: Subject: Re: [PATCH v2] ceph: fix posix ACL hooks From: Linus Torvalds To: Ilya Dryomov Cc: Sage Weil , Dave Jones , Linux Kernel Mailing List , ceph-devel@vger.kernel.org, linux-fsdevel , Christoph Hellwig , Al Viro , Guangliang Zhao , Li Wang , zheng.z.yan@intel.com Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,T_TVD_MIME_EPI, 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, Jan 29, 2014 at 8:37 AM, Ilya Dryomov wrote: > From: Sage Weil > > The merge of 7221fe4c2 raced with upstream changes in the generic POSIX > ACL code (2aeccbe95). Update Ceph to use the new helpers as well by > dropping the now-generic functions and setting the set_acl inode op. > > Signed-off-by: Sage Weil Ok, I already committed my untested (and broken) patch yesterday, because even a broken tree is better than a non-*compiling* tree for testing. I created a diff from my current tree to this, and will happily apply it, but the sign-off chain is now broken (Ilya didn't sign off on his changes to Sage's patch. Not that it really matters for what is really a one-liner change, but I thought I'd mention it, since another issue came up with this patch.. Ceph now does struct dentry *dentry = d_find_alias(inode); in its ceph_set_acl() function, and that's because the VFS layer doesn't pass down the dentry to the acl code any more. Christoph - that sounds like a misfeature in the new interfaces. I realize that for traditional unix filesystems, the path is irrelevant for an inode operation, but the thing is, from a Linux VFS standpoint and a conceptual standpoint, the dentry really is the more important and unique one, and some filesystems want the dentry because they are *correctly* designed to be about pathnames. Network filesystems that are pass file descriptors around (ie NFS etc) are not the "RightWay(tm)" of doing things, so I think that it is quite reasonable for ceph to want the *path* to the inode and not just the inode. So attached is the incremental diff of the patch by Sage and Ilya, and I'll apply it (delayed a bit to see if I can get the sign-off from Ilya), but I also think we should fix the (non-cached) ACL functions that call down to the filesystem layer to also get the dentry. We already do that for the xattr functions, just not for get_acl()/set_acl()/acl_chmod(). Christoph, Al? Yes, most filesystems will ignore the dentry pointer, but still.. Linus fs/ceph/acl.c | 9 ++++----- fs/ceph/dir.c | 1 + fs/ceph/inode.c | 2 ++ fs/ceph/super.h | 3 +++ 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c index f6911284c9bd..66d377a12f7c 100644 --- a/fs/ceph/acl.c +++ b/fs/ceph/acl.c @@ -107,14 +107,14 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) return acl; } -static int ceph_set_acl(struct dentry *dentry, struct inode *inode, - struct posix_acl *acl, int type) +int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) { int ret = 0, size = 0; const char *name = NULL; char *value = NULL; struct iattr newattrs; umode_t new_mode = inode->i_mode, old_mode = inode->i_mode; + struct dentry *dentry = d_find_alias(inode); if (acl) { ret = posix_acl_valid(acl); @@ -208,8 +208,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir) if (IS_POSIXACL(dir) && acl) { if (S_ISDIR(inode->i_mode)) { - ret = ceph_set_acl(dentry, inode, acl, - ACL_TYPE_DEFAULT); + ret = ceph_set_acl(inode, acl, ACL_TYPE_DEFAULT); if (ret) goto out_release; } @@ -217,7 +216,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir) if (ret < 0) goto out; else if (ret > 0) - ret = ceph_set_acl(dentry, inode, acl, ACL_TYPE_ACCESS); + ret = ceph_set_acl(inode, acl, ACL_TYPE_ACCESS); else cache_no_acl(inode); } else { diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 619616d585b0..6da4df84ba30 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -1303,6 +1303,7 @@ const struct inode_operations ceph_dir_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, .mknod = ceph_mknod, .symlink = ceph_symlink, .mkdir = ceph_mkdir, diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 8b8b506636cc..32d519d8a2e2 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -97,6 +97,7 @@ const struct inode_operations ceph_file_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, }; @@ -1616,6 +1617,7 @@ static const struct inode_operations ceph_symlink_iops = { .listxattr = ceph_listxattr, .removexattr = ceph_removexattr, .get_acl = ceph_get_acl, + .set_acl = ceph_set_acl, }; /* diff --git a/fs/ceph/super.h b/fs/ceph/super.h index 345933948b6e..aa260590f615 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -718,6 +718,7 @@ extern void ceph_queue_writeback(struct inode *inode); extern int ceph_do_getattr(struct inode *inode, int mask); extern int ceph_permission(struct inode *inode, int mask); extern int ceph_setattr(struct dentry *dentry, struct iattr *attr); +extern int ceph_setattr(struct dentry *dentry, struct iattr *attr); extern int ceph_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat); @@ -741,12 +742,14 @@ extern const struct xattr_handler *ceph_xattr_handlers[]; #ifdef CONFIG_CEPH_FS_POSIX_ACL struct posix_acl *ceph_get_acl(struct inode *, int); +int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type); int ceph_init_acl(struct dentry *, struct inode *, struct inode *); void ceph_forget_all_cached_acls(struct inode *inode); #else #define ceph_get_acl NULL +#define ceph_set_acl NULL static inline int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir)