Re: [PATCH 2/7] ext3: call blkdev_issue_flush() on fsync()

[Theodore Tso]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/