I caught this last week when running valgrind on ZoL's zdb. it turned out to be a false positive because zdb killed itself to test ZFS' resilence against kernel crashes. If I recall correctly, I concluded that this is freed properly in the zio_root.
On Jan 29, 2014, at 4:40 PM, Matthew Ahrens <[email protected]> wrote: > > > > On Wed, Jan 29, 2014 at 7:32 AM, Andriy Gapon <[email protected]> wrote: >> on 14/01/2014 19:33 Andriy Gapon said the following: >> > I was worried about buffers that get put on l2arc_free_on_write list in >> > arc_buf_data_free() in the case when an L2 write is still ongoing. More >> > specifically, it seems that arc_hdr_destroy() would remove an arc_buf_hdr_t >> > object from l2ad_buflist if it's there before calling arc_buf_data_free(). >> > And >> > thus l2arc_write_done() won't see that buffer. >> >> I am more and more of opinion that L2ARC compression commit introduced a >> memory >> leak for compressed data buffers in arc_hdr_destroy. >> >> The code in arc_hdr_destroy destroys b_l2hdr header, but the actual data is >> still pointed to by b_buf. arc_hdr_destroy then calls arc_buf_destroy, which >> uses arc_buf_data_free to free the data. The data can either be freed >> immediately or, if the data is still in use by an L2ARC write zio, it can be >> put >> on l2arc_free_on_write list, so that it is freed later when the zio is done.. >> >> The code seems to be unchanged by the L2ARC compression commit and the code >> is >> sound as long as there is no compression. >> But if L2ARC compression is enabled, then in addition to the original data >> buffer we now also have a buffer with compressed data. That buffer is >> pointed >> to by b_tmp_cdata in l2arc_buf_hdr_t. >> There has to be some additional code to free the compressed buffer in the >> arc_hdr_destroy code path described above. b_tmp_cdata must be passed along >> with b_data via l2arc_free_on_write list and b_tmp_cdata must then be freed >> in >> l2arc_do_free_on_write. > > My understanding was that the b_tmp_cdata should be NULL when > arc_hdr_destroy() is called. It should have previously been freed and > NULL-ed out in l2arc_release_cdata_buf(). > > I would suggest that we add assertions in arc_hdr_destroy() that b_tmp_cdata > is NULL. Also, in l2arc_release_cdata_buf() b_tmp_cdata should be NULL if > b_compress != LZ4. > > --matt > > _______________________________________________ > developer mailing list > [email protected] > http://lists.open-zfs.org/mailman/listinfo/developer
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
