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

Update TxnLogger to use uuid_t #15

Open
wants to merge 8 commits into
base: tc-server-2.7
Choose a base branch
from

Conversation

iakshay
Copy link
Contributor

@iakshay iakshay commented Jul 16, 2019

Also, added some simple tests for creating TxnLog from nfs compound

@iakshay iakshay requested a review from excelle08 July 18, 2019 01:02
assert(nfs_vunlink_to_txnlog(arg, &txn_log) == 0);
break;
case txn_VRename:
case txn_VMkdir:
Copy link
Contributor

Choose a reason for hiding this comment

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

mkdir and symlink are actually CREATE operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is more inline with vNFS client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well but this is not consistent with get_txn_type() because it won't return VMkdir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about if we even plan to support VMkdir, since it seems to serve same purpose as VCreate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so we can just keep VCreate and discard VMkdir and VSymlink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vNFS client actually has both operations, I have to see if they are being used or not before removing.

Copy link
Contributor

Choose a reason for hiding this comment

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

But mkdir and symlink in the client side are finally translated into CREATE, aren't they? If get_txn_type() doesn't return VMkdir by inspecting arguments of opcreate structure, there is no point having case txn_VMkdir here.

@@ -196,8 +194,8 @@ void serialize_unlink_txn(struct TxnLog *txn_log,
struct UnlinkId *txnobj = &txn_log->created_unlink_ids[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that txn_log->backup_dir_path is not initialized before, so the system will abort with SIGSEGV. Did you run tests?

strcpy(object.name, absl::StrJoin(path_components, "/").c_str());
created_unlink_ids.push_back(object);
} else {
std::cerr << "Unknow operation for VRemove: " << ops[i].argop;
Copy link
Contributor

@excelle08 excelle08 Jul 29, 2019

Choose a reason for hiding this comment

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

Please also consider GETATTR, SAVEFH and other auxiliary operations. Also I'm not sure if std::cerr can write message into the log file.

for (int i = 0; i < ops_len; ++i) {
if (ops[i].argop == NFS4_OP_CREATE) {
txn_type = proto::TransactionType::VCREATE;
if (ops[i].argop == NFS4_OP_CREATE &&
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to recognize NFS4_OP_OPEN where new files may be created or existing files can be truncated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants