Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

librbd: flush and invalidate cache via admin socket #6453

Merged
merged 3 commits into from Nov 9, 2015

Conversation

trociny
Copy link
Contributor

@trociny trociny commented Nov 3, 2015

Fixes: #2468
Signed-off-by: Mykola Golub mgolub@mirantis.com

@trociny
Copy link
Contributor Author

trociny commented Nov 4, 2015

@dillaman thank you for review. Rebased after addressing your comments.

@@ -41,6 +41,9 @@ enum {
l_librbd_readahead,
l_librbd_readahead_bytes,

l_librbd_flush_cache,
l_librbd_invalidate_cache,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two counter seemed a little different with other counters. Maybe another name?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The l_librbd_flush_cache can just use the existing l_librbd_flush perf counter. As for l_librbd_invalidate_cache, to me it is in-line with the other function-level perf counters such as l_librbd_snap_xyz, l_librbd_resize, etc.

@trociny
Copy link
Contributor Author

trociny commented Nov 4, 2015

@dillaman Thanks. Rebased after applying your suggestions.

@dillaman
Copy link

dillaman commented Nov 4, 2015

lgtm

@dillaman dillaman self-assigned this Nov 4, 2015
local counter=$2

ceph --admin-daemon $(rbd_watch_asok ${image}) perf dump |
jq ".\"librbd--rbd/${image}\".${counter}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change this to the following to work under Fedora 22: jq ".[\"librbd--rbd/${image}\"].${counter}". Also, the jq application is not necessarily installed. An alternative that doesn't require a new package dependency is something like:

ceph --admin-daemon $(rbd_watch_asok ${image}) perf dump librbd--rbd/${image} | grep ${counter} | awk '{print $NF}'

or use --format xml and the existing xmlstarlet dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will fix jq query.

The --format xml and xmlstarlet is the first thing I tried. Unfortunately it does not work due to "/" in "librbd--rbd/testimg23790" tag:

 zhuzha:~/ceph/ceph/src% ceph --format xml --admin-daemon out/client.admin.13067.asok perf dump |  xmlstarlet sel -t -m "."
 -:1.37: error parsing attribute name
 <perfcounter_collection><librbd--rbd/testimg23790><rd>0</rd><rd_bytes>0</rd_byte
                                     ^

(I think I have to create a PR for this).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for your comment that jq is not necessarily installed, we have in run-make-check.sh the instruction to install jq, before running the tests. That is why I decided it would be ok to use jq here, and not parse this e.g. with awk, which is fragile. I could also use a python oneliner with json module, which could be correct but heavier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldachary do you think it is ok to use jq for parsing json output in our tests or we'd rather avoid this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if we are already using it, I have no issue.

@trociny
Copy link
Contributor Author

trociny commented Nov 5, 2015

Rebased after fixing the jq query in the test.

@dillaman
Copy link

dillaman commented Nov 5, 2015

lgtm

@trociny
Copy link
Contributor Author

trociny commented Nov 8, 2015

I have updated the test, rbd_get_perfcounter() function, so it could work when #13720 is fixed (#6494).

Mykola Golub added 3 commits November 9, 2015 16:14
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Fixes: ceph#2468
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@trociny
Copy link
Contributor Author

trociny commented Nov 9, 2015

Rebased after updating the tests not to use jq.

dillaman pushed a commit that referenced this pull request Nov 9, 2015
librbd: flush and invalidate cache via admin socket

Reviewed-by: Jason Dillaman <dillaman@redhat.com>
@dillaman dillaman merged commit d93a795 into ceph:master Nov 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants