[LU-1747] osd_xattr_del() in osd-zfs returns -ENOENT when deleting nonexistent "system.posix_acl_access"

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.4.0
• Fix Version/s: Lustre 2.4.0
• Type: Bug
• Priority: Blocker
• Reporter: Li Wei (Inactive)
• Assignee: Minh Diep
• Resolution: Fixed
• Story Points: 1
• Severity: 3

Description

This leads to "cp -p" failures when copying files without ACLs. For example, sanity 103 may fail at "[22]". Deleting a nonexistent attribute, at least if it is "system.posix_acl_access", should succeed. That's also what ext4_xattr_acl_access_handler does.

Comments

[Alex Zhuravlev]

I'd rather think the caller should expect -ENOENT as a valid return.

[Andreas Dilger]

Assign to Minh for landing on master.

[Andreas Dilger]

Looking at the ldiskfs code for the current semantics:

/*
 * ext4_xattr_set_handle()
 *
 * Create, replace or remove an extended attribute for this inode.  Value
 * is NULL to remove an existing extended attribute, and non-NULL to
 * either replace an existing extended attribute, or create a new extended
 * attribute. The flags XATTR_REPLACE and XATTR_CREATE
 * specify that an extended attribute must exist and must not exist
 * previous to the call, respectively.
 *
 * Returns 0, or a negative error number on failure.
 */
int     
ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index,
                      const char *name, const void *value, size_t value_len,
                      int flags)
{       
        error = ext4_xattr_ibody_find(inode, &i, &is);
        if (error)
                goto cleanup;
        if (is.s.not_found)
                error = ext4_xattr_block_find(inode, &i, &bs);
        if (error)
                goto cleanup;
        if (is.s.not_found && bs.s.not_found) {
                error = -ENODATA;
                if (flags & XATTR_REPLACE)
                        goto cleanup;
                error = 0;
                if (!value)
                        goto cleanup;

So only if XATTR_REPLACE is set does the deletion of a non-existent entry cause -ENODATA to be returned. We can't even pass XATTR_REPLACE to the MDS for xattr deletion, nor is it possible with the VFS ->removexattr() method, so we may as well keep this semantic of not returning an error when deleting a non-existent xattr in the ZFS OSD layer as well.

[Andreas Dilger]

This is a blocker for ZFS functionality on 2.4. It isn't strictly required for 2.3, since we don't support ZFS MDTs with 2.3.

[Minh Diep]

patch for master http://review.whamcloud.com/#change,3833

[Minh Diep]

landed on master

[Alex Zhuravlev]

I'd like to hear opinions.. shouldn't MDD ignore such an error instead?

[Li Wei (Inactive)]

Alex,

osd-ldiskfs does not return an error when deleting a nonexistent "system.posix_acl_access" EA. Since that is not easy to change, osd-zfs shall do the same. Then, one concern is if osd-zfs should do the same for all EAs or just ACL ones. I think so far it looks "for all" is fine. If that ever becomes an issue, we can add EA name checks to osd-zfs.

[Andreas Dilger]

I was going to write the same as Li Wei. Since ldiskfs already hides this error, I have little motivation to patch the code just to discard the error at the MDD layer.

[Alex Zhuravlev]

ok, i'm fine with this then.