Skip to content

Commit c4d6d8d

Browse files
dhowellsdhowells
committed
CacheFiles: Fix the marking of cached pages
Under some circumstances CacheFiles defers the marking of pages with PG_fscache so that it can take advantage of pagevecs to reduce the number of calls to fscache_mark_pages_cached() and the netfs's hook to keep track of this. There are, however, two problems with this: (1) It can lead to the PG_fscache mark being applied _after_ the page is set PG_uptodate and unlocked (by the call to fscache_end_io()). (2) CacheFiles's ref on the page is dropped immediately following fscache_end_io() - and so may not still be held when the mark is applied. This can lead to the page being passed back to the allocator before the mark is applied. Fix this by, where appropriate, marking the page before calling fscache_end_io() and releasing the page. This means that we can't take advantage of pagevecs and have to make a separate call for each page to the marking routines. The symptoms of this are Bad Page state errors cropping up under memory pressure, for example: BUG: Bad page state in process tar pfn:002da page:ffffea0000009fb0 count:0 mapcount:0 mapping: (null) index:0x1447 page flags: 0x1000(private_2) Pid: 4574, comm: tar Tainted: G W 3.1.0-rc4-fsdevel+ #1064 Call Trace: [<ffffffff8109583c>] ? dump_page+0xb9/0xbe [<ffffffff81095916>] bad_page+0xd5/0xea [<ffffffff81095d82>] get_page_from_freelist+0x35b/0x46a [<ffffffff810961f3>] __alloc_pages_nodemask+0x362/0x662 [<ffffffff810989da>] __do_page_cache_readahead+0x13a/0x267 [<ffffffff81098942>] ? __do_page_cache_readahead+0xa2/0x267 [<ffffffff81098d7b>] ra_submit+0x1c/0x20 [<ffffffff8109900a>] ondemand_readahead+0x28b/0x29a [<ffffffff81098ee2>] ? ondemand_readahead+0x163/0x29a [<ffffffff810990ce>] page_cache_sync_readahead+0x38/0x3a [<ffffffff81091d8a>] generic_file_aio_read+0x2ab/0x67e [<ffffffffa008cfbe>] nfs_file_read+0xa4/0xc9 [nfs] [<ffffffff810c22c4>] do_sync_read+0xba/0xfa [<ffffffff81177a47>] ? security_file_permission+0x7b/0x84 [<ffffffff810c25dd>] ? rw_verify_area+0xab/0xc8 [<ffffffff810c29a4>] vfs_read+0xaa/0x13a [<ffffffff810c2a79>] sys_read+0x45/0x6c [<ffffffff813ac37b>] system_call_fastpath+0x16/0x1b As can be seen, PG_private_2 (== PG_fscache) is set in the page flags. Instrumenting fscache_mark_pages_cached() to verify whether page->mapping was set appropriately showed that sometimes it wasn't. This led to the discovery that sometimes the page has apparently been reclaimed by the time the marker got to see it. Reported-by: M. Stevens <m@tippett.com> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Jeff Layton <jlayton@redhat.com>
1 parent 1800098 commit c4d6d8d

File tree

4 files changed

+56
-52
lines changed

4 files changed

+56
-52
lines changed

