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.
>
> > 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.
> > +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.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
