linux-kernel - Re: [PATCH 02/10] writeback: switch to per-bdi threads for flushing data

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]

Message-ID: <20090831125840.GA20465@infradead.org>
Date:	Mon, 31 Aug 2009 08:58:40 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Jens Axboe <jens.axboe@...cle.com>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	chris.mason@...cle.com, david@...morbit.com, hch@...radead.org,
	akpm@...ux-foundation.org, jack@...e.cz
Subject: Re: [PATCH 02/10] writeback: switch to per-bdi threads for
	flushing data

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