insanity in ll_dirty_page_discard_warn()

DISCUSSION Al Viro 4 participants 5 messages Jul 28, 2016 – Aug 1, 2016 VFS

Share & tools

Al Viro

author maintainer

2016-07-28 18:27 UTC #0 lore raw

        /* this can be called inside spin lock so use GFP_ATOMIC. */
        buf = (char *)__get_free_page(GFP_ATOMIC);
        if (buf) {
                dentry = d_find_alias(page->mapping->host);
	...
	if (dentry)
		dput(dentry);

If it *can* be called under a spinlock, you have an obvious problem - dput() can sleep. d_find_alias() might've picked a hashed dentry with zero refcount that got unhashed by the time of dput(). Or other references used to exist, but got dropped by that point...

Oleg Drokin

2016-07-28 19:25 UTC #1 lore raw

Quoted 8 lines · Al Viro

       /* this can be called inside spin lock so use GFP_ATOMIC. */
       buf = (char *)__get_free_page(GFP_ATOMIC);
       if (buf) {
               dentry = d_find_alias(page->mapping->host);
	...
	if (dentry)
		dput(dentry);

Quoted 4 lines

If it *can* be called under a spinlock, you have an obvious problem - dput() can sleep. d_find_alias() might've picked a hashed dentry with zero refcount that got unhashed by the time of dput(). Or other references used to exist, but got dropped by that point...

Ah, the dput()->dentry_kill()->cpu_relax() I guess?

(the final iput cannot catch us here, I think, because we still have pages in the mapping)

Hm? So the original reported path was:
ll_dirty_page_discard_warn at ffffffffa0a3d252 [lustre] vvp_page_completion_common at ffffffffa0a7adfc [lustre] vvp_page_completion_write_common at ffffffffa0a7ae6b [lustre] vvp_page_completion_write at ffffffffa0a7b83e [lustre] cl_page_completion at ffffffffa05eed8f [obdclass] osc_completion at ffffffffa0880812 [osc] osc_ap_completion at ffffffffa086a544 [osc] brw_interpret at ffffffffa0876d69 [osc]

But we don't even have a call to osc_ap_completion from brw_interpret anymore.

osc_ap_completion() itself has a comment that it is to be called under cl_loi_list_lock, but then tries to take it itself, so the comment is definitely stale.
And osc_completion() is called outside of that coverage.

I tend to think the comment is stale now, but need to do some more investigations before I am 100% sure of that.

Thanks for bringing it to our attention.

Ben Evans

2016-07-29 17:22 UTC #2 lore raw

I'm also working on a fix in osc_completion where rc=-ENOMEM, and a bunch of the asserts are not true since various structures haven't been initialized yet, so there is definitely some work to be done in the area.

-Ben

Quoted 49 lines · lustre-devel on behalf of Oleg Drokin

Quoted 8 lines · Al Viro
       /* this can be called inside spin lock so use GFP_ATOMIC. */
       buf = (char *)__get_free_page(GFP_ATOMIC);
       if (buf) {
               dentry = d_find_alias(page->mapping->host);
	...
	if (dentry)
		dput(dentry);
Quoted 5 lines
If it *can* be called under a spinlock, you have an obvious problem - dput() can sleep. d_find_alias() might've picked a hashed dentry with zero refcount that got unhashed by the time of dput(). Or other references
used to exist, but got dropped by that point...
Ah, the dput()->dentry_kill()->cpu_relax() I guess?
(the final iput cannot catch us here, I think, because we still have pages in the mapping)
Hm? So the original reported path was:
ll_dirty_page_discard_warn at ffffffffa0a3d252 [lustre] vvp_page_completion_common at ffffffffa0a7adfc [lustre] vvp_page_completion_write_common at ffffffffa0a7ae6b [lustre] vvp_page_completion_write at ffffffffa0a7b83e [lustre] cl_page_completion at ffffffffa05eed8f [obdclass] osc_completion at ffffffffa0880812 [osc] osc_ap_completion at ffffffffa086a544 [osc] brw_interpret at ffffffffa0876d69 [osc]
But we don't even have a call to osc_ap_completion from brw_interpret anymore.
osc_ap_completion() itself has a comment that it is to be called under cl_loi_list_lock, but then tries to take it itself, so the comment is definitely stale.
And osc_completion() is called outside of that coverage.
I tend to think the comment is stale now, but need to do some more investigations
before I am 100% sure of that.
Thanks for bringing it to our attention.
_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

DEGREMONT Aurelien

2016-08-01 07:54 UTC #3 lore raw

Le 29/07/2016 ? 19:22, Ben Evans a ?crit :

Quoted 3 lines

I'm also working on a fix in osc_completion where rc=-ENOMEM, and a bunch of the asserts are not true since various structures haven't been initialized yet, so there is definitely some work to be done in the area.

Ben, did you notice?
https://jira.hpdd.intel.com/browse/LU-8435

Do you have a public tracking of your progress in this area? (jira, gerrit, ...

Aur?lien

Ben Evans

2016-08-01 13:14 UTC #4 lore raw

No, I'd looked for that failure before, but there was nothing publicly available.

I finally have a dev setup to reproduce this error, so I'll be working on the fix this week.

-Ben

Quoted 14 lines · DEGREMONT Aurelien

Le 29/07/2016 ? 19:22, Ben Evans a ?crit :
Quoted 5 lines
I'm also working on a fix in osc_completion where rc=-ENOMEM, and a bunch
of the asserts are not true since various structures haven't been initialized yet, so there is definitely some work to be done in the area.
Ben, did you notice?
https://jira.hpdd.intel.com/browse/LU-8435
Do you have a public tracking of your progress in this area? (jira, gerrit, ...
Aur?lien