Messages in this thread |  | | | Date | Tue, 10 Jan 2006 22:25:27 -0800 | | From | Andrew Morton <> | | Subject | Re: [PATCH 5/5] Direct Migration V9: Avoid writeback / page_migrate() method |
| |
Christoph Lameter <clameter@sgi.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |