| Message ID | 20190724172026.23999-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ceph: don't list vxattrs in listxattr() | expand |
Jeff Layton <jlayton@kernel.org> writes: (CC'ing David) > Most filesystems that provide virtual xattrs (e.g. CIFS) don't display > them via listxattr(). Ceph does, and that causes some of the tests in > xfstests to fail. > > Have cephfs stop listing vxattrs in listxattr. Userspace can always > query them directly when the name is known. I don't see a problem with this, unless we already have applications relying on this behaviour. The first thing that came to my mind was samba, but I guess David can probably confirm whether this is true or not. If we're unable to stop listing there xattrs, the other option for fixing the xfstests that _may_ be acceptable by the maintainers is to use an output filter (lots of examples in common/filter). Cheers,
Hi, On Thu, 25 Jul 2019 10:37:40 +0100, Luis Henriques wrote: > Jeff Layton <jlayton@kernel.org> writes: > > (CC'ing David) > > > Most filesystems that provide virtual xattrs (e.g. CIFS) don't display > > them via listxattr(). Ceph does, and that causes some of the tests in > > xfstests to fail. > > > > Have cephfs stop listing vxattrs in listxattr. Userspace can always > > query them directly when the name is known. > > I don't see a problem with this, unless we already have applications > relying on this behaviour. The first thing that came to my mind was > samba, but I guess David can probably confirm whether this is true or > not. > > If we're unable to stop listing there xattrs, the other option for > fixing the xfstests that _may_ be acceptable by the maintainers is to > use an output filter (lots of examples in common/filter). Samba currently only makes use of the ceph.snap.btime vxattr. It doesn't depend on it appearing in the listxattr(), so removal would be fine. Not sure about other applications though. Cheers, David
On Thu, 2019-07-25 at 11:54 +0200, David Disseldorp wrote: > Hi, > > On Thu, 25 Jul 2019 10:37:40 +0100, Luis Henriques wrote: > > > Jeff Layton <jlayton@kernel.org> writes: > > > > (CC'ing David) > > > > > Most filesystems that provide virtual xattrs (e.g. CIFS) don't display > > > them via listxattr(). Ceph does, and that causes some of the tests in > > > xfstests to fail. > > > > > > Have cephfs stop listing vxattrs in listxattr. Userspace can always > > > query them directly when the name is known. > > > > I don't see a problem with this, unless we already have applications > > relying on this behaviour. The first thing that came to my mind was > > samba, but I guess David can probably confirm whether this is true or > > not. > > > > If we're unable to stop listing there xattrs, the other option for > > fixing the xfstests that _may_ be acceptable by the maintainers is to > > use an output filter (lots of examples in common/filter). Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave it a NAK. He's probably right though, and fixing it in ceph.ko is a better approach I think. > > Samba currently only makes use of the ceph.snap.btime vxattr. It doesn't > depend on it appearing in the listxattr(), so removal would be fine. Not > sure about other applications though. > > Cheers, David Ok, thanks guys. I'll go ahead and push this into the ceph/testing branch and see if teuthology complains. Thanks,
On Thu, 25 Jul 2019 07:17:11 -0400, Jeff Layton wrote: > Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave > it a NAK. He's probably right though, and fixing it in ceph.ko is a > better approach I think. It sounds as though Christoph's objection is to any use of a "ceph" xattr namespace for exposing CephFS specific information. I'm not sure what the alternatives would be, but I find the vxattrs much nicer for consumption compared to ioctls, etc. > > Samba currently only makes use of the ceph.snap.btime vxattr. It doesn't > > depend on it appearing in the listxattr(), so removal would be fine. Not > > sure about other applications though. > > > > Cheers, David > > Ok, thanks guys. I'll go ahead and push this into the ceph/testing > branch and see if teuthology complains. Sounds good. Feel free to add my RB tag. Cheers, David
On Thu, Jul 25, 2019 at 4:59 AM David Disseldorp <ddiss@suse.de> wrote: > > On Thu, 25 Jul 2019 07:17:11 -0400, Jeff Layton wrote: > > > Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave > > it a NAK. He's probably right though, and fixing it in ceph.ko is a > > better approach I think. > > It sounds as though Christoph's objection is to any use of a "ceph" > xattr namespace for exposing CephFS specific information. I'm not sure > what the alternatives would be, but I find the vxattrs much nicer for > consumption compared to ioctls, etc. Agreed. I don't understand the objection [1] at all. If the issue is that utilities copying a file may also copy xattrs, I don't understand why there would be an expectation that all xattrs are copyable or should be copied. [1] https://www.spinics.net/lists/fstests/msg12282.html
On Thu, Jul 25, 2019 at 12:04 PM Patrick Donnelly <pdonnell@redhat.com> wrote: > > On Thu, Jul 25, 2019 at 4:59 AM David Disseldorp <ddiss@suse.de> wrote: > > > > On Thu, 25 Jul 2019 07:17:11 -0400, Jeff Layton wrote: > > > > > Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave > > > it a NAK. He's probably right though, and fixing it in ceph.ko is a > > > better approach I think. > > > > It sounds as though Christoph's objection is to any use of a "ceph" > > xattr namespace for exposing CephFS specific information. I'm not sure > > what the alternatives would be, but I find the vxattrs much nicer for > > consumption compared to ioctls, etc. > > Agreed. I don't understand the objection [1] at all. > > If the issue is that utilities copying a file may also copy xattrs, I > don't understand why there would be an expectation that all xattrs are > copyable or should be copied. I'm sure it is about this, and that's the expectation because, uh, outside of weird things like Ceph then all matters are copyable and should be copied. That's how the interface is defined and built. I'm actually a bit puzzled to see this patch go by because I didn't think we listed the ceph.* matters on a listxattr command! If we want discoverability (to see what options are available on a running system) we can return them in response to a getxattr on the "ceph" xattr or something. -Greg > > [1] https://www.spinics.net/lists/fstests/msg12282.html > > -- > Patrick Donnelly, Ph.D. > He / Him / His > Senior Software Engineer > Red Hat Sunnyvale, CA > GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
On Thu, 2019-07-25 at 12:10 -0700, Gregory Farnum wrote: > On Thu, Jul 25, 2019 at 12:04 PM Patrick Donnelly <pdonnell@redhat.com> wrote: > > On Thu, Jul 25, 2019 at 4:59 AM David Disseldorp <ddiss@suse.de> wrote: > > > On Thu, 25 Jul 2019 07:17:11 -0400, Jeff Layton wrote: > > > > > > > Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave > > > > it a NAK. He's probably right though, and fixing it in ceph.ko is a > > > > better approach I think. > > > > > > It sounds as though Christoph's objection is to any use of a "ceph" > > > xattr namespace for exposing CephFS specific information. I'm not sure > > > what the alternatives would be, but I find the vxattrs much nicer for > > > consumption compared to ioctls, etc. > > > > Agreed. I don't understand the objection [1] at all. > > > > If the issue is that utilities copying a file may also copy xattrs, I > > don't understand why there would be an expectation that all xattrs are > > copyable or should be copied. > > I'm sure it is about this, and that's the expectation because, uh, > outside of weird things like Ceph then all matters are copyable and > should be copied. That's how the interface is defined and built. > > I'm actually a bit puzzled to see this patch go by because I didn't > think we listed the ceph.* matters on a listxattr command! If we want > discoverability (to see what options are available on a running > system) we can return them in response to a getxattr on the "ceph" > xattr or something. Yeah, that would be better, I think. For the record, ceph-fuse seems to also report these via listxattr. Testing with a build as of this morning: $ strace -e listxattr -f -s 256 getfattr -m '.' scratch listxattr("scratch", NULL, 0) = 133 listxattr("scratch", "ceph.dir.entries\0ceph.dir.files\0ceph.dir.subdirs\0ceph.dir.rentries\0ceph.dir.rfiles\0ceph.dir.rsubdirs\0ceph.dir.rbytes\0ceph.dir.rctime\0", 256) = 133 It might be good to have it stop doing this as well. -- Jeff Layton <jlayton@kernel.org>
On Thu, Jul 25, 2019 at 12:10 PM Gregory Farnum <gfarnum@redhat.com> wrote: > > On Thu, Jul 25, 2019 at 12:04 PM Patrick Donnelly <pdonnell@redhat.com> wrote: > > > > On Thu, Jul 25, 2019 at 4:59 AM David Disseldorp <ddiss@suse.de> wrote: > > > > > > On Thu, 25 Jul 2019 07:17:11 -0400, Jeff Layton wrote: > > > > > > > Yeah, I rolled a half-assed xfstests patch that did this, and HCH gave > > > > it a NAK. He's probably right though, and fixing it in ceph.ko is a > > > > better approach I think. > > > > > > It sounds as though Christoph's objection is to any use of a "ceph" > > > xattr namespace for exposing CephFS specific information. I'm not sure > > > what the alternatives would be, but I find the vxattrs much nicer for > > > consumption compared to ioctls, etc. > > > > Agreed. I don't understand the objection [1] at all. > > > > If the issue is that utilities copying a file may also copy xattrs, I > > don't understand why there would be an expectation that all xattrs are > > copyable or should be copied. > > I'm sure it is about this, and that's the expectation because, uh, > outside of weird things like Ceph then all matters are copyable and > should be copied. That's how the interface is defined and built. I don't really think anyone has defined what the interface really should do. It's not even in POSIX (but apparently was in a draft). The Linux documentation is silent on this matter AFAICT. Until Linux gives us an acceptable mechanism (ioctl is not) to expose/manipulate this information, we will continue to use xattrs. (This patch annoys me but I'm okay with merging it.) -- Patrick Donnelly, Ph.D. He / Him / His Senior Software Engineer Red Hat Sunnyvale, CA GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index 939eab7aa219..2fba06b50f25 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -903,11 +903,9 @@ ssize_t ceph_listxattr(struct dentry *dentry, char *names, size_t size) { struct inode *inode = d_inode(dentry); struct ceph_inode_info *ci = ceph_inode(inode); - struct ceph_vxattr *vxattrs = ceph_inode_vxattrs(inode); bool len_only = (size == 0); u32 namelen; int err; - int i; spin_lock(&ci->i_ceph_lock); dout("listxattr %p ver=%lld index_ver=%lld\n", inode, @@ -936,33 +934,6 @@ ssize_t ceph_listxattr(struct dentry *dentry, char *names, size_t size) names = __copy_xattr_names(ci, names); size -= namelen; } - - - /* virtual xattr names, too */ - if (vxattrs) { - for (i = 0; vxattrs[i].name; i++) { - size_t this_len; - - if (vxattrs[i].flags & VXATTR_FLAG_HIDDEN) - continue; - if (vxattrs[i].exists_cb && !vxattrs[i].exists_cb(ci)) - continue; - - this_len = strlen(vxattrs[i].name) + 1; - namelen += this_len; - if (len_only) - continue; - - if (this_len > size) { - err = -ERANGE; - goto out; - } - - memcpy(names, vxattrs[i].name, this_len); - names += this_len; - size -= this_len; - } - } err = namelen; out: spin_unlock(&ci->i_ceph_lock);
Most filesystems that provide virtual xattrs (e.g. CIFS) don't display them via listxattr(). Ceph does, and that causes some of the tests in xfstests to fail. Have cephfs stop listing vxattrs in listxattr. Userspace can always query them directly when the name is known. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/xattr.c | 29 ----------------------------- 1 file changed, 29 deletions(-)