Re: [PATCH] writeback: Don't wait for completion in writeback_inodes_sb_nr

[Christoph Hellwig <hch@infradead.org>]

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.