Ceph » CephFS

Forgot to mention this: I tested this on both an octopus build and a nautilus build.

Interesting design choice here. Is it causing issues for some application of yours? I'm inclined not to change the behavior in case it breaks some applciation using libcephfs.

Looks like a legit bug. The Linux kernel does this in __vfs_setxattr:

        if (size == 0)
                value = "";  /* empty EA, do not remove */

...so basically if you set size to 0 on setxattr() syscall, you should get an xattr that is set but is zero-length. I think we'll probably want userland ceph client to do the same.

Patrick Donnelly wrote:

Interesting design choice here. Is it causing issues for some application of yours? I'm inclined not to change the behavior in case it breaks some applciation using libcephfs.

Not directly, as my "application" is a wrapper around the Ceph APIs in Go (golang). However, I certainly found the current behavior unexpected given my prior experience using xattrs on Linux (kernel/libc/etc). I filed the issue preemptively assuming the consumers of our library could also be surprised by this behavior.

If the ceph team wants the function to continue behaving the same for some applications, to avoid breaking old applications, perhaps a new flag to the function to get more standard behavior would be appropriate? In our layer we could always set the flag if we are given a zero-length value.

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

Jeff Layton wrote:

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

Good Point. I think we should handle that case too. As for the ceph userland client's current behaviour, when the posix_acl xattr is set then posix_acl_check() in _do_setxattr() catches this and makes the function return EINVAL. If that xattr is not set, then we don't handle it at all. I will add handling of this in the PR.

Sidharth Anupkrishnan wrote:

Jeff Layton wrote:

The kernel seems to key its behavior on the size parameter. When it's 0, the pointer passed in is ignored and an empty string is used instead. I expect that when you pass in a NULL pointer with a non-zero size parameter, you probably get back EINVAL, but you probably ought to test that on a local fs (xfs or ext4).

Good Point. I think we should handle that case too. As for the ceph userland client's current behaviour, when the posix_acl xattr is set then posix_acl_check() in _do_setxattr() catches this and makes the function return EINVAL. If that xattr is not set, then we don't handle it at all. I will add handling of this in the PR.

Also tested it out in my local fs (ext4). Supplying NULL value with a non zero size does indeed give EINVAL.

Jeff Layton wrote:

Looks like a legit bug. The Linux kernel does this in __vfs_setxattr:

[...]

...so basically if you set size to 0 on setxattr() syscall, you should get an xattr that is set but is zero-length. I think we'll probably want userland ceph client to do the same.

If I remember right. Old kernel does not do this. this change was introduced by

commit 6c6ef9f26e598fb977f60935e109cd5b266c941a
Author: Andreas Gruenbacher <agruenba@redhat.com>
Date: Thu Sep 29 17:48:44 2016 +0200

xattr: Stop calling {get,set,remove}xattr inode operations