rewrite rd

Hi,

This is my proposal for a (hopefully) backwards compatible rd driver. The motivation for me is not pressing, except that I have this code sitting here that is either going to rot or get merged. I'm happy to make myself maintainer of this code, but if any real block device driver writer would like to, that would be fine by me ;)

Comments?

--

This is a rewrite of the ramdisk block device driver.

The old one is really difficult because it effectively implements a block device which serves data out of its own buffer cache. It relies on the dirty bit being set, to pin its backing store in cache, however there are non trivial paths which can clear the dirty bit (eg. try_to_free_buffers()), which had recently lead to data corruption. And in general it is completely wrong for a block device driver to do this.

The new one is more like a regular block device driver. It has no idea about vm/vfs stuff. It's backing store is similar to the buffer cache (a simple radix-tree of pages), but it doesn't know anything about page cache (the pages in the radix tree are not pagecache pages).

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

The fact that it now goes through all the regular vm/fs paths makes it much more useful for testing, too.

   text    data     bss     dec     hex filename
   2837     849     384    4070     fe6 drivers/block/rd.o
   3528     371      12    3911     f47 drivers/block/brd.o

Text is larger, but data and bss are smaller, making total size smaller.

A few other nice things about it:
- Similar structure and layout to the new loop device handlinag.
- Dynamic ramdisk creation.
- Runtime flexible buffer head size (because it is no longer part of the ramdisk code).
- Boot / load time flexible ramdisk size, which could easily be extended to a per-ramdisk runtime changeable size (eg. with an ioctl).

+++ linux-2.6/drivers/block/brd.c

drivers/block/brd.c new file +14 -0

@@ -0,0 +1,536 @@
+/*
+ * Ram backed block device driver.
+ *
+ * Copyright (C) 2007 Nick Piggin
+ * Copyright (C) 2007 Novell Inc.
+ *
+ * Parts derived from drivers/block/rd.c, and drivers/block/loop.c, copyright
+ * of their respective owners.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/major.h>

drivers/block/brd.c new file +25 -0

+#define PAGE_SECTORS_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
+#define PAGE_SECTORS		(1 << PAGE_SECTORS_SHIFT)
+
+/*
+ * Each block ramdisk device has a radix_tree brd_pages of pages that stores
+ * the pages containing the block device's contents. A brd page's ->index is
+ * its offset in PAGE_SIZE units. This is similar to, but in no way connected
+ * with, the kernel's pagecache or buffer cache (which sit above our block
+ * device).
+ */
+struct brd_device {
+	int		brd_number;
+	int		brd_refcnt;
+	loff_t		brd_offset;
+	loff_t		brd_sizelimit;
+	unsigned	brd_blocksize;
+
+	struct request_queue	*brd_queue;
+	struct gendisk		*brd_disk;
+	struct list_head	brd_list;
+
+	/*
+	 * Backing store of pages and lock to protect it. This is the contents
+	 * of the block device.
+	 */

drivers/block/brd.c new file +28 -0

+	struct page *page;
+
+	/*
+	 * The page lifetime is protected by the fact that we have opened the
+	 * device node -- brd pages will never be deleted under us, so we
+	 * don't need any further locking or refcounting.
+	 *
+	 * This is strictly true for the radix-tree nodes as well (ie. we
+	 * don't actually need the rcu_read_lock()), however that is not a
+	 * documented feature of the radix-tree API so it is better to be
+	 * safe here (we don't have total exclusion from radix tree updates
+	 * here, only deletes).
+	 */
+	rcu_read_lock();
+	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
+	page = radix_tree_lookup(&brd->brd_pages, idx);
+	rcu_read_unlock();
+
+	BUG_ON(page && page->index != idx);
+
+	return page;
+}
+
+/*
+ * Look up and return a brd's page for a given sector.
+ * If one does not exist, allocate an empty page, and insert that. Then
+ * return it.
+ */

drivers/block/brd.c new file +45 -0

