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

cephfs-journal-tool: enable purge_queue journal's event commands #23467

Conversation

xxhdx1985126
Copy link
Contributor

@xxhdx1985126 xxhdx1985126 commented Aug 7, 2018

A new PR with increased struct version of PurgeItem for PR #22850

Resolves: http://tracker.ceph.com/issues/24604
Signed-off-by: Xuehan Xu xuxuehan@360.cn

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

@xxhdx1985126 xxhdx1985126 force-pushed the wip-cephfs-journal-tool-purge_queue-event-commands branch 3 times, most recently from 8dca3e9 to 300f2f5 Compare August 7, 2018 11:05
@tchaikov tchaikov added the cephfs Ceph File System label Aug 7, 2018
decode(stamp, p);
decode(pad_size, p);
p.advance(pad_size);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

encoding action/ino/size after padding. does this work for JournalTool::erase_region

enoop = ENoOp(padding);
enoop.encode_with_header(entry, CEPH_FEATURES_SUPPORTED_DEFAULT);
} else if (type == "purge_queue") {
pi.pad_size = padding;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ukernel yes, when calculating pad_size, all the space occupied by other fields are excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

ENoOp has not action/ino/size, why the ways to calculate padding are the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the way to calculate padding is to exclude the length of a purgeitem/logevent whose pad_size is 0 which include action/ino/size if the purgeitem/logevent has such fields

@@ -45,11 +50,35 @@ class PurgeItem
fragtree_t fragtree;

PurgeItem()
: action(NONE), ino(0), size(0)
: stamp(ceph_clock_now()), pad_size(0), action(NONE), ino(0), size(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

stamp(ceph_clock_now()) wastes cpu cycle when reading purge queue item from journal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok:-) I'll correct that:-).
BTW, I see you removed the last comment, is there anything wrong?

@ceph ceph deleted a comment from xxhdx1985126 Aug 8, 2018
@xxhdx1985126 xxhdx1985126 force-pushed the wip-cephfs-journal-tool-purge_queue-event-commands branch from 300f2f5 to 031670a Compare August 8, 2018 06:19
@xxhdx1985126
Copy link
Contributor Author

@ukernel Hi, I've just modified the code as you suggested, please take a look:-)

uint8_t static const pad = 0xff;
for (unsigned int i = 0; i<pad_size; i++) {
encode(pad, bl);
}
Copy link
Contributor

@ukernel ukernel Aug 9, 2018

Choose a reason for hiding this comment

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

void PurgeItem::encode(bufferlist &bl) const
{
  ENCODE_START(2, 1, bl);
  encode((uint8_t)action, bl);
  encode(ino, bl);
  encode(size, bl);
  encode(layout, bl, CEPH_FEATURE_FS_FILE_LAYOUT_V2);
  encode(old_pools, bl);
  encode(snapc, bl);
  encode(fragtree, bl);
  encode(stamp, bl);
  uint8_t static const pad = 0xff;
  for (unsigned int i = 0; i<pad_size; i++) {
    encode(pad, bl);
  }
  ENCODE_FINISH(bl);
}

We can keep backward compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes:-)

@xxhdx1985126 xxhdx1985126 force-pushed the wip-cephfs-journal-tool-purge_queue-event-commands branch from 031670a to 3179374 Compare August 9, 2018 07:31
void PurgeItem::encode(bufferlist &bl) const
{
ENCODE_START(1, 1, bl);
ENCODE_START(2, 2, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

2, 1

@xxhdx1985126 xxhdx1985126 force-pushed the wip-cephfs-journal-tool-purge_queue-event-commands branch from 3179374 to 5582fae Compare August 9, 2018 07:36
@xxhdx1985126
Copy link
Contributor Author

@ukernel Hi, thanks for the review. Required changes are done:-) Please take a look:-)

@batrick
Copy link
Member

batrick commented Aug 16, 2018

retest this please

@batrick batrick merged commit 5582fae into ceph:master Aug 20, 2018
batrick added a commit that referenced this pull request Aug 20, 2018
* refs/pull/23467/head:
	cephfs-journal-tool: enable purge_queue journal's event commands

Reviewed-by: Zheng Yan <zyan@redhat.com>
@xxhdx1985126 xxhdx1985126 deleted the wip-cephfs-journal-tool-purge_queue-event-commands branch June 18, 2019 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants