[PATCH] fscache: fix race between enablement and dropping of object

[PATCH] fscache: fix race between enablement and dropping of object

[NeilBrown]

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


It was observed that a process blocked indefintely in
__fscache_read_or_alloc_page(), waiting for FSCACHE_COOKIE_LOOKING_UP
to be cleared via fscache_wait_for_deferred_lookup().

At this time, ->backing_objects was empty, which would normaly prevent
__fscache_read_or_alloc_page() from getting to the point of waiting.
This implies that ->backing_objects was cleared *after*
__fscache_read_or_alloc_page was was entered.

When an object is "killed" and then "dropped",
FSCACHE_COOKIE_LOOKING_UP is cleared in fscache_lookup_failure(), then
KILL_OBJECT and DROP_OBJECT are "called" and only in DROP_OBJECT is
->backing_objects cleared.  This leaves a window where
something else can set FSCACHE_COOKIE_LOOKING_UP and
__fscache_read_or_alloc_page() can start waiting, before
->backing_objects is cleared

There is some uncertainty in this analysis, but it seems to be fit the
observations.  Adding the wake in this patch will be handled correctly
by __fscache_read_or_alloc_page(), as it checks if ->backing_objects
is empty again, after waiting.

Customer which reported the hang, also report that the hang cannot be
reproduced with this fix.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/fscache/object.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fscache/object.c b/fs/fscache/object.c
index 9edc920f651f..6d9cb1719de5 100644
--- a/fs/fscache/object.c
+++ b/fs/fscache/object.c
@@ -730,6 +730,9 @@ static const struct fscache_state *fscache_drop_object(struct fscache_object *ob
 
 	if (awaken)
 		wake_up_bit(&cookie->flags, FSCACHE_COOKIE_INVALIDATING);
+	if (test_and_clear_bit(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags))
+		wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
+
 
 	/* Prevent a race with our last child, which has to signal EV_CLEARED
 	 * before dropping our spinlock.
-- 
2.14.0.rc0.dirty


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

Re: [PATCH] fscache: fix race between enablement and dropping of object

[David Howells]

NeilBrown <neilb@suse.com> wrote:

> It was observed that a process blocked indefintely in
> __fscache_read_or_alloc_page(), waiting for FSCACHE_COOKIE_LOOKING_UP
> to be cleared via fscache_wait_for_deferred_lookup().

Was there a backtrace for that?

> This leaves a window where something else can set FSCACHE_COOKIE_LOOKING_UP

Attempting to enable the cookie again would have that effect.

> There is some uncertainty in this analysis, but it seems to be fit the
> observations.  Adding the wake in this patch will be handled correctly
> by __fscache_read_or_alloc_page(), as it checks if ->backing_objects
> is empty again, after waiting.

That should work.  The read op should just then fail with -ENOBUFS, causing
the netfs to issue the read over the network instead.

I think there's a more elegant solution to be had, but it'll take some thought
as to how to achieve it.

David

Re: [PATCH] fscache: fix race between enablement and dropping of object

[NeilBrown]

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

On Wed, Nov 28 2018, David Howells wrote:

> NeilBrown <neilb@suse.com> wrote:
>
>> It was observed that a process blocked indefintely in
>> __fscache_read_or_alloc_page(), waiting for FSCACHE_COOKIE_LOOKING_UP
>> to be cleared via fscache_wait_for_deferred_lookup().
>
> Was there a backtrace for that?

Yes.
PID: 29360  TASK: ffff881ff2ac0f80  CPU: 3   COMMAND: "zsh"
 #0 [ffff881ff43efbf8] schedule at ffffffff815e56f1
 #1 [ffff881ff43efc58] bit_wait at ffffffff815e64ed
 #2 [ffff881ff43efc68] __wait_on_bit at ffffffff815e61b8
 #3 [ffff881ff43efca0] out_of_line_wait_on_bit at ffffffff815e625e
 #4 [ffff881ff43efd08] fscache_wait_for_deferred_lookup at ffffffffa04f2e8f
[fscache]
 #5 [ffff881ff43efd18] __fscache_read_or_alloc_page at ffffffffa04f2ffe
[fscache]
 #6 [ffff881ff43efd58] __nfs_readpage_from_fscache at ffffffffa0679668 [nfs]
 #7 [ffff881ff43efd78] nfs_readpage at ffffffffa067092b [nfs]
 #8 [ffff881ff43efda0] generic_file_read_iter at ffffffff81187a73
 #9 [ffff881ff43efe50] nfs_file_read at ffffffffa066544b [nfs]
#10 [ffff881ff43efe70] __vfs_read at ffffffff811fc756
#11 [ffff881ff43efee8] vfs_read at ffffffff811fccfa
#12 [ffff881ff43eff18] sys_read at ffffffff811fda62
#13 [ffff881ff43eff50] entry_SYSCALL_64_fastpath at ffffffff815e986e

This is in SLES12-SP2 which is based on Linux-4.4

>
>> This leaves a window where something else can set FSCACHE_COOKIE_LOOKING_UP
>
> Attempting to enable the cookie again would have that effect.
>
>> There is some uncertainty in this analysis, but it seems to be fit the
>> observations.  Adding the wake in this patch will be handled correctly
>> by __fscache_read_or_alloc_page(), as it checks if ->backing_objects
>> is empty again, after waiting.
>
> That should work.  The read op should just then fail with -ENOBUFS, causing
> the netfs to issue the read over the network instead.
>
> I think there's a more elegant solution to be had, but it'll take some thought
> as to how to achieve it.

Please let me know when you come up with something.

Thanks,
NeilBrown

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