mds,client: add new getvxattr op by mchangir · Pull Request #42001 · ceph/ceph

mchangir · 2021-06-24T04:20:02Z

This new op fetches ceph namespace specific extended attributes
i.e. attributes that start with "ceph."

Fixes: https://tracker.ceph.com/issues/51062
Signed-off-by: Milind Changire mchangir@redhat.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

src/mds/Server.cc

batrick · 2021-06-25T22:16:50Z

src/mds/Server.cc

+  int r = 0;
+  ceph::bufferlist bl;
+  if (name == "ceph.dir.layout"sv) {
+    cur->get_inode()->layout.encode(bl, (uint64_t)-1);


Since these values should eventually be returned from the syscall, I think a text string makes sense. The client/kclient should not need to interpret (i.e. decode) these.

as per the discussion, I'm keeping the code to fetch individual layout fields in binary form rather than the proposed text form
this avoids the string parsing hassles on the client side as well

Coming back to this: I'm not sure what the point is of binary encoding anything for getvxattr. The client (kernel or Client) does not need any of these vxattrs to function. If the client does need a vxattr, it should be encoded and sent to the client with caps (like file layouts). So I'm not seeing a use-case to binary encode these values for any kind of convenience for the client. I would suggest JSON encoding the values and let the client pass it unmodified to the application/syscall.

src/mds/Server.cc

batrick · 2021-07-15T22:24:23Z

src/mds/Server.cc

+  int r = 0;
+  ceph::bufferlist bl;
+  if (name == "ceph.dir.layout"sv) {
+    cur->get_inode()->layout.encode(bl, (uint64_t)-1);


Coming back to this: I'm not sure what the point is of binary encoding anything for getvxattr. The client (kernel or Client) does not need any of these vxattrs to function. If the client does need a vxattr, it should be encoded and sent to the client with caps (like file layouts). So I'm not seeing a use-case to binary encode these values for any kind of convenience for the client. I would suggest JSON encoding the values and let the client pass it unmodified to the application/syscall.

batrick

Please change commit title to: mds,client: add new getvxattr op. It would probably be better to separate them too.

Add unit tests in src/test/libcephfs/

batrick · 2021-10-19T01:27:26Z

@mchangir let me know (here) when this is ready for review

mchangir · 2021-10-19T03:05:32Z

@mchangir let me know (here) when this is ready for review

@batrick mostly ready, please review

src/mds/Server.cc

src/client/Client.cc

batrick · 2021-10-26T23:38:51Z

src/test/libcephfs/vxattr.cc

+  int r = ceph_getxattr(cmount, "test/d0", "ceph.dir.layout", (void*)value, sizeof(value));
+  value[r] = '\0';
+  ASSERT_GT(r, 0);
+  ASSERT_STRNE((char*)NULL, strstr(value, "\"inheritance\": \"@default\""));


Break these into separate unit tests with different names please. A unit test should be as small and specific as possible. Otherwise, it makes the mental tracking of side-effects difficult to achieve by both the test author and future code-readers.

@batrick let me know if you need me to break the tests even further

vshankar · 2021-11-11T07:12:07Z

jenkins test make check

vshankar

@mchangir the added changes have weird indentation. That needs fixing too.

src/mds/Server.cc

src/client/Client.cc

src/mds/Server.cc

src/client/Client.cc

ajarr · 2021-11-25T00:40:08Z

src/client/Client.cc

+        memcpy(value, buf.c_str(), len);
+      }
+    }
+    res = len;


if size = 0, then we return 'len', is that what we want?

we do need to return len if output buffer size is passed as 0
I've rearranged the code a bit; please take a look

src/client/Client.cc

src/mds/Server.cc

batrick

Well done! I think documentation is the last piece before this is ready for general QA.

mchangir · 2022-02-08T02:36:09Z

jenkins test api

mchangir · 2022-02-08T06:38:35Z

Teuthology run for fs:libcephfs.

@mchangir