‎fs/cachefiles/rdwr.c

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,8 @@ static void cachefiles_read_copier(struct fscache_operation *_op)
176176
recheck:
177177
if (PageUptodate(monitor->back_page)) {
178178
copy_highpage(monitor->netfs_page, monitor->back_page);
179-
180-
pagevec_add(&pagevec, monitor->netfs_page);
181-
fscache_mark_pages_cached(monitor->op, &pagevec);
179+
fscache_mark_page_cached(monitor->op,
180+
monitor->netfs_page);
182181
error = 0;
183182
} else if (!PageError(monitor->back_page)) {
184183
/* the page has probably been truncated */
@@ -335,8 +334,7 @@ static int cachefiles_read_backing_file_one(struct cachefiles_object *object,
335334
backing_page_already_uptodate:
336335
_debug("- uptodate");
337336

338-
pagevec_add(pagevec, netpage);
339-
fscache_mark_pages_cached(op, pagevec);
337+
fscache_mark_page_cached(op, netpage);
340338

341339
copy_highpage(netpage, backpage);
342340
fscache_end_io(op, netpage, 0);
@@ -448,8 +446,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
448446
&pagevec);
449447
} else if (cachefiles_has_space(cache, 0, 1) == 0) {
450448
/* there's space in the cache we can use */
451-
pagevec_add(&pagevec, page);
452-
fscache_mark_pages_cached(op, &pagevec);
449+
fscache_mark_page_cached(op, page);
453450
ret = -ENODATA;
454451
} else {
455452
ret = -ENOBUFS;
@@ -465,8 +462,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
465462
*/
466463
static int cachefiles_read_backing_file(struct cachefiles_object *object,
467464
struct fscache_retrieval *op,
468-
struct list_head *list,
469-
struct pagevec *mark_pvec)
465+
struct list_head *list)
470466
{
471467
struct cachefiles_one_read *monitor = NULL;
472468
struct address_space *bmapping = object->backer->d_inode->i_mapping;
@@ -626,13 +622,13 @@ static int cachefiles_read_backing_file(struct cachefiles_object *object,
626622
page_cache_release(backpage);
627623
backpage = NULL;
628624

629-
if (!pagevec_add(mark_pvec, netpage))
630-
fscache_mark_pages_cached(op, mark_pvec);
625+
fscache_mark_page_cached(op, netpage);
631626

632627
page_cache_get(netpage);
633628
if (!pagevec_add(&lru_pvec, netpage))
634629
__pagevec_lru_add_file(&lru_pvec);
635630

631+
/* the netpage is unlocked and marked up to date here */
636632
fscache_end_io(op, netpage, 0);
637633
page_cache_release(netpage);
638634
netpage = NULL;
@@ -775,15 +771,11 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
775771
/* submit the apparently valid pages to the backing fs to be read from
776772
* disk */
777773
if (nrbackpages > 0) {
778-
ret2 = cachefiles_read_backing_file(object, op, &backpages,
779-
&pagevec);
774+
ret2 = cachefiles_read_backing_file(object, op, &backpages);
780775
if (ret2 == -ENOMEM || ret2 == -EINTR)
781776
ret = ret2;
782777
}
783778

784-
if (pagevec_count(&pagevec) > 0)
785-
fscache_mark_pages_cached(op, &pagevec);
786-
787779
_leave(" = %d [nr=%u%s]",
788780
ret, *nr_pages, list_empty(pages) ? " empty" : "");
789781
return ret;
@@ -806,7 +798,6 @@ int cachefiles_allocate_page(struct fscache_retrieval *op,
806798
{
807799
struct cachefiles_object *object;
808800
struct cachefiles_cache *cache;
809-
struct pagevec pagevec;
810801
int ret;
811802

812803
object = container_of(op->op.object,
@@ -817,13 +808,10 @@ int cachefiles_allocate_page(struct fscache_retrieval *op,
817808
_enter("%p,{%lx},", object, page->index);
818809

819810
ret = cachefiles_has_space(cache, 0, 1);
820-
if (ret == 0) {
821-
pagevec_init(&pagevec, 0);
822-
pagevec_add(&pagevec, page);
823-
fscache_mark_pages_cached(op, &pagevec);
824-
} else {
811+
if (ret == 0)
812+
fscache_mark_page_cached(op, page);
813+
else
825814
ret = -ENOBUFS;
826-
}
827815

828816
_leave(" = %d", ret);
829817
return ret;

‎fs/fscache/page.c

Lines changed: 36 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -914,6 +914,40 @@ void __fscache_uncache_page(struct fscache_cookie *cookie, struct page *page)
914914
}
915915
EXPORT_SYMBOL(__fscache_uncache_page);
916916

917+
/**
918+
* fscache_mark_page_cached - Mark a page as being cached
919+
* @op: The retrieval op pages are being marked for
920+
* @page: The page to be marked
921+
*
922+
* Mark a netfs page as being cached. After this is called, the netfs
923+
* must call fscache_uncache_page() to remove the mark.
924+
*/
925+
void fscache_mark_page_cached(struct fscache_retrieval *op, struct page *page)
926+
{
927+
struct fscache_cookie *cookie = op->op.object->cookie;
928+
929+
#ifdef CONFIG_FSCACHE_STATS
930+
atomic_inc(&fscache_n_marks);
931+
#endif
932+
933+
_debug("- mark %p{%lx}", page, page->index);
934+
if (TestSetPageFsCache(page)) {
935+
static bool once_only;
936+
if (!once_only) {
937+
once_only = true;
938+
printk(KERN_WARNING "FS-Cache:"
939+
" Cookie type %s marked page %lx"
940+
" multiple times\n",
941+
cookie->def->name, page->index);
942+
}
943+
}
944+
945+
if (cookie->def->mark_page_cached)
946+
cookie->def->mark_page_cached(cookie->netfs_data,
947+
op->mapping, page);
948+
}
949+
EXPORT_SYMBOL(fscache_mark_page_cached);
950+
917951
/**
918952
* fscache_mark_pages_cached - Mark pages as being cached
919953
* @op: The retrieval op pages are being marked for
@@ -925,32 +959,11 @@ EXPORT_SYMBOL(__fscache_uncache_page);
925959
void fscache_mark_pages_cached(struct fscache_retrieval *op,
926960
struct pagevec *pagevec)
927961
{
928-
struct fscache_cookie *cookie = op->op.object->cookie;
929962
unsigned long loop;
930963

931-
#ifdef CONFIG_FSCACHE_STATS
932-
atomic_add(pagevec->nr, &fscache_n_marks);
933-
#endif
934-
935-
for (loop = 0; loop < pagevec->nr; loop++) {
936-
struct page *page = pagevec->pages[loop];
937-
938-
_debug("- mark %p{%lx}", page, page->index);
939-
if (TestSetPageFsCache(page)) {
940-
static bool once_only;
941-
if (!once_only) {
942-
once_only = true;
943-
printk(KERN_WARNING "FS-Cache:"
944-
" Cookie type %s marked page %lx"
945-
" multiple times\n",
946-
cookie->def->name, page->index);
947-
}
948-
}
949-
}
964+
for (loop = 0; loop < pagevec->nr; loop++)
965+
fscache_mark_page_cached(op, pagevec->pages[loop]);
950966

951-
if (cookie->def->mark_pages_cached)
952-
cookie->def->mark_pages_cached(cookie->netfs_data,
953-
op->mapping, pagevec);
954967
pagevec_reinit(pagevec);
955968
}
956969
EXPORT_SYMBOL(fscache_mark_pages_cached);

‎include/linux/fscache-cache.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,9 @@ extern void fscache_withdraw_cache(struct fscache_cache *cache);
504504

505505
extern void fscache_io_error(struct fscache_cache *cache);
506506

507+
extern void fscache_mark_page_cached(struct fscache_retrieval *op,
508+
struct page *page);
509+
507510
extern void fscache_mark_pages_cached(struct fscache_retrieval *op,
508511
struct pagevec *pagevec);
509512

‎include/linux/fscache.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,14 @@ struct fscache_cookie_def {
135135
*/
136136
void (*put_context)(void *cookie_netfs_data, void *context);
137137

138-
/* indicate pages that now have cache metadata retained
139-
* - this function should mark the specified pages as now being cached
140-
* - the pages will have been marked with PG_fscache before this is
138+
/* indicate page that now have cache metadata retained
139+
* - this function should mark the specified page as now being cached
140+
* - the page will have been marked with PG_fscache before this is
141141
* called, so this is optional
142142
*/
143-
void (*mark_pages_cached)(void *cookie_netfs_data,
144-
struct address_space *mapping,
145-
struct pagevec *cached_pvec);
143+
void (*mark_page_cached)(void *cookie_netfs_data,
144+
struct address_space *mapping,
145+
struct page *page);
146146

147147
/* indicate the cookie is no longer cached
148148
* - this function is called when the backing store currently caching

0 commit comments

Comments
 (0)