linux-kernel - Re: [PATCH 2/7] ext3: call blkdev_issue

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090330140427.GG13356@mit.edu>
Date:	Mon, 30 Mar 2009 10:04:27 -0400
From:	Theodore Tso <tytso@....edu>
To:	Fernando Luis Vázquez Cao 
	<fernando@....ntt.co.jp>
Cc:	Jeff Garzik <jeff@...zik.org>,
	Christoph Hellwig <hch@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Arjan van de Ven <arjan@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Nick Piggin <npiggin@...e.de>, David Rees <drees76@...il.com>,
	Jesper Krogh <jesper@...gh.cc>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	chris.mason@...cle.com, david@...morbit.com, tj@...nel.org
Subject: Re: [PATCH 2/7] ext3: call blkdev_issue_flush() on fsync()

On Mon, Mar 30, 2009 at 09:11:58PM +0900, Fernando Luis Vázquez Cao wrote:
> To ensure that bits are truly on-disk after an fsync or fdatasync, we
> should force a disk flush explicitly when there is dirty data/metadata
> and the journal didn't emit a write barrier (either because metadata is
> not being synched or barriers are disabled).

NACK.

As Eric commented on linux-ext4 (and I think it was Chris Mason
deserves the credit for originally pointing this out), we don't need
to call blkdev_issue_flush() after calling sync_inode().  That's
because sync_inode() eventually (after going through a very deep call
tree inside fs/fs-writeback.c) __sync_single_inode(), which calls
write_inode(), which calls the filesystem-specific ->write_inode()
function, which for both ext3 and ext4, ends up calling
ext[34]_force_commit.  Which, if barriers are enabled, will end up
issuing a barrier after writing the commit block.

In the code paths that don't end up calling sync_inode() or
ext4_force_commit(), (i.e., in the fdatasync() case) calling
block_flush_device is appropriate.  But as it stands, this patch (and
the related one for ext4) will result in multiple unnecessary barrier
requests being sent to the block layer.

So two out of the three places where this patch adds
block_flush_device() are not necessary; as far as I can tell, only
this one is one we should add.

> -	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
> -		goto out;
> +	if (datasync && !(i_state & I_DIRTY_DATASYNC)) {
> +		if (i_state & I_DIRTY_PAGES)
> +			ret = block_flush_device(inode->i_sb->s_bdev);
> +		return ret;
> +	}

A similar fixup is needed for the ext4 patch.

(And can we please start a new thread for these patches?  Thanks!!)

Regards,

     	    	   	       	      	  	- Ted

--
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/