linux-kernel - Re: [Patch 15/18] fs/logfs/super.c

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: <200706101827.50948.arnd@arndb.de>
Date:	Sun, 10 Jun 2007 18:27:49 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Jörn Engel <joern@...ybastard.org>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mtd@...ts.infradead.org, akpm@...l.org,
	Sam Ravnborg <sam@...nborg.org>,
	John Stoffel <john@...ffel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Jamie Lokier <jamie@...reable.org>,
	Artem Bityutskiy <dedekind@...radead.org>,
	CaT <cat@....com.au>, Jan Engelhardt <jengelh@...ux01.gwdg.de>,
	Evgeniy Polyakov <johnpol@....mipt.ru>,
	David Weinehall <tao@....umu.se>, Willy Tarreau <w@....eu>,
	Kyle Moffett <mrmacman_g4@....com>,
	Dongjun Shin <djshin90@...il.com>, Pavel Machek <pavel@....cz>,
	Bill Davidsen <davidsen@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Albert Cahalan <acahalan@...il.com>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Roland Dreier <rdreier@...co.com>,
	Ondrej Zajicek <santiago@...reenet.org>,
	Ulisses Furquim <ulissesf@...il.com>
Subject: Re: [Patch 15/18] fs/logfs/super.c

On Sunday 03 June 2007, Jörn Engel wrote:
> +static int mtdread(struct super_block *sb, loff_t ofs, size_t len, void *buf)
> +{
> +	struct mtd_info *mtd = logfs_super(sb)->s_mtd;
> +	size_t retlen;
> +	int ret;
> +
> +	ret = mtd->read(mtd, ofs, len, &retlen, buf);
> +	BUG_ON(ret == -EINVAL);
> +	if (ret)
> +		return ret;
> +
> +	/* Not sure if we should loop instead. */
> +	if (retlen != len)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int mtdwrite(struct super_block *sb, loff_t ofs, size_t len, void *buf)
> +{
> +	struct logfs_super *super = logfs_super(sb);
> +	struct mtd_info *mtd = super->s_mtd;
> +	struct inode *inode = super->s_dev_inode;
> +	size_t retlen;
> +	loff_t page_start, page_end;
> +	int ret;
> +
> +	if (super->s_flags & LOGFS_SB_FLAG_RO)
> +		return -EROFS;
> +
> +	BUG_ON((ofs >= mtd->size) || (len > mtd->size - ofs));
> +	BUG_ON(ofs != (ofs >> super->s_writeshift) << super->s_writeshift);
> +	BUG_ON(len > PAGE_CACHE_SIZE);
> +	page_start = ofs & PAGE_CACHE_MASK;
> +	page_end = PAGE_CACHE_ALIGN(ofs + len) - 1;
> +	truncate_inode_pages_range(&inode->i_data, page_start, page_end);
> +	ret = mtd->write(mtd, ofs, len, &retlen, buf);
> +	if (ret || (retlen != len))
> +		return -EIO;
> +
> +	return 0;
> +}

It seems that these functions are completely synchronous and afaics, the
writes are called in the pdflush context, effectively blocking out operations
on other file systems at the time. Not sure if this is something that
should be fixed, but it seems to limit scalability on mtd backends.

It seems that jffs2 has the same behaviour.

> +static int bdwrite(struct super_block *sb, loff_t to, size_t len, void *buf)
> +{
> +	struct block_device *bdev = logfs_super(sb)->s_bdev;
> +	struct address_space *mapping = bdev->bd_inode->i_mapping;
> +	struct page *page;
> +	long index = to >> PAGE_SHIFT;
> +	long offset = to & (PAGE_SIZE-1);
> +	long copylen;
> +
> +	while (len) {
> +		copylen = min((ulong)len, PAGE_SIZE - offset);
> +
> +		page = read_cache_page(mapping, index,
> +				(filler_t*)mapping->a_ops->readpage, NULL);
> +		if (!page)
> +			return -ENOMEM;
> +		if (IS_ERR(page))
> +			return PTR_ERR(page);
> +		lock_page(page);
> +		memcpy(page_address(page) + offset, buf, copylen);
> +		set_page_dirty(page);
> +		unlock_page(page);
> +		page_cache_release(page);
> +
> +		buf += copylen;
> +		len -= copylen;
> +		offset = 0;
> +		index++;
> +	}
> +	return 0;
> +}

How about using submit_bio here instead of going to the page cache?
That would avoid doubling all the memory consumption here.

> +/*
> + * logfs_crash_dump - dump debug information to device
> + *
> + * The LogFS superblock only occupies part of a segment.  This function will
> + * write as much debug information as it can gather into the spare space.
> + */
> +void logfs_crash_dump(struct super_block *sb)
> +{
> +	struct logfs_super *super = logfs_super(sb);
> +	int i, blockno = 2, bs = sb->s_blocksize;
> +	void *scratch = super->s_wblock[0];
> +	void *stack = (void *) ((ulong)current & ~0x1fffUL);
> +
> +	/* all wbufs */
> +	for (i=0; i<LOGFS_NO_AREAS; i++) {
> +		void *wbuf = super->s_area[i]->a_wbuf;
> +		u64 ofs = sb->s_blocksize + i*super->s_writesize;
> +		mtdwrite(sb, ofs, super->s_writesize, wbuf);
> +	}

shouldn't this use the ->write() function instead of hardcoding mtdwrite.

> +module_init(logfs_init);
> +module_exit(logfs_exit);

You are missing at least a MODULE_LICENSE and should probably
add the MODULE_AUTHOR and MODULE_DESCRIPTION tags as well.
Right now, it's impossible to even load the module, because
it uses a few GPL-only symbols.

	Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/