On Tue, 2008-02-26 at 13:45 +0100, Miklos Szeredi wrote:

Starting review in the middle, because this is the part I'm most
familiar with.

quoted

New addres_space_operations methods are added:
  int swapfile(struct address_space *, int);

Separate ->swapon() and ->swapoff() methods would be so much cleaner IMO.

I'm ok with that, but its a_ops bloat, do we care about that? I guess
since it has limited instances - typically one per filesystem - there is
no issue here.

Also is there a reason why 'struct file *' cannot be supplied to these
functions?

No real reason here. I guess its cleaner indeed. Thanks.

quoted

+int swap_set_page_dirty(struct page *page)
+{
+	struct swap_info_struct *sis = page_swap_info(page);
+
+	if (sis->flags & SWP_FILE) {
+		const struct address_space_operations *a_ops =
+			sis->swap_file->f_mapping->a_ops;
+		int (*spd)(struct page *) = a_ops->set_page_dirty;
+#ifdef CONFIG_BLOCK
+		if (!spd)
+			spd = __set_page_dirty_buffers;
+#endif

This ifdef is not really needed.  Just require ->set_page_dirty() be
filled in by filesystems which want swapfiles (and others too, in the
longer term, the fallback is just historical crud).

Agreed. This is a good motivation to clean up that stuff.

Here's an incremental patch addressing these issues and beautifying
the new code.

Thanks, I'll fold it into the patch and update the documentation. I'll
put your creds in akpm style.