lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <E1JTzBV-0001aO-R3@pomaz-ex.szeredi.hu>
Date:	Tue, 26 Feb 2008 13:45:25 +0100
From:	Miklos Szeredi <miklos@...redi.hu>
To:	a.p.zijlstra@...llo.nl
CC:	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	netdev@...r.kernel.org, trond.myklebust@....uio.no,
	a.p.zijlstra@...llo.nl, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 22/28] mm: add support for non block device backed swap files

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.

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

[snip]

> +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).

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

Signed-off-by: Miklos Szeredi <mszeredi@...e.cz>

Index: linux/mm/page_io.c
===================================================================
--- linux.orig/mm/page_io.c	2008-02-26 11:15:58.000000000 +0100
+++ linux/mm/page_io.c	2008-02-26 13:40:55.000000000 +0100
@@ -106,8 +106,10 @@ int swap_writepage(struct page *page, st
 	}
 
 	if (sis->flags & SWP_FILE) {
-		ret = sis->swap_file->f_mapping->
-			a_ops->swap_out(sis->swap_file, page, wbc);
+		struct file *swap_file = sis->swap_file;
+		struct address_space *mapping = swap_file->f_mapping;
+
+		ret = mapping->a_ops->swap_out(swap_file, page, wbc);
 		if (!ret)
 			count_vm_event(PSWPOUT);
 		return ret;
@@ -136,12 +138,13 @@ void swap_sync_page(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;
-		if (a_ops->sync_page)
-			a_ops->sync_page(page);
-	} else
+		struct address_space *mapping = sis->swap_file->f_mapping;
+
+		if (mapping->a_ops->sync_page)
+			mapping->a_ops->sync_page(page);
+	} else {
 		block_sync_page(page);
+	}
 }
 
 int swap_set_page_dirty(struct page *page)
@@ -149,17 +152,12 @@ int swap_set_page_dirty(struct page *pag
 	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
-		return (*spd)(page);
-	}
+		struct address_space *mapping = sis->swap_file->f_mapping;
 
-	return __set_page_dirty_nobuffers(page);
+		return mapping->a_ops->set_page_dirty(page);
+	} else {
+		return __set_page_dirty_nobuffers(page);
+	}
 }
 
 int swap_readpage(struct file *file, struct page *page)
@@ -172,8 +170,10 @@ int swap_readpage(struct file *file, str
 	BUG_ON(PageUptodate(page));
 
 	if (sis->flags & SWP_FILE) {
-		ret = sis->swap_file->f_mapping->
-			a_ops->swap_in(sis->swap_file, page);
+		struct file *swap_file = sis->swap_file;
+		struct address_space *mapping = swap_file->f_mapping;
+
+		ret = mapping->a_ops->swap_in(swap_file, page);
 		if (!ret)
 			count_vm_event(PSWPIN);
 		return ret;
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h	2008-02-26 11:15:58.000000000 +0100
+++ linux/include/linux/fs.h	2008-02-26 13:29:40.000000000 +0100
@@ -485,7 +485,8 @@ struct address_space_operations {
 	/*
 	 * swapfile support
 	 */
-	int (*swapfile)(struct address_space *, int);
+	int (*swapon)(struct file *file);
+	int (*swapoff)(struct file *file);
 	int (*swap_out)(struct file *file, struct page *page,
 			struct writeback_control *wbc);
 	int (*swap_in)(struct file *file, struct page *page);
Index: linux/mm/swapfile.c
===================================================================
--- linux.orig/mm/swapfile.c	2008-02-26 12:43:57.000000000 +0100
+++ linux/mm/swapfile.c	2008-02-26 13:34:57.000000000 +0100
@@ -1014,9 +1014,11 @@ static void destroy_swap_extents(struct 
 	}
 
 	if (sis->flags & SWP_FILE) {
+		struct file *swap_file = sis->swap_file;
+		struct address_space *mapping = swap_file->f_mapping;
+
 		sis->flags &= ~SWP_FILE;
-		sis->swap_file->f_mapping->a_ops->
-			swapfile(sis->swap_file->f_mapping, 0);
+		mapping->a_ops->swapoff(swap_file);
 	}
 }
 
@@ -1092,7 +1094,9 @@ add_swap_extent(struct swap_info_struct 
  */
 static int setup_swap_extents(struct swap_info_struct *sis, sector_t *span)
 {
-	struct inode *inode;
+	struct file *swap_file = sis->swap_file;
+	struct address_space *mapping = swap_file->f_mapping;
+	struct inode *inode = mapping->host;
 	unsigned blocks_per_page;
 	unsigned long page_no;
 	unsigned blkbits;
@@ -1103,16 +1107,14 @@ static int setup_swap_extents(struct swa
 	int nr_extents = 0;
 	int ret;
 
-	inode = sis->swap_file->f_mapping->host;
 	if (S_ISBLK(inode->i_mode)) {
 		ret = add_swap_extent(sis, 0, sis->max, 0);
 		*span = sis->pages;
 		goto done;
 	}
 
-	if (sis->swap_file->f_mapping->a_ops->swapfile) {
-		ret = sis->swap_file->f_mapping->a_ops->
-			swapfile(sis->swap_file->f_mapping, 1);
+	if (mapping->a_ops->swapon) {
+		ret = mapping->a_ops->swapon(swap_file);
 		if (!ret) {
 			sis->flags |= SWP_FILE;
 			ret = add_swap_extent(sis, 0, sis->max, 0);



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