Re: [PATCH 02/10] writeback: switch to per-bdi threads forflushing data

[Christoph Hellwig]

On Mon, Aug 31, 2009 at 02:14:43PM +0200, Jens Axboe wrote:
> -static void generic_sync_bdi_inodes(struct backing_dev_info *bdi,
> -				    struct writeback_control *wbc,
> -				    struct super_block *sb,
> -				    int is_blkdev_sb)
> +void generic_sync_bdi_inodes(struct super_block *sb,
> +			     struct writeback_control *wbc)

I think we're better off having the sb also in the writeback control.
Now that the inodes actually hang off the backing device it's just
another parameter to limit the amount of writeback done.

> +		/*
> +		 * If this fs is currently being u/remounted, leave the
> +		 * inode alone
> +		 */
> +		if (!down_read_trylock(&inode->i_sb->s_umount)) {
> +			requeue_io(inode);
> +			continue;
> +		}

This looks correct to me, but I wonder if the increased traffic on
s_umount will hurt us in some way for the writeback of lots of small
files.


>  void generic_sync_sb_inodes(struct super_block *sb,
>  				struct writeback_control *wbc)
>  {
> -	const int is_blkdev_sb = sb_is_blkdev_sb(sb);
> -	struct backing_dev_info *bdi;
> -
> -	mutex_lock(&bdi_lock);
> -	list_for_each_entry(bdi, &bdi_list, bdi_list)
> -		generic_sync_bdi_inodes(bdi, wbc, sb, is_blkdev_sb);
> -	mutex_unlock(&bdi_lock);
> +	if (wbc->bdi)
> +		generic_sync_bdi_inodes(sb, wbc);
> +	else
> +		bdi_writeback_all(sb, wbc);

With the sb in writeback_control this gem would also be gone.


Btw, some ordering in the patch series seems odd, e.g. you have 
most of the high level flushing code above generic_sync_wb_inodes
which makes reading fs-writeback.c rather inconvenient.  And also
leads to having two forward declarations for generic_sync_wb_inodes
beeing added inside the file.
--
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/