Re: [PATCH] per-backing dev unplugging #2

[Chris Mason <mason@suse.com>]

On Fri, 2004-03-12 at 15:51, Jens Axboe wrote:
> On Fri, Mar 12 2004, Chris Mason wrote:
> > On Fri, 2004-03-12 at 15:34, Jens Axboe wrote:
> > 
> > > 
> > > I don't see how this can make too much of a difference, aside of perhaps
> > > just moving the window a little. If page->mapping can disappear here,
> > > that's still a possibility.
> > 
> > As Andrew pointed out, the mapping struct won't disappear, but
> > page->mapping may go null.  So the idea is to use barriers to get a
> > trusted copy of page->mapping, and use the copy everywhere.
> 
> So trusting an atomic assignment of mapping = page->mapping, it should
> work. It feels a bit icky, though.

I reproduced on 2.6.4-mm1 + backing dev, but 2.6.4-mm1 alone ran fine. 
To make a long story short, the swap address space and backing dev don't
define an unplug_io_fn.  I was able to reproduce quickly with a swap
heavy workload.  The patch below should fix the oops, but probably isn't
correct solution since no queues will get unplugged while waiting on
swap pages.

It keeps the barriers for reading page->mapping, I still think we need
something there, maybe someone can suggest something better.

-chris

Index: linux.akpm/fs/buffer.c
===================================================================
--- linux.akpm.orig/fs/buffer.c	2004-03-14 15:15:37.409241104 -0500
+++ linux.akpm/fs/buffer.c	2004-03-14 15:15:51.137154144 -0500
@@ -2923,7 +2923,10 @@ EXPORT_SYMBOL(try_to_free_buffers);
 
 int block_sync_page(struct page *page)
 {
-	blk_run_address_space(page->mapping);
+	struct address_space *mapping;
+	smp_mb();
+	mapping = page->mapping;
+	blk_run_address_space(mapping);
 	return 0;
 }
 
Index: linux.akpm/mm/filemap.c
===================================================================
--- linux.akpm.orig/mm/filemap.c	2004-03-14 15:15:32.876930120 -0500
+++ linux.akpm/mm/filemap.c	2004-03-14 15:15:51.138153992 -0500
@@ -120,8 +120,10 @@ void remove_from_page_cache(struct page 
 
 static inline int sync_page(struct page *page)
 {
-	struct address_space *mapping = page->mapping;
-
+	struct address_space *mapping;
+	
+	smp_mb();
+	mapping = page->mapping;
 	if (mapping && mapping->a_ops && mapping->a_ops->sync_page)
 		return mapping->a_ops->sync_page(page);
 	return 0;
Index: linux.akpm/include/linux/blkdev.h
===================================================================
--- linux.akpm.orig/include/linux/blkdev.h	2004-03-14 15:15:37.460233352 -0500
+++ linux.akpm/include/linux/blkdev.h	2004-03-14 15:17:24.035031520 -0500
@@ -527,7 +527,7 @@ static inline request_queue_t *bdev_get_
 
 static inline void blk_run_backing_dev(struct backing_dev_info *bdi)
 {
-	if (bdi)
+	if (bdi && bdi->unplug_io_fn)
 		bdi->unplug_io_fn(bdi);
 }