[RFC] ceph: don't list vxattrs in listxattr()
diff mbox series

Message ID 20190724172026.23999-1-jlayton@kernel.org
State New
Headers show
Series
  • [RFC] ceph: don't list vxattrs in listxattr()
Related show

Commit Message

Jeff Layton July 24, 2019, 5:20 p.m. UTC
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(-)

Comments

Luis Henriques July 25, 2019, 9:37 a.m. UTC | #1
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,
David Disseldorp July 25, 2019, 9:54 a.m. UTC | #2
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
Jeff Layton July 25, 2019, 11:17 a.m. UTC | #3
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,
David Disseldorp July 25, 2019, 11:58 a.m. UTC | #4
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
Patrick Donnelly July 25, 2019, 7:03 p.m. UTC | #5
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
Gregory Farnum July 25, 2019, 7:10 p.m. UTC | #6
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
Jeff Layton July 25, 2019, 7:47 p.m. UTC | #7
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>
Patrick Donnelly July 25, 2019, 7:48 p.m. UTC | #8
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

Patch
diff mbox series

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);