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

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

[Milosz Tanski]

This is meant to avoid a recusive 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

Signed-off-by: Milosz Tanski <milosz@adfin.com>
---
 fs/fscache/page.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 7f5c658..4911f2cf 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,11 @@ page_busy:
 	}
 
 	fscache_stat(&fscache_n_store_vmscan_wait);
-	__fscache_wait_on_page_write(cookie, page);
+	if (!relase_page_wait_timeout(cookie, page)) {
+		_debug("fscache writeout timeout page: %p{%lx}",
+			page, page->index);
+	}
+
 	gfp &= ~__GFP_WAIT;
 	goto try_again;
 }
-- 
1.7.9.5

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

I've been seeing issues with disposing cookies under vma pressure. The symptom
is that the refcount gets out of sync. In this case we fail to decrement the
refcount if submit fails. I found this while auditing the error in and around
cookie operations.

Signed-off-by: Milosz Tanski <milosz@adfin.com>
---
 fs/fscache/object.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index d3b4539..e1eb1f5 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -982,6 +982,8 @@ nomem:
 submit_op_failed:
 	clear_bit(FSCACHE_OBJECT_IS_LIVE, &object->flags);
 	spin_unlock(&cookie->lock);
+	if (__fscache_unuse_cookie(cookie))
+		__fscache_wake_unused_cookie(cookie);
 	kfree(op);
 	_leave(" [EIO]");
 	return transit_to(KILL_OBJECT);
-- 
1.7.9.5

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

[Milosz Tanski]

I've been seeing issues with disposing cookies under vma pressure. The symptom
is that the refcount gets out of sync. In this case we fail to decrement the
refcount if submit fails. I found this while auditing the error in and around
cookie operations.

Signed-off-by: Milosz Tanski <milosz@adfin.com>
---
 fs/fscache/object.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index d3b4539..e1eb1f5 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -982,6 +982,8 @@ nomem:
 submit_op_failed:
 	clear_bit(FSCACHE_OBJECT_IS_LIVE, &object->flags);
 	spin_unlock(&cookie->lock);
+	if (__fscache_unuse_cookie(cookie))
+		__fscache_wake_unused_cookie(cookie);
 	kfree(op);
 	_leave(" [EIO]");
 	return transit_to(KILL_OBJECT);
-- 
1.7.9.5

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

[Milosz Tanski]

In rare cases under heavy VMA pressure the ref count for a fscache cookie
becomes corrupt. In this case we decrement ref count even if we fail before
incrementing 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 ]---

Signed-off-by: Milosz Tanski <milosz@adfin.com>
---
 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 4911f2cf..fdfe996 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -199,7 +199,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);
 
@@ -229,15 +229,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

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

[Milosz Tanski]

In rare cases under heavy VMA pressure the ref count for a fscache cookie
becomes corrupt. In this case we decrement ref count even if we fail before
incrementing 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 ]---

Signed-off-by: Milosz Tanski <milosz@adfin.com>
---
 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 4911f2cf..fdfe996 100644
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -199,7 +199,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);
 
@@ -229,15 +229,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 2/3] FS-Cache: Reduce cookie ref count if submit fails.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On Wed, 13 Aug 2014 12:58:21 -0400 Milosz Tanski <milosz@adfin.com> wrote:

> I've been seeing issues with disposing cookies under vma pressure. The symptom
> is that the refcount gets out of sync. In this case we fail to decrement the
> refcount if submit fails. I found this while auditing the error in and around
> cookie operations.
> 
> Signed-off-by: Milosz Tanski <milosz@adfin.com>
> ---
>  fs/fscache/object.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index d3b4539..e1eb1f5 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -982,6 +982,8 @@ nomem:
>  submit_op_failed:
>  	clear_bit(FSCACHE_OBJECT_IS_LIVE, &object->flags);
>  	spin_unlock(&cookie->lock);
> +	if (__fscache_unuse_cookie(cookie))
> +		__fscache_wake_unused_cookie(cookie);
>  	kfree(op);
>  	_leave(" [EIO]");
>  	return transit_to(KILL_OBJECT);

Should this simple by
  +     fscache_unuse_cookie(cookie);

it does both the "unuse" and the "wake".

Otherwise they all look good to me.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]

On Wed, 13 Aug 2014 12:58:21 -0400 Milosz Tanski <milosz@adfin.com> wrote:

