[Linux-cachefs] [PATCH 1/1] [CacheFiles] Fix to handle Oops in cachefiles module during new object lookup while old object is being cleaned up

[David Howells]

KiranKumar Modukuri <kiran.modukuri at gmail.com> wrote:

> CacheFiles: Error: Overlong wait for old active object to go away.

Do you have the rest of the information it logs in this case?  It should have
logged a summary of the two objects:

	pr_err("Error: Overlong wait for old active object to go away\n");
	cachefiles_printk_object(object, xobject);    <------

> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -194,7 +194,7 @@ wait_for_old_object:
>                 pr_err("\n");
>                 pr_err("Error: Unexpected object collision\n");
>                 cachefiles_printk_object(object, xobject);
> -               BUG();
> +               WARN(true, "Unexpected object collision\n");

You didn't mention in your patch description that you'd hit this, so why are
you changing it?  If you did, do you have the object summaries printed by
cachefiles_printk_object()?

That said, it seems reasonable to either return an error at this point or wait
for the ACTIVE flag to be cleared on xobject.  Maybe only permit one passage
of "wait_for_old_object:", but since objects shouldn't get reactivated that
probably isn't necessary.

I think this bit:

	Remove the BUG() for the case where the old object is still being
	dropped and convert to WARN()

needs to be in its own patch since it's not obviously anything to do with the
subject at hand.

>         }
>         atomic_inc(&xobject->usage);
>         write_unlock(&cache->active_lock);
> @@ -247,6 +247,7 @@ wait_for_old_object:
> 
>         ASSERT(!test_bit(CACHEFILES_OBJECT_ACTIVE, &xobject->flags));
> 
> +       clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
>         cache->cache.ops->put_object(&xobject->fscache);
>         goto try_again;

This bit looks right.  It needs trace_cachefiles_mark_inactive() by it for
upstream consumption.

David