linux-kernel - Re: [PATCH] writeback: Don't wait for completion in writeback_inodes_sb

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:	Tue, 19 Jul 2011 12:51:49 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Jan Kara <jack@...e.cz>
Cc:	Christoph Hellwig <hch@...radead.org>,
	Curt Wohlgemuth <curtw@...gle.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	fengguang.wu@...el.com
Subject: Re: [PATCH] writeback: Don't wait for completion in
 writeback_inodes_sb_nr

On Wed, Jul 13, 2011 at 12:37:15AM +0200, Jan Kara wrote:
> -static int __sync_filesystem(struct super_block *sb, int wait)
> +static void __sync_filesystem(struct super_block *sb, int wait)
>  {
> -	/*
> -	 * This should be safe, as we require bdi backing to actually
> -	 * write out data in the first place
> -	 */
> -	if (sb->s_bdi == &noop_backing_dev_info)
> -		return 0;
> -
>  	if (sb->s_qcop && sb->s_qcop->quota_sync)
>  		sb->s_qcop->quota_sync(sb, -1, wait);
>  
> @@ -47,7 +40,6 @@ static int __sync_filesystem(struct super_block *sb, int wait)
>  
>  	if (sb->s_op->sync_fs)
>  		sb->s_op->sync_fs(sb, wait);
> -	return __sync_blockdev(sb->s_bdev, wait);

Removing this call breaks sys_syncfs and similar semantics on filesystem
just pushing metadata into buffers in ->write_inode or ->sync_fs and
then expecting the caller to write them out.  This list of filesystem
includes ext2 and in general most filesystems without journaling or
similar technics.

I'm perfectly fine with pushing the sync_blockdev call into the
filesystem for these, but we'll need a way to handle them.

> +	/*
> +	 * This should be safe, as we require bdi backing to actually
> +	 * write out data in the first place.
> +	 */
> +	if (sb->s_bdi == &noop_backing_dev_info)
> +		return 0;

There's really no reason to do that check early.  There's nothing
no reason to not have a filesystem that doesn't use the writeback
code, but still has a ->sync_fs method.  IMHO this check should
move into sync_inodes_sb/writeback_inodes_sb.

> +static void sync_all_bdevs(int wait)
> +{
> +	struct inode *inode, *old_inode = NULL;
> +
> +	spin_lock(&inode_sb_list_lock);
> +	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
> +		struct address_space *mapping = inode->i_mapping;
> +
> +		spin_lock(&inode->i_lock);
> +		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
> +		    mapping->nrpages == 0) {
> +			spin_unlock(&inode->i_lock);
> +			continue;
> +		}
> +		__iget(inode);
> +		spin_unlock(&inode->i_lock);
> +		spin_unlock(&inode_sb_list_lock);
> +		/*
> +		 * We hold a reference to 'inode' so it couldn't have been
> +		 * removed from s_inodes list while we dropped the
> +		 * inode_sb_list_lock.  We cannot iput the inode now as we can
> +		 * be holding the last reference and we cannot iput it under
> +		 * inode_sb_list_lock. So we keep the reference and iput it
> +		 * later.
> +		 */
> +		iput(old_inode);
> +		old_inode = inode;
> +
> +		__sync_blockdev(I_BDEV(inode), wait);
> +
> +		spin_lock(&inode_sb_list_lock);
> +	}
> +	spin_unlock(&inode_sb_list_lock);
> +	iput(old_inode);

At which point we could fold this code into a blkdev_sync_fs method for
now.  Long term we'll need to support multiple BDIs per SB anyway, at
which point the code can go away again.

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