Re: [PATCH 5/5] Direct Migration V9: Avoid writeback / page_migrate() method

[Andrew Morton <akpm@osdl.org>]

Christoph Lameter <clameter@engr.sgi.com> wrote:
>
> On Tue, 10 Jan 2006, Andrew Morton wrote:
> 
> > Christoph Lameter <clameter@sgi.com> wrote:
> > >
> > > +	spin_lock(&mapping->private_lock);
> > >  +
> > >  +	bh = head;
> > >  +	do {
> > >  +		get_bh(bh);
> > >  +		lock_buffer(bh);
> > >  +		bh = bh->b_this_page;
> > >  +
> > >  +	} while (bh != head);
> > >  +
> > 
> > Guys, lock_buffer() sleeps and cannot be called inside spinlock.
> 
> I took it the way it was in the hotplug patches.We are taking the 
> spinlock here to protect the scan over the list of bh's of this page 
> right?
> 
> Is it not sufficient to have the page locked to guarantee that the list of 
> buffers is not changed? Seems that ext3 does that (see 
> ext3_ordered_writepage() etc).

Yes, the page lock protects the buffer ring.

> like this?
> 
> Index: linux-2.6.15/fs/buffer.c
> ===================================================================
> --- linux-2.6.15.orig/fs/buffer.c	2006-01-10 22:13:37.000000000 -0800
> +++ linux-2.6.15/fs/buffer.c	2006-01-10 22:37:28.000000000 -0800
> @@ -3070,8 +3070,6 @@ int buffer_migrate_page(struct page *new
>  	if (migrate_page_remove_references(newpage, page, 3))
>  		return -EAGAIN;
>  
> -	spin_lock(&mapping->private_lock);
> -
>  	bh = head;
>  	do {
>  		get_bh(bh);
> @@ -3094,11 +3092,9 @@ int buffer_migrate_page(struct page *new
>  	} while (bh != head);
>  
>  	SetPagePrivate(newpage);
> -	spin_unlock(&mapping->private_lock);
>  
>  	migrate_page_copy(newpage, page);
>  
> -	spin_lock(&mapping->private_lock);
>  	bh = head;
>  	do {
>  		unlock_buffer(bh);
> @@ -3106,7 +3102,6 @@ int buffer_migrate_page(struct page *new
>  		bh = bh->b_this_page;
>  
>  	} while (bh != head);
> -	spin_unlock(&mapping->private_lock);
>  
>  	return 0;
>  }

Seems right, I think.


So let's see.  Suppose the kernel is about to dink with a page's buffer
ring.  It does:

	get_page(page);
	lock_page(page);
	dink_with(page_buffers(page));

how do these patches ensure that the page doesn't get migrated under my
feet?