+	if (!page)
+		return NULL;
+
+	if (radix_tree_preload(GFP_NOIO)) {
+		__free_page(page);
+		return NULL;
+	}
+
+	spin_lock(&brd->brd_lock);
+	idx = sector >> PAGE_SECTORS_SHIFT;
+	if (radix_tree_insert(&brd->brd_pages, idx, page)) {
+		__free_page(page);
+		page = radix_tree_lookup(&brd->brd_pages, idx);
+		BUG_ON(!page);
+		BUG_ON(page->index != idx);
+	} else
+		page->index = idx;
+	spin_unlock(&brd->brd_lock);
+
+	radix_tree_preload_end();
+
+	return page;
+}
+
+/*
+ * Free all backing store pages and radix tree. This must only be called when
+ * there are no other users of the device.
+ */
+#define FREE_BATCH 16
+static void brd_free_pages(struct brd_device *brd)
+{
+	unsigned long pos = 0;
+	struct page *pages[FREE_BATCH];
+	int nr_pages;
+
+	do {
+		int i;
+
+		nr_pages = radix_tree_gang_lookup(&brd->brd_pages,
+				(void **)pages, pos, FREE_BATCH);
+
+		for (i = 0; i < nr_pages; i++) {
+			void *ret;
+
+			BUG_ON(pages[i]->index < pos);

drivers/block/brd.c new file +12 -0

+	if (!brd_insert_page(brd, sector))
+		return -ENOMEM;
+	if (copy < n) {
+		sector += copy >> SECTOR_SHIFT;
+		if (!brd_insert_page(brd, sector))
+			return -ENOMEM;
+	}
+	return 0;
+}
+
+/*
+ * Copy n bytes from src to the brd starting at sector. Does not sleep.

drivers/block/brd.c new file +15 -0

+	if (copy < n) {
+		src += copy;
+		sector += copy >> SECTOR_SHIFT;
+		copy = n - copy;
+		page = brd_lookup_page(brd, sector);
+		BUG_ON(!page);
+
+		dst = kmap_atomic(page, KM_USER1);
+		memcpy(dst, src, copy);
+		kunmap_atomic(dst, KM_USER1);
+	}
+}
+
+/*
+ * Copy n bytes to dst from the brd starting at sector. Does not sleep.

drivers/block/brd.c new file +32 -0

+	page = brd_lookup_page(brd, sector);
+	if (page) {
+		src = kmap_atomic(page, KM_USER1);
+		memcpy(dst, src + offset, copy);
+		kunmap_atomic(src, KM_USER1);
+	} else
+		memset(dst, 0, copy);
+
+	if (copy < n) {
+		dst += copy;
+		sector += copy >> SECTOR_SHIFT;
+		copy = n - copy;
+		page = brd_lookup_page(brd, sector);
+		if (page) {
+			src = kmap_atomic(page, KM_USER1);
+			memcpy(dst, src, copy);
+			kunmap_atomic(src, KM_USER1);
+		} else
+			memset(dst, 0, copy);
+	}
+}
+
+/*
+ * Process a single bvec of a bio.
+ */
+static int brd_do_bvec(struct brd_device *brd, struct page *page,
+			unsigned int len, unsigned int off, int rw,
+			sector_t sector)
+{
+	void *mem;
+	int err = 0;
+

drivers/block/brd.c new file +21 -0

+	} else
+		copy_to_brd(brd, mem + off, sector, len);
+	kunmap_atomic(mem, KM_USER0);
+
+out:
+	return err;
+}
+
 
...
 
+static int brd_ioctl(struct inode *inode, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	int error;
+	struct block_device *bdev = inode->i_bdev;
+	struct brd_device *brd = bdev->bd_disk->private_data;
+
+	if (cmd != BLKFLSBUF)
+		return -ENOTTY;
+
+	/*
+	 * ram device BLKFLSBUF has special semantics, we want to actually
+	 * release and destroy the ramdisk data.

drivers/block/brd.c new file +8 -0

+		brd_free_pages(brd);
+		error = 0;
+	}
+	mutex_unlock(&bdev->bd_mutex);
+
+	return error;
+}
+
 
...

Quoted 8 lines · On Tue, 4 Dec 2007 05:26:28 +0100 Nick Piggin <[email protected]> wrote:

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

what about mmap(/dev/ram0)?

Quoted 9 lines

Text is larger, but data and bss are smaller, making total size smaller.

A few other nice things about it:
- Similar structure and layout to the new loop device handlinag.
- Dynamic ramdisk creation.
- Runtime flexible buffer head size (because it is no longer part of the ramdisk code).
- Boot / load time flexible ramdisk size, which could easily be extended to a per-ramdisk runtime changeable size (eg. with an ioctl).

This ramdisk driver can use highmem whereas the existing one can't (yes?). That's a pretty major difference. Plus look at the revoltingness in rd.c's mapping_set_gfp_mask()s.

Quoted 1 line

+#define SECTOR_SHIFT 9

That's our third definition of SECTOR_SHIFT.

drivers/block/brd.c line 59

Quoted 6 lines

drivers/block/brd.c new file +6 -0

+/*
+ * Look up and return a brd's page for a given sector.
+ */
+static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
+{
+	unsigned long idx;

Could use pgoff_t here if you think that's clearer.

drivers/block/brd.c line 97

Quoted 9 lines

drivers/block/brd.c new file +9 -0

+{
+	unsigned long idx;
+	struct page *page;
+
+	page = brd_lookup_page(brd, sector);
+	if (page)
+		return page;
+
+	page = alloc_page(GFP_NOIO | __GFP_HIGHMEM | __GFP_ZERO);

Why GFP_NOIO?

Have you thought about __GFP_MOVABLE treatment here? Seems pretty desirable as this sucker can be large.

Quoted 22 lines

drivers/block/brd.c new file +22 -0

+static void brd_free_pages(struct brd_device *brd)
+{
+	unsigned long pos = 0;
+	struct page *pages[FREE_BATCH];
+	int nr_pages;
+
+	do {
+		int i;
+
+		nr_pages = radix_tree_gang_lookup(&brd->brd_pages,
+				(void **)pages, pos, FREE_BATCH);
+
+		for (i = 0; i < nr_pages; i++) {
+			void *ret;
+
+			BUG_ON(pages[i]->index < pos);
+			pos = pages[i]->index;
+			ret = radix_tree_delete(&brd->brd_pages, pos);
+			BUG_ON(!ret || ret != pages[i]);
+			__free_page(pages[i]);
+		}
+

drivers/block/brd.c line 152

Quoted 4 lines

drivers/block/brd.c new file +4 -0

+		pos++;
+
+	} while (nr_pages == FREE_BATCH);
+}

I have vague memories that radix_tree_gang_lookup()'s "naive" implementation may return fewer items than you asked for even when there are more items remaining - when it hits certain boundaries.

drivers/block/brd.c line 162

Quoted 9 lines

drivers/block/brd.c new file +9 -0

+/*
+ * copy_to_brd_setup must be called before copy_to_brd. It may sleep.
+ */
+static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n)
+{
+	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
+	size_t copy;
+
+	copy = min((unsigned long)n, PAGE_SIZE - offset);

use min_t. Or, better, sort out the types.

drivers/block/brd.c line 184

Quoted 9 lines

drivers/block/brd.c new file +9 -0

+static void copy_to_brd(struct brd_device *brd, const void *src,
+			sector_t sector, size_t n)
+{
+	struct page *page;
+	void *dst;
+	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
+	size_t copy;
+
+	copy = min((unsigned long)n, PAGE_SIZE - offset);

Ditto.

drivers/block/brd.c line 190

Quoted 6 lines

drivers/block/brd.c new file +6 -0

+	page = brd_lookup_page(brd, sector);
+	BUG_ON(!page);
+
+	dst = kmap_atomic(page, KM_USER1);
+	memcpy(dst + offset, src, copy);
+	kunmap_atomic(dst, KM_USER1);

Might need flush_dcache_page() if mmap gets sorted out.

Quoted 11 lines

drivers/block/brd.c new file +11 -0

+static int brd_do_bvec(struct brd_device *brd, struct page *page,
+			unsigned int len, unsigned int off, int rw,
+			sector_t sector)
+{
+	void *mem;
+	int err = 0;
+
+	if (rw != READ) {
+		err = copy_to_brd_setup(brd, sector, len);
+		if (err)
+			goto out;

drivers/block/brd.c line 258

Quoted 6 lines

drivers/block/brd.c new file +6 -0

+	}
+
+	mem = kmap_atomic(page, KM_USER0);
+	if (rw == READ) {
+		copy_from_brd(mem + off, brd, sector, len);
+		flush_dcache_page(page);

hm, there's a flush_dcache_page(). I guess you've throught it through ;)

Quoted 4 lines

drivers/block/brd.c new file +4 -0

+	mutex_lock(&bdev->bd_mutex);
+	error = -EBUSY;
+	if (bdev->bd_openers <= 1) {
+		/*

drivers/block/brd.c line 323

Quoted 5 lines

drivers/block/brd.c new file +5 -0

+		 * Invalidate the cache first, so it isn't written
+		 * back to the device.
+		 */
+		invalidate_bh_lrus();
+		truncate_inode_pages(bdev->bd_inode->i_mapping, 0);

hm, some other thread can instantiate pagecache here. I guess it's always been like that and there's not a lot we can (or should) do about it.

Quoted 14 lines

drivers/block/brd.c new file +8 -0

+		brd_free_pages(brd);
+		error = 0;
+	}
+	mutex_unlock(&bdev->bd_mutex);
+
+	return error;
+}
+
 
...

fs/buffer.c void invalidate_bh_lrus(void) · line 1439

Quoted 4 lines

fs/buffer.c +1 -0

@@ -1436,6 +1436,7 @@ void invalidate_bh_lrus(void)
 {
 	on_each_cpu(invalidate_bh_lru, NULL, 1, 1);
 }
+EXPORT_SYMBOL_GPL(invalidate_bh_lrus);

Maybe create a new helper function which does invalidate_bh_lrus()+truncate_inode_pages(), call that from kill_bdev() and here, make invalidate_bh_lrus() static.

That's a separate patch, I guess.

Thanks for the review, I'll post an incremental patch in a sec.

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

Quoted 7 lines · On Monday 03 December 2007 22:26:28 Nick Piggin wrote:

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

For the embedded world, initramfs has pretty much rendered initrd obsolete, and that was the biggest user of the ramdisk code I know of. Beyond that, loopback mounts give you more flexible transient block devices than ramdisks do. (In fact, ramdisks are such an amazing pain to use/size/free that if I really needed something like that I'd just make a loopback mount in a ramfs instance.)

Embedded users who still want a block interface for memory are generally trying to use a cramfs or squashfs image out of ROM or flash, although there are flash-specific filesystems for this and I dunno if they're actually mounting /dev/mem at an offset or something (md? losetup -o? Beats me, I haven't tried that myself yet...)

Quoted 9 lines · Rob Landley

Quoted 7 lines · On Monday 03 December 2007 22:26:28 Nick Piggin wrote:

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

Quoted 6 lines

For the embedded world, initramfs has pretty much rendered initrd obsolete, and that was the biggest user of the ramdisk code I know of. Beyond that, loopback mounts give you more flexible transient block devices than ramdisks do. (In fact, ramdisks are such an amazing pain to use/size/free that if I really needed something like that I'd just make a loopback mount in a ramfs instance.)

They are, although we could easily hook up a couple more ioctls for them now (if anybody is interested).

From a usability perspective, an ioctl is no substitute for being able to

The rd driver can potentially be a _lot_ more scalable than the loop driver. It's completely lockless in the fastpath and doesn't even dirty any cachelines except for the actual destination memory. OK, this is only really important for testing purposes (eg. testing scalability of a filesystem etc.) But it is one reason why I (personally) want brd...

Quoted 5 lines

Embedded users who still want a block interface for memory are generally trying to use a cramfs or squashfs image out of ROM or flash, although there are flash-specific filesystems for this and I dunno if they're actually mounting /dev/mem at an offset or something (md? losetup -o? Beats me, I haven't tried that myself yet...)

OK, it would be interesting to hear from anyone using rd for things like cramfs.

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

Am Dienstag, 4. Dezember 2007 schrieb Nick Piggin:
[...]

Quoted 7 lines

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

This is just an idea, I dont know if it is worth the trouble, but have you though about implementing direct_access for brd? That would allow execute-in-place (xip) on brd eliminating the extra copy.

Quoted 4 lines

This is just an idea, I dont know if it is worth the trouble, but have you though about implementing direct_access for brd? That would allow execute-in-place (xip) on brd eliminating the extra copy.

Actually that's a pretty good idea. It would allow xip to be tested without special hardware as well...

I'll see what the patch looks like. Thanks

Am I missing something here? I wonder how s390 works without this change?

--
ext2 should not worry about checking sb->s_blocksize for XIP before the sb's blocksize actually gets set.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/fs/ext2/super.c
===================================================================

Quoted 16 lines · Nick Piggin

Am I missing something here? I wonder how s390 works without this change?

--
ext2 should not worry about checking sb->s_blocksize for XIP before the sb's blocksize actually gets set.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/fs/ext2/super.c
===================================================================

fs/ext2/super.c +0 -0

@@ -844,8 +844,7 @@ static int ext2_fill_super(struct super_
 
 	blocksize = BLOCK_SIZE << le32_to_cpu(sbi->s_es->s_log_block_size);
 

fs/ext2/super.c static int ext2_fill_super(struct super_ · line 850

Quoted 6 lines

fs/ext2/super.c +1 -2

-	if ((ext2_use_xip(sb)) && ((blocksize != PAGE_SIZE) ||
-				  (sb->s_blocksize != blocksize))) {
+	if (ext2_use_xip(sb) && blocksize != PAGE_SIZE) {
 		if (!silent)
 			printk("XIP: Unsupported blocksize\n");
 		goto failed_mount;

"blocksize" contains the blocksize of the device here, and sb->s_blocksize does contain the filesystem block size as saved in the super block.

Xip does only work, if both do match PAGE_SIZE because it does'nt support multiple calls to direct_access in the get_xip_page address space operation. Thus we check both here, actually this was changed from how it looks after your patch as a bugfix where our tester tried a 4k filesystem on a 2k blockdev.
Did I miss something?

Quoted 2 lines

Xip does only work, if both do match PAGE_SIZE because it does'nt support multiple calls to direct_access in the get_xip_page address space operation. Thus we check both here, actually this was changed from how it looks after your patch as a bugfix where our tester tried a 4k filesystem on a 2k blockdev. Did I miss something?

However, the bdev block size may be changed with sb_set_blocksize. It doesn't actually have to match the hardware sector size -- if this does matter for XIP, then I think you need some other check here.

Quoted 7 lines · Nick Piggin

Quoted 6 lines

Xip does only work, if both do match PAGE_SIZE because it does'nt support multiple calls to direct_access in the get_xip_page address space operation. Thus we check both here, actually this was changed from how it looks after your patch as a bugfix where our tester tried a 4k filesystem on a 2k blockdev.
Did I miss something?

Quoted 3 lines

However, the bdev block size may be changed with sb_set_blocksize. It doesn't actually have to match the hardware sector size -- if this does matter for XIP, then I think you need some other check here.

Hmmmmhh. For a bdev with PAGE_SIZE hardsect size, there is no other valid value then PAGE_SIZE that one could set it to. Or can it indeed be changed to a value greater then PAGE_SIZE or smaller then hardsect size?

After my patch, we can do XIP in a hardsect size < PAGE_SIZE block device -- this seems to be a fine thing to do at least for the ramdisk code. Would this situation be problematic for existing drivers, and if so, in what way?

Quoted 4 lines · Nick Piggin

After my patch, we can do XIP in a hardsect size < PAGE_SIZE block device -- this seems to be a fine thing to do at least for the ramdisk code. Would this situation be problematic for existing drivers, and if so, in what way?

I have done some archeology, and our ancient CVS logs show this check was introduced in early 2005 into our 2.6.x. codebase. However, it existed way before, and was copied from our prehistorical ext2 split named xip2 back in the old days of 2.4.x where we did not really have a block device behind because that one was scamped into the file system in a very queer way.

After all, I don't see any risk in removing the check. The only driver we have that does direct_access is drivers/s390/block/dcssblk.c, and that one only supports block_size == PAGE_SIZE. I think the patch should go into mainline.

Acked-by Carsten Otte <[email protected]>

OK, thanks for taking a look at that. It will be helpful for testing XIP with my new ramdisk driver (did you see the patch?).

Actually another one's recently sprung up too (arch/powerpc/sysdev/axonram.c) but it looks like that one should be fine as it looks to be all simply memory mapped.

Quoted 2 lines · Nick Piggin

OK, thanks for taking a look at that. It will be helpful for testing XIP with my new ramdisk driver (did you see the patch?).

I have'nt looked at it yet. I do appreciate it, I think it might broaden the user-base of this feature which is up to now s390 only due to the fact that the flash memory extensions have not been implemented (yet?). And it enables testing xip on other platforms. The patch is on my must-read list.

Quoted 5 lines

I have'nt looked at it yet. I do appreciate it, I think it might broaden the user-base of this feature which is up to now s390 only due to the fact that the flash memory extensions have not been implemented (yet?). And it enables testing xip on other platforms. The patch is on my must-read list.

query: which feature is currently s390 only? (Execute In Place?)

Quoted 7 lines

Quoted 5 lines

I have'nt looked at it yet. I do appreciate it, I think it might broaden the user-base of this feature which is up to now s390 only due to the fact that the flash memory extensions have not been implemented (yet?). And it enables testing xip on other platforms. The patch is on my must-read list.

query: which feature is currently s390 only? (Execute In Place?)

I think so. The filemap_xip.c functionality doesn't work for Flash memory yet. Flash memory doesn't have struct pages to back it up with which this stuff depends on.

Quoted 11 lines · On Thursday 06 December 2007 21:22:25 Jared Hulbert wrote:

Quoted 7 lines

Quoted 5 lines

I have'nt looked at it yet. I do appreciate it, I think it might broaden the user-base of this feature which is up to now s390 only due to the fact that the flash memory extensions have not been implemented (yet?). And it enables testing xip on other platforms. The patch is on my must-read list.

query: which feature is currently s390 only? (Execute In Place?)

I think so. The filemap_xip.c functionality doesn't work for Flash memory yet. Flash memory doesn't have struct pages to back it up with which this stuff depends on.

Um, trying to clarify: S390. Also known as zSeries, big iron machine, uses its own weird processor design rather than x86, x86-64, arm, or mips processors.

How does "struct page" enter into this?

What I want to know is, are you saying execute in place doesn't work on things like arm and mips? (I so, I was unaware of this. I heard about somebody getting it to work on a Nintendo DS:
http://forums.maxconsole.net/showthread.php?t=18668 )

Um, trying to clarify: S390. Also known as zSeries, big iron machine, uses its own weird processor design rather than x86, x86-64, arm, or mips processors.

How does "struct page" enter into this?

What I want to know is, are you saying execute in place doesn't work on things like arm and mips? (I so, I was unaware of this. I heard about somebody getting it to work on a Nintendo DS:
http://forums.maxconsole.net/showthread.php?t=18668 )

I think so. The filemap_xip.c functionality doesn't work for Flash memory yet. Flash memory doesn't have struct pages to back it up with which this stuff depends on.

Quoted 3 lines

I think so. The filemap_xip.c functionality doesn't work for Flash memory yet. Flash memory doesn't have struct pages to back it up with which this stuff depends on.

Struct page is not the major issue. The primary problem is writing to the media (and I am not a flash expert at all, just relaying here):
For some period of time, the flash memory is not usable and thus we need to make sure we can nuke the page table entries that we have in userland page tables. For that, we need a callback from the device so that it can ask to get its references back. Oh, and a put_xip_page counterpart to get_xip_page, so that the driver knows when it's safe to erase.

drivers/block/brd.c static struct page *brd_insert_page(stru · line 110

Quoted 10 lines · On Tue, 4 Dec 2007 12:21:00 +0100 Nick Piggin <[email protected]> wrote:

drivers/block/brd.c +9 -0

+	 *
+	 * Cannot support XIP and highmem, because our ->direct_access
+	 * routine for XIP must return memory that is always addressable.
+	 * If XIP was reworked to use pfns and kmap throughout, this
+	 * restriction might be able to be lifted.
 	 */
+	gfp_flags = GFP_NOIO | __GFP_ZERO;
+#ifndef CONFIG_BLK_DEV_XIP
+	gfp_flags |= __GFP_HIGHMEM;
+#endif

A dubious tradeoff?

Quoted 14 lines · Andrew Morton

Review

Quoted 10 lines · On Tue, 4 Dec 2007 12:21:00 +0100 Nick Piggin <[email protected]> wrote:

(quoted diff) +9 -0

+	 *
+	 * Cannot support XIP and highmem, because our ->direct_access
+	 * routine for XIP must return memory that is always addressable.
+	 * If XIP was reworked to use pfns and kmap throughout, this
+	 * restriction might be able to be lifted.
 	 */
+	gfp_flags = GFP_NOIO | __GFP_ZERO;
+#ifndef CONFIG_BLK_DEV_XIP
+	gfp_flags |= __GFP_HIGHMEM;
+#endif

A dubious tradeoff?

On big highmem machines certainly. It may be somewhat useful on small memory systems... but having the config option there is nice for a VM developer without an s390 easily available ;)

But don't apply these XIP patches yet -- after a bit more testing I'm seeing some data corruption, so I'll have to work out what's going wrong with that first.

Here we go. See, brd already found a bug ;)
Can you apply the ext2 XIP patch too? And I'll resend the brd XIP patch.

Writing to XIP files at a non-page-aligned offset results in data corruption because the writes were always sent to the start of the page.

Hi Nick,

Quoted 2 lines

Here we go. See, brd already found a bug ;)
Can you apply the ext2 XIP patch too? And I'll resend the brd XIP patch.

[...]

Quoted 2 lines

Writing to XIP files at a non-page-aligned offset results in data corruption because the writes were always sent to the start of the page.

[...]

mm/filemap_xip.c __xip_file_write(struct file *filp, cons · line 319

Quoted 8 lines

mm/filemap_xip.c +1 -1

@@ -314,7 +314,7 @@ __xip_file_write(struct file *filp, cons
 		fault_in_pages_readable(buf, bytes);
 		kaddr = kmap_atomic(page, KM_USER0);
 		copied = bytes -
-			__copy_from_user_inatomic_nocache(kaddr, buf, bytes);
+			__copy_from_user_inatomic_nocache(kaddr + offset, buf, bytes);
 		kunmap_atomic(kaddr, KM_USER0);
 		flush_dcache_page(page);

I asked myself why this problem never happened before. So I asked our testers to reproduce this problem on 2.6.23 and service levels. As the testcase did not trigger, I looked into the 2.6.23 code. This problem was introduced by commit 4a9e5ef1f4f15205e477817a5cefc34bd3f65f55 (mm: write iovec cleanup from Nick Piggin) during 2.6.24-rc:
--------snip-------

(quoted diff) +2 -2

-		copied = filemap_copy_from_user(page, offset, buf, bytes);
[...]
+		copied = bytes -
+			__copy_from_user_inatomic_nocache(kaddr, buf, bytes);
-------------------

So yes, its good to have xip on brd. It even tests your changes ;-) Good news is, that we dont need anything for stable.

Review

Quoted 5 lines · Nick Piggin

(quoted diff) +4 -0

+       gfp_flags = GFP_NOIO | __GFP_ZERO;
+#ifndef CONFIG_BLK_DEV_XIP
+       gfp_flags |= __GFP_HIGHMEM;
+#endif
        page = alloc_page(GFP_NOIO | __GFP_HIGHMEM | __GFP_ZERO);

I think that should be alloc_page(gfp_flags), no?

drivers/block/brd.c new file +5 -0

+static void copy_to_brd(struct brd_device *brd, const void *src,
+			sector_t sector, size_t n)
+{
+	struct page *page;
+	void *dst;

Quoted 11 lines · Nick Piggin

drivers/block/brd.c new file +11 -0

+static void copy_to_brd(struct brd_device *brd, const void *src,
+			sector_t sector, size_t n)
+{
+	struct page *page;
+	void *dst;
+	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
+	size_t copy;
+
+	copy = min((unsigned long)n, PAGE_SIZE - offset);
+	page = brd_lookup_page(brd, sector);
+	BUG_ON(!page);

drivers/block/brd.c line 190

Quoted 4 lines

drivers/block/brd.c new file +4 -0

+
+	dst = kmap_atomic(page, KM_USER1);
+	memcpy(dst + offset, src, copy);
+	kunmap_atomic(dst, KM_USER1);

You're using kmap_atomic, but I see no reason you can't be preempted. Don't you need to at least disable preemption while you have stuff atomically kmapped?