[PATCH 1/3] FS-Cache: Timeout for releasepage()

[PATCH 1/3] FS-Cache: Timeout for releasepage()

[Milosz Tanski]

This is meant to avoid a recursive hang caused by underlying filesystem trying
to grab a free page and causing a write-out.

INFO: task kworker/u30:7:28375 blocked for more than 120 seconds.
      Not tainted 3.15.0-virtual #74
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u30:7   D 0000000000000000     0 28375      2 0x00000000
Workqueue: fscache_operation fscache_op_work_func [fscache]
 ffff88000b147148 0000000000000046 0000000000000000 ffff88000b1471c8
 ffff8807aa031820 0000000000014040 ffff88000b147fd8 0000000000014040
 ffff880f0c50c860 ffff8807aa031820 ffff88000b147158 ffff88007be59cd0
Call Trace:
 [<ffffffff815930e9>] schedule+0x29/0x70
 [<ffffffffa018bed5>] __fscache_wait_on_page_write+0x55/0x90 [fscache]
 [<ffffffff810a4350>] ? __wake_up_sync+0x20/0x20
 [<ffffffffa018c135>] __fscache_maybe_release_page+0x65/0x1e0 [fscache]
 [<ffffffffa02ad813>] ceph_releasepage+0x83/0x100 [ceph]
 [<ffffffff811635b0>] ? anon_vma_fork+0x130/0x130
 [<ffffffff8112cdd2>] try_to_release_page+0x32/0x50
 [<ffffffff81140096>] shrink_page_list+0x7e6/0x9d0
 [<ffffffff8113f278>] ? isolate_lru_pages.isra.73+0x78/0x1e0
 [<ffffffff81140932>] shrink_inactive_list+0x252/0x4c0
 [<ffffffff811412b1>] shrink_lruvec+0x3e1/0x670
 [<ffffffff8114157f>] shrink_zone+0x3f/0x110
 [<ffffffff81141b06>] do_try_to_free_pages+0x1d6/0x450
 [<ffffffff8114a939>] ? zone_statistics+0x99/0xc0
 [<ffffffff81141e44>] try_to_free_pages+0xc4/0x180
 [<ffffffff81136982>] __alloc_pages_nodemask+0x6b2/0xa60
 [<ffffffff811c1d4e>] ? __find_get_block+0xbe/0x250
 [<ffffffff810a405e>] ? wake_up_bit+0x2e/0x40
 [<ffffffff811740c3>] alloc_pages_current+0xb3/0x180
 [<ffffffff8112cf07>] __page_cache_alloc+0xb7/0xd0
 [<ffffffff8112da6c>] grab_cache_page_write_begin+0x7c/0xe0
 [<ffffffff81214072>] ? ext4_mark_inode_dirty+0x82/0x220
 [<ffffffff81214a89>] ext4_da_write_begin+0x89/0x2d0
 [<ffffffff8112c6ee>] generic_perform_write+0xbe/0x1d0
 [<ffffffff811a96b1>] ? update_time+0x81/0xc0
 [<ffffffff811ad4c2>] ? mnt_clone_write+0x12/0x30
 [<ffffffff8112e80e>] __generic_file_aio_write+0x1ce/0x3f0
 [<ffffffff8112ea8e>] generic_file_aio_write+0x5e/0xe0
 [<ffffffff8120b94f>] ext4_file_write+0x9f/0x410
 [<ffffffff8120af56>] ? ext4_file_open+0x66/0x180
 [<ffffffff8118f0da>] do_sync_write+0x5a/0x90
 [<ffffffffa025c6c9>] cachefiles_write_page+0x149/0x430 [cachefiles]
 [<ffffffff812cf439>] ? radix_tree_gang_lookup_tag+0x89/0xd0
 [<ffffffffa018c512>] fscache_write_op+0x222/0x3b0 [fscache]
 [<ffffffffa018b35a>] fscache_op_work_func+0x3a/0x100 [fscache]
 [<ffffffff8107bfe9>] process_one_work+0x179/0x4a0
 [<ffffffff8107d47b>] worker_thread+0x11b/0x370
 [<ffffffff8107d360>] ? manage_workers.isra.21+0x2e0/0x2e0
 [<ffffffff81083d69>] kthread+0xc9/0xe0
 [<ffffffff81010000>] ? ftrace_raw_event_xen_mmu_release_ptpage+0x70/0x90
 [<ffffffff81083ca0>] ? flush_kthread_worker+0xb0/0xb0
 [<ffffffff8159eefc>] ret_from_fork+0x7c/0xb0
 [<ffffffff81083ca0>] ? flush_kthread_worker+0xb0/0xb0