> I've been seeing issues with disposing cookies under vma pressure. The symptom
> is that the refcount gets out of sync. In this case we fail to decrement the
> refcount if submit fails. I found this while auditing the error in and around
> cookie operations.
> 
> Signed-off-by: Milosz Tanski <milosz@adfin.com>
> ---
>  fs/fscache/object.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index d3b4539..e1eb1f5 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -982,6 +982,8 @@ nomem:
>  submit_op_failed:
>  	clear_bit(FSCACHE_OBJECT_IS_LIVE, &object->flags);
>  	spin_unlock(&cookie->lock);
> +	if (__fscache_unuse_cookie(cookie))
> +		__fscache_wake_unused_cookie(cookie);
>  	kfree(op);
>  	_leave(" [EIO]");
>  	return transit_to(KILL_OBJECT);

Should this simple by
  +     fscache_unuse_cookie(cookie);

it does both the "unuse" and the "wake".

Otherwise they all look good to me.

NeilBrown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> + * since we might be called recusively via parent fs.

I've changed that to "recursively".

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

I've changed that to "release_page_wait_timeout()".

> +	if (!relase_page_wait_timeout(cookie, page)) {
> +		_debug("fscache writeout timeout page: %p{%lx}",
> +			page, page->index);
> +	}

And I've dropped the braces there.

David

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

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> + * since we might be called recusively via parent fs.

I've changed that to "recursively".

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

I've changed that to "release_page_wait_timeout()".

> +	if (!relase_page_wait_timeout(cookie, page)) {
> +		_debug("fscache writeout timeout page: %p{%lx}",
> +			page, page->index);
> +	}

And I've dropped the braces there.

David

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

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> +	if (__fscache_unuse_cookie(cookie))
> +		__fscache_wake_unused_cookie(cookie);

I've replaced these two lines with "fscache_unuse_cookie(cookie);" as
NeilBrown suggested.

David

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

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> +	if (__fscache_unuse_cookie(cookie))
> +		__fscache_wake_unused_cookie(cookie);

I've replaced these two lines with "fscache_unuse_cookie(cookie);" as
NeilBrown suggested.

David

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

[Milosz Tanski]

That's fine by me. If you'd like me to send a updated/fixed pull
request (much easier then patches over email) for these changes in my
code I can do that as well.

On Wed, Aug 27, 2014 at 10:31 AM, David Howells <dhowells@redhat.com> wrote:
> Milosz Tanski <milosz@adfin.com> wrote:
>
>> +     if (__fscache_unuse_cookie(cookie))
>> +             __fscache_wake_unused_cookie(cookie);
>
> I've replaced these two lines with "fscache_unuse_cookie(cookie);" as
> NeilBrown suggested.
>
> 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]

David, I know you're busy with other things. Would you like me to
re-spin the cleaned patches for you as a pull requests so you try to
get this upstream. I really want to see those fixes make it into
upstream.

- M

On Thu, Sep 4, 2014 at 12:00 PM, Milosz Tanski <milosz@adfin.com> wrote:
> That's fine by me. If you'd like me to send a updated/fixed pull
> request (much easier then patches over email) for these changes in my
> code I can do that as well.
>
> On Wed, Aug 27, 2014 at 10:31 AM, David Howells <dhowells@redhat.com> wrote:
>> Milosz Tanski <milosz@adfin.com> wrote:
>>
>>> +     if (__fscache_unuse_cookie(cookie))
>>> +             __fscache_wake_unused_cookie(cookie);
>>
>> I've replaced these two lines with "fscache_unuse_cookie(cookie);" as
>> NeilBrown suggested.
>>
>> 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:

> David, I know you're busy with other things. Would you like me to
> re-spin the cleaned patches for you as a pull requests so you try to
> get this upstream. I really want to see those fixes make it into
> upstream.

I've sent them Linuswards.

David

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

[David Howells]

Milosz Tanski <milosz@adfin.com> wrote:

> David, I know you're busy with other things. Would you like me to
> re-spin the cleaned patches for you as a pull requests so you try to
> get this upstream. I really want to see those fixes make it into
> upstream.

I've sent them Linuswards.

David

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

[Milosz Tanski]

On Wed, Sep 17, 2014 at 4:23 PM, David Howells <dhowells@redhat.com> wrote:
> Milosz Tanski <milosz@adfin.com> wrote:
>
>> David, I know you're busy with other things. Would you like me to
>> re-spin the cleaned patches for you as a pull requests so you try to
>> get this upstream. I really want to see those fixes make it into
>> upstream.
>
> I've sent them Linuswards.
>
> David

I saw that in my inbox earlier. Thank you for taking care of it.

-- 
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

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

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.

[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.

[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.

[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

[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