Date: Mon, 25 Aug 2008 19:16:54 GMT From: Edward Tomasz Napierala <trasz@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 148425 for review Message-ID: <200808251916.m7PJGsHT076838@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=148425 Change 148425 by trasz@trasz_traszkan on 2008/08/25 19:16:10 Fix a performance problem, visible e.g. with untarring the ports tree. Change the return type of acl_nfs4_sync_acl_from_mode and acl_nfs4_compute_inherited_acl to void, as they cannot return an error. Affected files ... .. //depot/projects/soc2008/trasz_nfs4acl/sys/kern/subr_acl_nfs4.c#28 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/sys/acl.h#24 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/ufs/ufs/ufs_acl.c#12 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/ufs/ufs/ufs_vnops.c#16 edit Differences ... ==== //depot/projects/soc2008/trasz_nfs4acl/sys/kern/subr_acl_nfs4.c#28 (text+ko) ==== @@ -324,7 +324,7 @@ return (&(aclp->acl_entry[entry_index + 1])); } -int +void acl_nfs4_sync_acl_from_mode(struct acl *aclp, mode_t mode, int file_owner_id) { int i, meets, must_append; @@ -335,6 +335,10 @@ const int WRITE = 02; const int EXEC = 01; + KASSERT(aclp->acl_cnt >= 0, ("aclp->acl_cnt >= 0")); + KASSERT(aclp->acl_cnt <= ACL_MAX_ENTRIES, + ("aclp->acl_cnt <= ACL_MAX_ENTRIES")); + /* * NFSv4 Minor Version 1, draft-ietf-nfsv4-minorversion1-03.txt * @@ -600,8 +604,8 @@ } if (must_append) { - if (aclp->acl_cnt + 6 >= ACL_MAX_ENTRIES) - return (EPERM); + KASSERT(aclp->acl_cnt + 6 <= ACL_MAX_ENTRIES, + ("aclp->acl_cnt <= ACL_MAX_ENTRIES")); a1 = _acl_append(aclp, ACL_USER_OBJ, 0, ACL_EXTENDED_DENY); a2 = _acl_append(aclp, ACL_USER_OBJ, ACL_WRITE_ACL | @@ -661,10 +665,6 @@ a6->ae_perm |= ACL_EXECUTE; else a5->ae_perm |= ACL_EXECUTE; - - KASSERT(aclp->acl_cnt >= 6, ("acl->acl_cnt >= 6")); - - return (0); } void @@ -674,6 +674,10 @@ mode_t old_mode = *_mode, mode = 0, seen = 0; const struct acl_entry *entry; + KASSERT(aclp->acl_cnt > 0, ("aclp->acl_cnt > 0")); + KASSERT(aclp->acl_cnt <= ACL_MAX_ENTRIES, + ("aclp->acl_cnt <= ACL_MAX_ENTRIES")); + /* * NFSv4 Minor Version 1, draft-ietf-nfsv4-minorversion1-03.txt * @@ -800,16 +804,19 @@ *_mode = mode | (old_mode & ACL_PRESERVE_MASK); } -int +void acl_nfs4_compute_inherited_acl(const struct acl *parent_aclp, struct acl *child_aclp, mode_t mode, int file_owner_id, int is_directory) { - int i, error, flags; + int i, flags; const struct acl_entry *parent_entry; struct acl_entry *entry, *copy; KASSERT(child_aclp->acl_cnt == 0, ("child_aclp->acl_cnt == 0")); + KASSERT(parent_aclp->acl_cnt > 0, ("parent_aclp->acl_cnt > 0")); + KASSERT(parent_aclp->acl_cnt <= ACL_MAX_ENTRIES, + ("parent_aclp->acl_cnt <= ACL_MAX_ENTRIES")); /* * NFSv4 Minor Version 1, draft-ietf-nfsv4-minorversion1-03.txt @@ -955,9 +962,31 @@ * in Section 2.16.6.3, using the mode that is to be used for file * creation. */ - error = acl_nfs4_sync_acl_from_mode(child_aclp, mode, file_owner_id); + acl_nfs4_sync_acl_from_mode(child_aclp, mode, file_owner_id); +} + +static int +_acls_are_equal(const struct acl *a, const struct acl *b) +{ + int i; + const struct acl_entry *entrya, *entryb; + + if (a->acl_cnt != b->acl_cnt) + return (0); + + for (i = 0; i < b->acl_cnt; i++) { + entrya = &(a->acl_entry[i]); + entryb = &(b->acl_entry[i]); + + if (entrya->ae_tag != entryb->ae_tag || + entrya->ae_id != entryb->ae_id || + entrya->ae_perm != entryb->ae_perm || + entrya->ae_extended != entryb->ae_extended || + entrya->ae_flags != entryb->ae_flags) + return (0); + } - return (error); + return (1); } /* @@ -965,9 +994,32 @@ * that stores ACL contents. */ int -acl_nfs4_is_trivial(const struct acl *aclp) +acl_nfs4_is_trivial(const struct acl *aclp, int file_owner_id) { - return (0); + int trivial; + mode_t tmpmode = 0; + struct acl *tmpaclp; + + if (aclp->acl_cnt != 6) + return (0); + + /* + * Compute the mode from the ACL, then compute new ACL from that mode. + * If the ACLs are identical, then the ACL is trivial. + * + * XXX: I guess there is a faster way to do this. However, even + * this slow implementation significantly speeds things up + * for files that don't have any extended ACL entries - it's + * critical for performance to not use EA when they are not + * needed. + */ + tmpaclp = acl_alloc(); + acl_nfs4_sync_mode_from_acl(&tmpmode, aclp); + acl_nfs4_sync_acl_from_mode(tmpaclp, tmpmode, file_owner_id); + trivial = _acls_are_equal(aclp, tmpaclp); + acl_free(tmpaclp); + + return (trivial); } int ==== //depot/projects/soc2008/trasz_nfs4acl/sys/sys/acl.h#24 (text+ko) ==== @@ -270,12 +270,13 @@ mode_t acl_posix1e_newfilemode(mode_t cmode, struct acl *dacl); -int acl_nfs4_sync_acl_from_mode(struct acl *aclp, +void acl_nfs4_sync_acl_from_mode(struct acl *aclp, mode_t mode, int file_owner_id); void acl_nfs4_sync_mode_from_acl(mode_t *mode, const struct acl *aclp); -int acl_nfs4_is_trivial(const struct acl *aclp); -int acl_nfs4_compute_inherited_acl( +int acl_nfs4_is_trivial(const struct acl *aclp, + int file_owner_id); +void acl_nfs4_compute_inherited_acl( const struct acl *parent_aclp, struct acl *child_aclp, mode_t mode, int file_owner_id, int is_directory); ==== //depot/projects/soc2008/trasz_nfs4acl/sys/ufs/ufs/ufs_acl.c#12 (text+ko) ==== @@ -163,10 +163,9 @@ * Legitimately no ACL set on object, purely * emulate it through the inode. */ - error = acl_nfs4_sync_acl_from_mode(ap->a_aclp, ip->i_mode, - ip->i_uid); + acl_nfs4_sync_acl_from_mode(ap->a_aclp, ip->i_mode, ip->i_uid); - return (error); + return (0); } if (error) @@ -404,16 +403,23 @@ return (EPERM); /* - * Must hold VADMIN (be file owner) or have appropriate privilege. + * Must hold VWRITE_ACL or have appropriate privilege. */ if ((error = VOP_ACCESS(ap->a_vp, VWRITE_ACL, ap->a_cred, ap->a_td))) return (error); - if (acl_nfs4_is_trivial(ap->a_aclp)) { + if (acl_nfs4_is_trivial(ap->a_aclp, ip->i_uid)) { error = vn_extattr_rm(ap->a_vp, IO_NODELOCKED, NFS4_ACL_EXTATTR_NAMESPACE, NFS4_ACL_EXTATTR_NAME, ap->a_td); + /* + * An attempt to remove ACL from a file that didn't have + * any extended entries is not an error. + */ + if (error == ENOATTR) + error = 0; + } else { ap->a_aclp->acl_magic = ACL_MAGIC; error = vn_extattr_set(ap->a_vp, IO_NODELOCKED, ==== //depot/projects/soc2008/trasz_nfs4acl/sys/ufs/ufs/ufs_vnops.c#16 (text+ko) ==== @@ -669,9 +669,7 @@ if (error) goto out; - error = acl_nfs4_sync_acl_from_mode(aclp, mode, file_owner_id); - if (error) - goto out; + acl_nfs4_sync_acl_from_mode(aclp, mode, file_owner_id); error = VOP_SETACL(vp, ACL_TYPE_NFS4, aclp, cred, td); @@ -1430,10 +1428,8 @@ if (error) goto out; - error = acl_nfs4_compute_inherited_acl(parent_aclp, child_aclp, + acl_nfs4_compute_inherited_acl(parent_aclp, child_aclp, child_mode, VTOI(tvp)->i_uid, tvp->v_type == VDIR); - if (error) - goto out; error = VOP_SETACL(tvp, ACL_TYPE_NFS4, child_aclp, cred, td); if (error)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200808251916.m7PJGsHT076838>