Server::handle_client_request() ignores unknown client operation by returning -ENOTSUPP, however, Server::perf_gather_op_latency() aborts on unknown client op, thereby causing -ENOTSUPP to never reach the client. ceph_abort() seems unnecessary here. Note, we could have invoked Server::perf_gather_op_latency() when the return value to client is not -ENOTSUPP, however, a valid client operation *might* just return -ENOTSUPP in some cases. @mchangir ran into this with his getvxattr op changes (PR ceph#42001). Signed-off-by: Venky Shankar <vshankar@redhat.com> (cherry picked from commit 2f4060b)

@mchangir

Server::handle_client_request() ignores unknown client operation by returning -ENOTSUPP, however, Server::perf_gather_op_latency() aborts on unknown client op, thereby causing -ENOTSUPP to never reach the client. ceph_abort() seems unnecessary here. Note, we could have invoked Server::perf_gather_op_latency() when the return value to client is not -ENOTSUPP, however, a valid client operation *might* just return -ENOTSUPP in some cases. @mchangir ran into this with his getvxattr op changes (PR ceph#42001). Signed-off-by: Venky Shankar <vshankar@redhat.com> (cherry picked from commit 2f4060b)

@default

This new op fetches ceph namespace specific extended attributes i.e. attributes "ceph.dir.layout.json", "ceph.file.layout.json" and "ceph.dir.pin*" For layout attributes, value of the .json xattrs provide the following features: 1. output in json format 2. resolves layout from the most closest ancestor inode, starting at self 3. adds an 'inheritance' field to show the layout inheritance where: @default - implies default system-wide layout @inherited - implies layout has been inherited from an ancestor @set - implies layout has been set for that specific inode 4. adds two fields: pool_name and pool_id to differentiate between the two values; especially for the situations where users have chosen to use purely digit sequences to name pools; since pool_id is system generated and its output is a decimal number as well, users cannot distinguish the value in the field named 'pool' Fixes: https://tracker.ceph.com/issues/51062 Signed-off-by: Milind Changire <mchangir@redhat.com>

This new op fetches ceph namespace specific extended attributes i.e. attributes 1. ceph.dir.layout.json and ceph.file.layout.json 2. ceph.dir.layout.pool_name and ceph.file.layout.pool_name 3. ceph.dir.layout.pool_id and ceph.file.layout.pool_id 4. ceph.dir.pin, ceph.dir.pin.random and ceph.dir.pin.distributed Fixes: https://tracker.ceph.com/issues/51062 Signed-off-by: Milind Changire <mchangir@redhat.com>

Setting the entire layout via setfattr -n ceph.dir.layout.json requires a json value with the name of all mandatory fields as returned by getfattr, except 'inheritance'. Also, the json format expects a 'pool_name' or 'pool_id'. If both are specified, the 'pool_name' is given priority over 'pool_id' for better disambiguation. ceph.(dir|file).layout.pool_name and ceph.(dir|file).layout.pool_id can also be set and fetched as individual fields. Fixes: https://tracker.ceph.com/issues/51062 Signed-off-by: Milind Changire <mchangir@redhat.com>

Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir · 2022-02-11T15:09:50Z

@vshankar PR seems ready for QA; we just need your PR to "ignore unknown op" to go in first, I guess;

Comments addressed.

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Master#2022-Mar-08

vshankar · 2022-03-07T05:02:43Z

Nice work, @mchangir

gregsfortytwo · 2022-03-09T14:36:41Z

Should this PR get backported into quincy @mchangir @vshankar ? It's not tagged in the redmine ticket.

vshankar · 2022-03-09T14:52:21Z

Should this PR get backported into quincy @mchangir @vshankar ? It's not tagged in the redmine ticket.

I recall @batrick mentioning that there is no real urgency for this to land in quincy. @mchangir would you concur?

@mchangir

Server::handle_client_request() ignores unknown client operation by returning -ENOTSUPP, however, Server::perf_gather_op_latency() aborts on unknown client op, thereby causing -ENOTSUPP to never reach the client. ceph_abort() seems unnecessary here. Note, we could have invoked Server::perf_gather_op_latency() when the return value to client is not -ENOTSUPP, however, a valid client operation *might* just return -ENOTSUPP in some cases. @mchangir ran into this with his getvxattr op changes (PR ceph#42001). Signed-off-by: Venky Shankar <vshankar@redhat.com> (cherry picked from commit 2f4060b)

mchangir requested a review from batrick June 24, 2021 04:20

github-actions bot added the cephfs Ceph File System label Jun 24, 2021

mchangir added the DNM label Jun 24, 2021

mchangir requested a review from vshankar June 24, 2021 10:52

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 1066508 to 9ffde77 Compare June 25, 2021 12:41

batrick requested changes Jun 25, 2021

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch 4 times, most recently from 0192e45 to 12f59e1 Compare July 1, 2021 14:08

batrick reviewed Jul 15, 2021

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 12f59e1 to 922d846 Compare July 26, 2021 06:09

batrick requested changes Sep 16, 2021

View reviewed changes

mchangir changed the title ~~mds/{client,server}: add new getvxattr op~~ mds: add new getvxattr op Sep 20, 2021

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 922d846 to 2aecaca Compare October 18, 2021 16:06

github-actions bot added build/ops tests labels Oct 18, 2021

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch 3 times, most recently from fff9d11 to a064f23 Compare October 18, 2021 18:00

mchangir marked this pull request as ready for review October 19, 2021 03:04

batrick requested changes Oct 26, 2021

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from a064f23 to 2da8424 Compare November 9, 2021 10:38

vshankar reviewed Nov 11, 2021

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 2da8424 to 90273d1 Compare November 18, 2021 08:51

ajarr reviewed Nov 25, 2021

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 90273d1 to aeff6e6 Compare November 25, 2021 08:44

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 86c7f4a to 16d1300 Compare January 27, 2022 16:07

batrick requested changes Feb 4, 2022

View reviewed changes

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from 16d1300 to ff704d0 Compare February 7, 2022 08:48

mchangir requested a review from a team as a code owner February 7, 2022 11:13

github-actions bot added the documentation label Feb 7, 2022

mchangir added 5 commits February 11, 2022 14:00

qa: unit test to test getvxattr

5d9c81c

Signed-off-by: Milind Changire <mchangir@redhat.com>

doc: describe json, pool_id and pool_name layout fields

ece8909

Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir force-pushed the mds-implement-new-getvxattr-rpc branch from ebccc82 to ece8909 Compare February 11, 2022 11:06

batrick approved these changes Feb 16, 2022

View reviewed changes

vshankar added the wip-vshankar-testing label Feb 21, 2022

vshankar approved these changes Mar 7, 2022

View reviewed changes

vshankar merged commit e0e2049 into ceph:master Mar 7, 2022

vshankar removed the wip-vshankar-testing label Mar 7, 2022

This was referenced Mar 14, 2022

quincy: mds,client: add new getvxattr op #45377

Merged

pacific: mds,client: add new getvxattr op #45487

Merged

batrick mentioned this pull request May 24, 2022

mds/client: Inherit layout from ancestor directories #37191

Closed

3 tasks

batrick mentioned this pull request Jul 13, 2022

mds/client: fail the request if the peer MDS doesn't support getvxattr op #47063

Merged

14 tasks

Conversation

mchangir commented Jun 24, 2021

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

batrick commented Oct 19, 2021

Uh oh!

mchangir commented Oct 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar commented Nov 11, 2021

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

mchangir commented Feb 8, 2022

Uh oh!

mchangir commented Feb 8, 2022

Uh oh!

mchangir commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Mar 7, 2022

Uh oh!

gregsfortytwo commented Mar 9, 2022

Uh oh!

vshankar commented Mar 9, 2022

Uh oh!

Reviewers

mchangir commented Feb 11, 2022 •

edited

Loading