dmu_zfetch: don't leak unreferenced stream when zfetch is freed

[mattmacy]

Currently streams are only freed when:

• They have no referencing zfetch and and their I/O references
  go to zero.
• They are more than 2s old and a new I/O request comes in on
  the same zfetch.

This means that we will leak unreferenced streams when their zfetch
structure is freed.

This change checks the reference count on a stream at zfetch free
time. If it is zero we free it immediately. If it has remaining
references we allow the prefetch callback to free it at I/O
completion time.

Signed-off-by: Matt Macy [email protected]

Motivation and Context

Description

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[adamdmoss]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

[mattmacy]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

No, not at all. When the file is closed but the I/O hasn't completed there will still be references.

[adamdmoss]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

No, not at all. When the file is closed but the I/O hasn't completed there will still be references.

Cool, but how is an orphaned stream not leaked? As far as I can tell the only surviving pointer to the stream is in the zf->zf_stream list, which is about to be destroyed.

[mattmacy]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

No, not at all. When the file is closed but the I/O hasn't completed there will still be references.

Cool, but how is an orphaned stream not leaked? As far as I can tell the only surviving pointer to the stream is in the zf->zf_stream list, which is about to be destroyed.

The completion routine frees it in that case. Hence the fetch pointer.

[adamdmoss]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

Why should it ever orphan? Orphan == leak as far as I can tell. :(

No, not at all. When the file is closed but the I/O hasn't completed there will still be references.

Cool, but how is an orphaned stream not leaked? As far as I can tell the only surviving pointer to the stream is in the zf->zf_stream list, which is about to be destroyed.

The completion routine frees it in that case. Hence the fetch pointer.

That's how I think it should work, but I don't believe it does. Nowhere does the code access zs_fetch.

I may be wrong - you're the expert - I'd be grateful if you could point me to the code in question so I can smack myself upside the head. :)

[mattmacy]

Why should it ever orphan? Orphan == leak as far as I can tell. :(

Why should it ever orphan? Orphan == leak as far as I can tell. :(

No, not at all. When the file is closed but the I/O hasn't completed there will still be references.

Cool, but how is an orphaned stream not leaked? As far as I can tell the only surviving pointer to the stream is in the zf->zf_stream list, which is about to be destroyed.

The completion routine frees it in that case. Hence the fetch pointer.

That's how I think it should work, but I don't believe it does. Nowhere does the code access zs_fetch.

I may be wrong - you're the expert - I'd be grateful if you could point me to the code in question so I can smack myself upside the head. :)

Uhm ... orphan sets it to NULL. The completion routine checks against NULL and then frees it. Not sure how you mean it doesn't access zs_fetch.

static void
dmu_zfetch_stream_orphan(zfetch_t *zf, zstream_t *zs)
{
	ASSERT(MUTEX_HELD(&zf->zf_lock));
	list_remove(&zf->zf_stream, zs);
	zs->zs_fetch = NULL;
	zf->zf_numstreams--;
}

static void
dmu_zfetch_stream_done(void *arg, boolean_t io_issued)
{
	zstream_t *zs = arg;

	if (zs->zs_start_time && io_issued) {
		hrtime_t now = gethrtime();
		hrtime_t delta = NSEC2USEC(now - zs->zs_start_time);

		zs->zs_start_time = 0;
		ZFETCHSTAT_SET(zfetchstat_last_completion_us, delta);
		if (delta > ZFETCHSTAT_GET(zfetchstat_max_completion_us))
			ZFETCHSTAT_SET(zfetchstat_max_completion_us, delta);
	}

	if (zfs_refcount_remove(&zs->zs_blocks, NULL) != 0)
		return;

	/*
	 * The parent fetch structure has gone away
	 */
	if (zs->zs_fetch == NULL)
		dmu_zfetch_stream_fini(zs);
}

[adamdmoss]

I think maybe I see it - dmu_zfetch_stream_done() detects orphans with zs->zs_fetch == NULL and destroys them properly?
Is that right?

[mattmacy]

I think maybe I see it - dmu_zfetch_stream_done() detects orphans with zs->zs_fetch == NULL and destroys them properly?
Is that right?

Correct.

[adamdmoss]

Thanks so much, thanks for your patience.
So the leak occurs when the completion routine is called before dmu_zfetch_fini(), but not the other way around, right?

[mattmacy]

Thanks so much, thanks for your patience.
So the leak occurs when the completion routine is called before dmu_zfetch_fini(), but not the other way around, right?

Correct.

[adamdmoss]

In that case I wholeheartedly agree with your fix, thanks for your patience. :)

[adamdmoss]

Seems like hubris for me to review this change after I submitted a bogus fix but I approve. 👍

[mattmacy]

Seems like hubris for me to review this change after I submitted a bogus fix but I approve. +1

Well, you did find the bug.

[adamdmoss]

Has this been considered for zfs-2.0.0-rcX? @behlendorf
Cheers.

[behlendorf]

@adamdmoss the original zfetch change was not applied to the 2.0.0 branch so there's no need to pick up this fix.

[adamdmoss]

Oh - hooray!