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.

Sign up for GitHub

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

DAOS-6709 rsvc: xfer only attr size and fix tests #4745

Merged
merged 7 commits into from
Mar 23, 2021
+23 −1
Merged

DAOS-6709 rsvc: xfer only attr size and fix tests #4745

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e508f8e
DAOS-6709 rsvc: xfer only attr size and fix tests
bfaccini Feb 23, 2021
885a6ff
Merge branch 'master' into DAOS-6709_2nd
bfaccini Feb 25, 2021
04bf17b
Merge branch 'master' into DAOS-6709_2nd
bfaccini Mar 2, 2021
637617d
Merge branch 'master' into DAOS-6709_2nd
bfaccini Mar 2, 2021
8e5819d
Merge branch 'master' into DAOS-6709_2nd
bfaccini Mar 5, 2021
af2534e
Merge branch 'master' into DAOS-6709_2nd
bfaccini Mar 9, 2021
d9c7dca
DAOS-6709 rsvc: send each segment as a single bulk
bfaccini Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/rsvc/srv_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,18 @@ ds_rsvc_get_attr(struct ds_rsvc *svc, struct rdb_tx *tx, rdb_path_t *path,
svc->s_name, DP_KEY(&key), rc);
goto out_iovs;
}
iovs[j].iov_buf_len = sizes[i];
sizes[i] = iovs[j].iov_len;
if (iovs[j].iov_len < sizes[i]) {
/* do not xfer more than attr length */
bulk_size -= sizes[i] - iovs[j].iov_len;
/* return real size sent */
sizes[i] = iovs[j].iov_len;
} else {
/* only return requested size */
iovs[j].iov_buf_len = sizes[i];
/* only attr size is requested */
if (sizes[i] == 0)
sizes[i] = iovs[j].iov_len;
}
Copy link
Contributor

@liuxuezhao liuxuezhao Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I see you only want to transfer the real size for each attribute, but the handling here is incorrect.

  1. I think you need to change attr_bulk_transfer() - start one crt_bulk_transfer() for each attribute's actual size, and set correct offset/length for each attribute's value's bulk transfer.
  2. the sizes setting and bulk_size setting above looks not correct to me.

You may refer obj_bulk_transfer()'s code, just FYI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, Bruno's change looks right to me. The problem is that the original author fell into the crt pitfall that crt looks at iov_buf_len instead of iov_len when creating a bulk handle, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

in client's attr_bulk_create(), the bulk handle is created by -
2267 > /* values */
2268 > if (sizes != NULL && values != NULL) {
2269 > > for (j = 0; j < n; ++j)
2270 > > > if (sizes[j] > 0)
2271 > > > > d_iov_set(&sgl.sg_iovs[i++],
2272 > > > > > values[j], sizes[j]);
2273 > }

i.e. for the attr values, each bulk segment is set to user provided buffer list.

Then at server-side, need to start multi-bulk-transferring to fill to each attr's value's segment, if the real size of some middle segments is less than user provided buffer.
For example, client read 2 attrs, each provide 1KB buffer. But actually at server side, it finds that each attr only with value of 0.5KB. Then should start 2 bulk transfer, 1st transfer to first segment's 0-0.5KB, 2nd transfer to 2nd segment's 0-0.5KB. This is what this patch want to do, no?
or I may mis-understood it...

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I mistakenly assumed that the client-side buffer is one continuous segment.

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 a lot @liuxuezhao and @liw for these very interesting and helpful comments. I think I had to have a look to the similar behaviour in obj_bulk_transfer() before !!... And also it seems to me that attr_bulk_transfer() has been designed to work with a single/common buffer at some point of time...
Will try to push a new commit soon, based on your comments, many thanks again !

Copy link
Contributor

Choose a reason for hiding this comment

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

So you are planning to update this PR @bfaccini ? I noticed that @liw approved it after that comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps it's just github being weird. It shows this comment block and afterward, the approval but the approval appears tied to the prior comment from @liw, not the most recent one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you are planning to update this PR @bfaccini ?

@jolivier23 , yes I will try to implement the fix for DAOS-6709 based on these comments.

I think @liw had first approved and then felt ok with @liuxuezhao about the way that should be used to fix...


/* If buffer length is zero, send only size */
if (iovs[j].iov_buf_len > 0)
Expand Down
9 changes: 6 additions & 3 deletions src/tests/suite/daos_container.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ co_attribute(void **state)
};
int n = (int) ARRAY_SIZE(names);
char out_buf[10 * BUFSIZE] = { 0 };
/* use 2 consecutives pieces of big buffer to avoid the need to
* unpack and merge bulk segments
*/
void *out_values[] = {
&out_buf[0 * BUFSIZE],
&out_buf[1 * BUFSIZE]
Expand Down Expand Up @@ -169,12 +172,12 @@ co_attribute(void **state)

print_message("Verifying Name-Value (A)..\n");
assert_int_equal(out_sizes[0], in_sizes[0]);
assert_memory_equal(out_values[0], in_values[0], in_sizes[0]);
assert_memory_equal(out_buf, in_values[0], in_sizes[0]);

print_message("Verifying Name-Value (B)..\n");
assert_true(in_sizes[1] > BUFSIZE);
assert_int_equal(out_sizes[1], in_sizes[1]);
assert_memory_equal(out_values[1], in_values[1], BUFSIZE);
assert_int_equal(out_sizes[1], BUFSIZE);
assert_memory_equal(out_buf + out_sizes[0], in_values[1], out_sizes[1]);

rc = daos_cont_get_attr(arg->coh, n, names, NULL, out_sizes,
arg->async ? &ev : NULL);
Expand Down
9 changes: 6 additions & 3 deletions src/tests/suite/daos_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,9 @@ pool_attribute(void **state)
};
int n = (int) ARRAY_SIZE(names);
char out_buf[10 * BUFSIZE] = { 0 };
/* use 2 consecutives pieces of big buffer to avoid the need to
* unpack and merge bulk segments
*/
void *out_values[] = {
&out_buf[0 * BUFSIZE],
&out_buf[1 * BUFSIZE]
Expand Down Expand Up @@ -304,12 +307,12 @@ pool_attribute(void **state)

print_message("Verifying Name-Value (A)..\n");
assert_int_equal(out_sizes[0], in_sizes[0]);
assert_memory_equal(out_values[0], in_values[0], in_sizes[0]);
assert_memory_equal(out_buf, in_values[0], in_sizes[0]);

print_message("Verifying Name-Value (B)..\n");
assert_true(in_sizes[1] > BUFSIZE);
assert_int_equal(out_sizes[1], in_sizes[1]);
assert_memory_equal(out_values[1], in_values[1], BUFSIZE);
assert_int_equal(out_sizes[1], BUFSIZE);
assert_memory_equal(out_buf + out_sizes[0], in_values[1], out_sizes[1]);

rc = daos_pool_get_attr(arg->pool.poh, n, names, NULL, out_sizes,
arg->async ? &ev : NULL);
Expand Down