---
 fs/fscache/page.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 7f5c658..84e11aa 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -44,6 +44,19 @@ void __fscache_wait_on_page_write(struct fscache_cookie *cookie, struct page *pa
 EXPORT_SYMBOL(__fscache_wait_on_page_write);
 
 /*
+ * wait for a page to finish being written to the cache. Put a timeout here
+ * since we might be called recusively via parent fs.
+ */
+static
+bool relase_page_wait_timeout(struct fscache_cookie *cookie, struct page *page)
+{
+	wait_queue_head_t *wq = bit_waitqueue(&cookie->flags, 0);
+
+	return wait_event_timeout(*wq, !__fscache_check_page_write(cookie, page),
+				  HZ);
+}
+
+/*
  * decide whether a page can be released, possibly by cancelling a store to it
  * - we're allowed to sleep if __GFP_WAIT is flagged
  */
@@ -115,7 +128,7 @@ page_busy:
 	}
 
 	fscache_stat(&fscache_n_store_vmscan_wait);
-	__fscache_wait_on_page_write(cookie, page);
+	relase_page_wait_timeout(cookie, page);
 	gfp &= ~__GFP_WAIT;
 	goto try_again;
 }
-- 
1.7.9.5

Re: [PATCH 1/3] FS-Cache: Timeout for releasepage()

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> +bool relase_page_wait_timeout(struct fscache_cookie *cookie, struct page *page

I presume you mean "release_page_wait_timeout".

Also, the new function returns a value which you then ignore.  Should you do
something with it?

David

Re: [PATCH 1/3] FS-Cache: Timeout for releasepage()

[Milosz Tanski]

Originally I was planning doing something with that (a debug message)
but I decided against it and forgot to remove it.

Neil Brown came up with a very similar solution that he posted as I
was developing my own. His patch might be a better candidate.

- M

On Tue, Jul 29, 2014 at 11:32 AM, David Howells <dhowells@redhat.com> wrote:
> Milosz Tanski <milosz@adfin.com> wrote:
>
>> +bool relase_page_wait_timeout(struct fscache_cookie *cookie, struct page *page
>
> I presume you mean "release_page_wait_timeout".
>
> Also, the new function returns a value which you then ignore.  Should you do
> something with it?
>
> David



-- 
Milosz Tanski
CTO
16 East 34th Street, 15th floor
New York, NY 10016

p: 646-253-9055
e: milosz@adfin.com

[PATCH 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[Milosz Tanski]

---
 fs/fscache/object.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index d3b4539..186076b 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -925,6 +925,7 @@ static const struct fscache_state *_fscache_invalidate_object(struct fscache_obj
 {
 	struct fscache_operation *op;
 	struct fscache_cookie *cookie = object->cookie;
+	bool wake_cookie = false;
 
 	_enter("{OBJ%x},%d", object->debug_id, event);
 
@@ -981,7 +982,10 @@ nomem:
 
 submit_op_failed:
 	clear_bit(FSCACHE_OBJECT_IS_LIVE, &object->flags);
+	wake_cookie = __fscache_unuse_cookie(cookie);
 	spin_unlock(&cookie->lock);
+	if (wake_cookie)
+		__fscache_wake_unused_cookie(cookie);
 	kfree(op);
 	_leave(" [EIO]");
 	return transit_to(KILL_OBJECT);
-- 
1.7.9.5

Re: [PATCH 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> +	wake_cookie = __fscache_unuse_cookie(cookie);
>  	spin_unlock(&cookie->lock);
> +	if (wake_cookie)
> +		__fscache_wake_unused_cookie(cookie);

Why do __fscache_unuse_cookie() with cookie->lock held?

David

Re: [PATCH 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[Milosz Tanski]

The honest answer is I don't know if it know if needs to be unlocked
before or after. I saw a same pattern with unlocking order inside of
__fscache_attr_changed in the failure case.

If this can be re-ordered I can take care of that in my next version I
submit to you.

- Milosz

On Tue, Jul 29, 2014 at 11:37 AM, David Howells <dhowells@redhat.com> wrote:
> Milosz Tanski <milosz@adfin.com> wrote:
>
>> +     wake_cookie = __fscache_unuse_cookie(cookie);
>>       spin_unlock(&cookie->lock);
>> +     if (wake_cookie)
>> +             __fscache_wake_unused_cookie(cookie);
>
> Why do __fscache_unuse_cookie() with cookie->lock held?
>
> David



-- 
Milosz Tanski
CTO
16 East 34th Street, 15th floor
New York, NY 10016

p: 646-253-9055
e: milosz@adfin.com

Re: [PATCH 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[Milosz Tanski]

Any suggestion what's the right thing to do here David? I'd like to
re-spin and re-submit the patches.

- M

On Wed, Aug 6, 2014 at 11:21 AM, Milosz Tanski <milosz@adfin.com> wrote:
> The honest answer is I don't know if it know if needs to be unlocked
> before or after. I saw a same pattern with unlocking order inside of
> __fscache_attr_changed in the failure case.
>
> If this can be re-ordered I can take care of that in my next version I
> submit to you.
>
> - Milosz
>
> On Tue, Jul 29, 2014 at 11:37 AM, David Howells <dhowells@redhat.com> wrote:
>> Milosz Tanski <milosz@adfin.com> wrote:
>>
>>> +     wake_cookie = __fscache_unuse_cookie(cookie);
>>>       spin_unlock(&cookie->lock);
>>> +     if (wake_cookie)
>>> +             __fscache_wake_unused_cookie(cookie);
>>
>> Why do __fscache_unuse_cookie() with cookie->lock held?
>>
>> David
>
>
>
> --
> Milosz Tanski
> CTO
> 16 East 34th Street, 15th floor
> New York, NY 10016
>
> p: 646-253-9055
> e: milosz@adfin.com



-- 
Milosz Tanski
CTO
16 East 34th Street, 15th floor
New York, NY 10016

p: 646-253-9055
e: milosz@adfin.com

Re: [PATCH 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> The honest answer is I don't know if it know if needs to be unlocked
> before or after. I saw a same pattern with unlocking order inside of
> __fscache_attr_changed in the failure case.

Following the enomem label, I'm calling fscache_unuse_cookie() which does this
without holding the lock in the same function.

I don't think the lock is required because:

 (1) We hold a ref on cookie->n_active so the cookie cannot go away until we
     release it, so calling __fscache_unuse_cookie() without the lock held
     should be fine.

 (2) wake_up_atomic_t() does not access cookie->n_active.  The address is
     merely needed as a key for the waiters to match on.

David

[PATCH 3/3] FS-Cache: refcount becomes corrupt under vma preasure.

[Milosz Tanski]

In rare cases under heavy VMA preasure the ref count for fscache cookies
becomes corrupt. In this case we decrement ref count even if fail before
incrememnt the refcount.

FS-Cache: Assertion failed bnode-eca5f9c6/syslog
0 > 0 is false
------------[ cut here ]------------
kernel BUG at fs/fscache/cookie.c:519!
invalid opcode: 0000 [#1] SMP
Call Trace:
[<ffffffffa01ba060>] __fscache_relinquish_cookie+0x50/0x220 [fscache]
[<ffffffffa02d64ce>] ceph_fscache_unregister_inode_cookie+0x3e/0x50 [ceph]
[<ffffffffa02ae1d3>] ceph_destroy_inode+0x33/0x200 [ceph]
[<ffffffff811cf67e>] ? __fsnotify_inode_delete+0xe/0x10
[<ffffffff811a9e0c>] destroy_inode+0x3c/0x70
[<ffffffff811a9f51>] evict+0x111/0x180
[<ffffffff811aa763>] iput+0x103/0x190
[<ffffffff811a5de8>] __dentry_kill+0x1c8/0x220
[<ffffffff811a5f31>] shrink_dentry_list+0xf1/0x250
[<ffffffff811a762c>] prune_dcache_sb+0x4c/0x60
[<ffffffff811930af>] super_cache_scan+0xff/0x170
[<ffffffff8113d7a0>] shrink_slab_node+0x140/0x2c0
[<ffffffff8113f2da>] shrink_slab+0x8a/0x130
[<ffffffff81142572>] balance_pgdat+0x3e2/0x5d0
[<ffffffff811428ca>] kswapd+0x16a/0x4a0
[<ffffffff810a43f0>] ? __wake_up_sync+0x20/0x20
[<ffffffff81142760>] ? balance_pgdat+0x5d0/0x5d0
[<ffffffff81083e09>] kthread+0xc9/0xe0
[<ffffffff81010000>] ? ftrace_raw_event_xen_mmu_release_ptpage+0x70/0x90
[<ffffffff81083d40>] ? flush_kthread_worker+0xb0/0xb0
[<ffffffff8159f63c>] ret_from_fork+0x7c/0xb0
[<ffffffff81083d40>] ? flush_kthread_worker+0xb0/0xb0
RIP [<ffffffffa01b984b>] __fscache_disable_cookie+0x1db/0x210 [fscache]
RSP <ffff8803bc85f9b8>
---[ end trace 254d0d7c74a01f25 ]---
---
 fs/fscache/page.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 84e11aa..c83dd89 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -195,7 +195,7 @@ int __fscache_attr_changed(struct fscache_cookie *cookie)
 {
 	struct fscache_operation *op;
 	struct fscache_object *object;
-	bool wake_cookie;
+	bool wake_cookie = false;
 
 	_enter("%p", cookie);
 
@@ -225,15 +225,16 @@ int __fscache_attr_changed(struct fscache_cookie *cookie)
 
 	__fscache_use_cookie(cookie);
 	if (fscache_submit_exclusive_op(object, op) < 0)
-		goto nobufs;
+		goto nobufs_dec;
 	spin_unlock(&cookie->lock);
 	fscache_stat(&fscache_n_attr_changed_ok);
 	fscache_put_operation(op);
 	_leave(" = 0");
 	return 0;
 
-nobufs:
+nobufs_dec:
 	wake_cookie = __fscache_unuse_cookie(cookie);
+nobufs:
 	spin_unlock(&cookie->lock);
 	kfree(op);
 	if (wake_cookie)
-- 
1.7.9.5

Re: [PATCH 3/3] FS-Cache: refcount becomes corrupt under vma preasure.

[David Howells]

Looks okay, but no S-o-b line.

David