FS-Cache: The retrieval remaining-pages counter needs to be atomic_t

struct fscache_retrieval contains a count of the number of pages that still
need some processing (n_pages).  This is decremented as the pages are
processed.

However, this needs to be atomic as fscache_retrieval_complete() (I think) just
occasionally may be called from cachefiles_read_backing_file() and
cachefiles_read_copier() simultaneously.

This happens when an fscache_read_or_alloc_pages() request containing a lot of
pages (say a couple of hundred) is being processed.  The read on each backing
page is dispatched individually because we need to insert a monitor into the
waitqueue to catch when the read completes.  However, under low-memory
conditions, we might be forced to wait in the allocator - and this gives the
I/O on the backing page a chance to complete first.

When the I/O completes, fscache_enqueue_retrieval() chucks the retrieval onto
the workqueue without waiting for the operation to finish the initial I/O
dispatch (we want to release any pages we can as soon as we can), thus both can
end up running simultaneously and potentially attempting to partially complete
the retrieval simultaneously (ENOMEM may occur, backing pages may already be in
the page cache).

This was demonstrated by parallelling the non-atomic counter with an atomic
counter and printing both of them when the assertion fails.  At this point, the
atomic counter has reached zero, but the non-atomic counter has not.

To fix this, make the counter an atomic_t.

This results in the following bug appearing

	FS-Cache: Assertion failed
	3 == 5 is false
	------------[ cut here ]------------
	kernel BUG at fs/fscache/operation.c:421!

or

	FS-Cache: Assertion failed
	3 == 5 is false
	------------[ cut here ]------------
	kernel BUG at fs/fscache/operation.c:414!

With a backtrace like the following:

RIP: 0010:[<ffffffffa0211b1d>] fscache_put_operation+0x1ad/0x240 [fscache]
Call Trace:
 [<ffffffffa0213185>] fscache_retrieval_work+0x55/0x270 [fscache]
 [<ffffffffa0213130>] ? fscache_retrieval_work+0x0/0x270 [fscache]
 [<ffffffff81090b10>] worker_thread+0x170/0x2a0
 [<ffffffff81096d10>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff810909a0>] ? worker_thread+0x0/0x2a0
 [<ffffffff81096966>] kthread+0x96/0xa0
 [<ffffffff8100c0ca>] child_rip+0xa/0x20
 [<ffffffff810968d0>] ? kthread+0x0/0xa0
 [<ffffffff8100c0c0>] ? child_rip+0x0/0x20

Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-and-tested-By: Milosz Tanski <milosz@adfin.com>
Acked-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/fscache/page.c b/fs/fscache/page.c
index 780bac6ffde52110452a92dc1e0351e2dde0565b..d479ab3c63e487ba097ff2b865c34401a9fcbfcb 100644 (file)
--- a/fs/fscache/page.c
+++ b/fs/fscache/page.c
@@ -235,7 +235,7 @@ static void fscache_release_retrieval_op(struct fscache_operation *_op)
 
        _enter("{OP%x}", op->op.debug_id);
 
-       ASSERTCMP(op->n_pages, ==, 0);
+       ASSERTCMP(atomic_read(&op->n_pages), ==, 0);
 
        fscache_hist(fscache_retrieval_histogram, op->start_time);
        if (op->context)
@@ -316,7 +316,7 @@ static void fscache_do_cancel_retrieval(struct fscache_operation *_op)
        struct fscache_retrieval *op =
                container_of(_op, struct fscache_retrieval, op);
 
-       op->n_pages = 0;
+       atomic_set(&op->n_pages, 0);
 }
 
 /*
@@ -406,7 +406,7 @@ int __fscache_read_or_alloc_page(struct fscache_cookie *cookie,
                _leave(" = -ENOMEM");
                return -ENOMEM;
        }
-       op->n_pages = 1;
+       atomic_set(&op->n_pages, 1);
 
        spin_lock(&cookie->lock);
 
@@ -533,7 +533,7 @@ int __fscache_read_or_alloc_pages(struct fscache_cookie *cookie,
        op = fscache_alloc_retrieval(cookie, mapping, end_io_func, context);
        if (!op)
                return -ENOMEM;
-       op->n_pages = *nr_pages;
+       atomic_set(&op->n_pages, *nr_pages);
 
        spin_lock(&cookie->lock);
 
@@ -643,7 +643,7 @@ int __fscache_alloc_page(struct fscache_cookie *cookie,
        op = fscache_alloc_retrieval(cookie, page->mapping, NULL, NULL);
        if (!op)
                return -ENOMEM;
-       op->n_pages = 1;
+       atomic_set(&op->n_pages, 1);
 
        spin_lock(&cookie->lock);
 

diff --git a/include/linux/fscache-cache.h b/include/linux/fscache-cache.h
index d32f70611a00bc2f368704f54b5d89a48b4461ec..a9ff9a36b86dc45c0f324fc2bacfc036f49fd0e3 100644 (file)
--- a/include/linux/fscache-cache.h
+++ b/include/linux/fscache-cache.h
@@ -151,7 +151,7 @@ struct fscache_retrieval {
        void                    *context;       /* netfs read context (pinned) */
        struct list_head        to_do;          /* list of things to be done by the backend */
        unsigned long           start_time;     /* time at which retrieval started */
-       unsigned                n_pages;        /* number of pages to be retrieved */
+       atomic_t                n_pages;        /* number of pages to be retrieved */
 };
 
 typedef int (*fscache_page_retrieval_func_t)(struct fscache_retrieval *op,
@@ -195,15 +195,14 @@ static inline void fscache_enqueue_retrieval(struct fscache_retrieval *op)
 static inline void fscache_retrieval_complete(struct fscache_retrieval *op,
                                              int n_pages)
 {
-       op->n_pages -= n_pages;
-       if (op->n_pages <= 0)
+       atomic_sub(n_pages, &op->n_pages);
+       if (atomic_read(&op->n_pages) <= 0)
                fscache_op_complete(&op->op, true);
 }
 
 /**
  * fscache_put_retrieval - Drop a reference to a retrieval operation
  * @op: The retrieval operation affected
- * @n_pages: The number of pages to account for
  *
  * Drop a reference to a retrieval operation.
  */