[LU-5051] HSM recv buffer reallocated unnecessarily Created: 12/May/14  Updated: 08/Oct/14  Resolved: 30/May/14

Status: Resolved
Project: Lustre
Component/s: None
Affects Version/s: Lustre 2.5.0, Lustre 2.6.0
Fix Version/s: Lustre 2.6.0, Lustre 2.5.4

Type: Improvement Priority: Minor
Reporter: Patrick Farrell (Inactive) Assignee: Cliff White (Inactive)
Resolution: Fixed Votes: 0
Labels: hsm, patch

Attachments: File 0003-Instead-of-allocating-1MB-for-every-call-to-llapi_hs.patch    
Rank (Obsolete): 13954

 Description   

Currently, the 1 MB buffer used by llapi_hsm_copytool_recv is allocated and freed every time through the ct_run loop.

This is unnecessary, as long as the copy tool clears the the data before calling llapi_hsm_copytool_recv, which lhsmtool_posix already does.

This patch allocates the buffer once in llapi_hsm_copytool_register and frees it in unregister.

Patch will be available in Gerrit shortly.

 Comments   
Comment by Robert Read (Inactive) [ 12/May/14 ]

Would it make sense fir llapi_hsm_copytool_recv() to reinitialize the buffer memset(0) each time rather than relying on the application to do this?

Comment by Patrick Farrell (Inactive) [ 12/May/14 ]

Good point, I can update the patch to do that. Before I do that, would you weigh in on my comment in the patch:
[...] In llapi_hsm_copytool_recv, now that we no longer free the buffer at the end of this function, should we rename "out_free" to "out_err"?

I'm probably going to make that change unless anyone disagrees.

Comment by Frank Zago (Inactive) [ 13/May/14 ]

The existing code is not memset'ing the buffer (using malloc, not calloc), and it belongs to the application, so there's no leak involved. Since it's also a very large buffer, I don't think we should add that.

Comment by Robert Read (Inactive) [ 13/May/14 ]

Yes, please change the label, in case you are still wondering about that.

As for the memset issue - Patrick mentioned that the copytool is currently clearing the data but I didn't see it doing that. Since this buffer was being managed by the API then it made sense to me to clear it there, but I agree it might not be required, either.

Comment by Patrick Farrell (Inactive) [ 13/May/14 ]

I opened LU-5059 via Maloo for the test failures. (On all of the test runs, all of the sub tests passed, but the overall suite still reported failure...)

Comment by Patrick Farrell (Inactive) [ 19/May/14 ]

Patch is here:
http://review.whamcloud.com/#/c/10299/

Needs refresh, doing that shortly...

Comment by Cliff White (Inactive) [ 20/May/14 ]

I will monitor this one, thanks for the patch.

Comment by Peter Jones [ 30/May/14 ]

Landed for 2.6

Comment by James Nunez (Inactive) [ 02/Jul/14 ]

Patch for b2_5 at http://review.whamcloud.com/#/c/10716

Generated at Sat Feb 10 01:48:06 UTC 2024 using Jira 9.4.14#940014-sha1:734e6822bbf0d45eff9af51f82432957f73aa32c.