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

[Andrew Morton]

Christoph Lameter <clameter@xxxxxxx> wrote:
>
> +int buffer_migrate_page(struct page *newpage, struct page *page)
>  +{
>  +	struct address_space *mapping = page->mapping;
>  +	struct buffer_head *bh, *head;
>  +
>  +	if (!mapping)
>  +		return -EAGAIN;
>  +
>  +	if (!page_has_buffers(page))
>  +		return migrate_page(newpage, page);
>  +
>  +	head = page_buffers(page);
>  +
>  +	if (migrate_page_remove_references(newpage, page, 3))
>  +		return -EAGAIN;
>  +
>  +	spin_lock(&mapping->private_lock);

Why are you taking ->private_lock here?

address_space.private_lock protects the list of buffers at
address_space.private_list.  For a regular file (or directory or long
symlink..) that list contains buffers against the blockdev mapping (a
different address_space) which need to be synced for a successful fsync of
this file.  ie: dirty metadata for this file.

So we have two situations:

a) page->mapping->host refers to a regular file/dir/etc

   Here, mapping->private_list holds potentially-dirty buffers against
   the blockdev mapping (a different address_space).

   Nothing needs to be done.

b) page->mapping->host refers to a blockdev (/dev/hda1's pages)

   Here, mapping->private_list is actually always empty.

   Nothing needs to be done.


   BUT, page_buffers(page) refers to buffers which might be on some
   other address_space's ->private_list.  Because a blockdev may have dirty
   buffers which some other address_space needs to write out for its sync.

   blockdevmapping->private_lock is the correct lock for these buffers. 
   Each regular file has a copy of blockdevmapping in its ->assoc_mapping,
   so all files end up taking the same lock when manipulating their
   ->private_list.

   As long as you've taken a ref on the blockdev mapping's buffers and
   locked them then nobody will be starting I/O against them or fiddling
   with ->b_page while you do the swizzle (I think).

AFAIK nobody ever used address_space.private_list for anything apart from
the associated buffers, but that's just a btw.

Anyway, ->private_lock is purely for protecting the thing at
->private_list, so I suspect this locking is simply unneeded.

Please explain the reasoning behind taking this lock.  In fact, that should
have been commented, in the spirit of buffer.c's glorious overcommenting,
which I'm sure you enjoyed ;)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/