linux-kernel - Re: [PATCH 4/4] vfs: Make sys_sync() use fsync

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]

Date:	Thu, 23 Apr 2009 12:57:36 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Jan Kara <jack@...e.cz>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@...radead.org>,
	Trond Myklebust <trond.myklebust@....uio.no>,
	linux-fsdevel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 4/4] vfs: Make sys_sync() use fsync_super() (version 2)

> +int __sync_blockdev(struct block_device *bdev, int wait)
> +{
> +	if (!bdev)
> +		return 0;
> +	if (!wait)
> +		return filemap_flush(bdev->bd_inode->i_mapping);
> +	return filemap_write_and_wait(bdev->bd_inode->i_mapping);
> +}

I wonder if the filemap_flush for the async case really buys us much,
all the async and then later sync writeback activities of the FS will
redirty lots of bits of the blockdev mapping that we then have to write
twice.

> @@ -284,7 +277,12 @@ static int __fsync_super(struct super_block *sb)
>   */
>  int fsync_super(struct super_block *sb)
>  {
> -	return __fsync_super(sb);
> +	int ret;
> +
> +	ret = __fsync_super(sb, 0);
> +	if (ret < 0)
> +		return ret;
> +	return __fsync_super(sb, 1);

This async first then wait approach does have some benefits when syncing
multiple filesystems, but I wonder if it isn't actually conta-productive
when syncing a single one.

Maybe this should be a separate patch ontop to allow for more
fine-grained benchmarking.

>  /*
> - * Call the ->sync_fs super_op against all filesystems which are r/w and
> - * which implement it.
> + * Sync all the data for all the filesystems (called by do_sync())

Your patch removes do_sync :)

>  static void do_sync_work(struct work_struct *work)
>  {
> -	do_sync(0);
> +	/*
> +	 * Sync twice to reduce the possibility we skipped some inodes / pages
> +	 * because they were temporarily locked
> +	 */
> +	sync_filesystems(0);
> +	sync_filesystems(0);
> +	printk("Emergency Sync complete\n");
>  	kfree(work);

Ah, nice.  Good to have this out of the sys_sync path.


The patch looks generally good but I'll need some heavy testing.  I'll
do some XFS testing with it ASAP.
--
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/