linux-kernel - Re: [PATCHv2 11/11] writeback: prevent unnecessary bdi threads wakeups

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100722031922.GA3446@amd>
Date:	Thu, 22 Jul 2010 13:19:22 +1000
From:	Nick Piggin <npiggin@...e.de>
To:	Artem Bityutskiy <dedekind1@...il.com>
Cc:	Jens Axboe <axboe@...nel.dk>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 11/11] writeback: prevent unnecessary bdi threads
 wakeups

On Wed, Jul 21, 2010 at 12:31:46PM +0300, Artem Bityutskiy wrote:
> @@ -973,22 +981,37 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>  		 * reposition it (that would break b_dirty time-ordering).
>  		 */
>  		if (!was_dirty) {
> -			struct bdi_writeback *wb = &inode_to_bdi(inode)->wb;
> -			struct backing_dev_info *bdi = wb->bdi;
> -
> -			if (bdi_cap_writeback_dirty(bdi) &&
> -			    !test_bit(BDI_registered, &bdi->state)) {
> -				WARN_ON(1);
> -				printk(KERN_ERR "bdi-%s not registered\n",
> -								bdi->name);
> +			bdi = inode_to_bdi(inode);
> +
> +			if (bdi_cap_writeback_dirty(bdi)) {
> +				WARN(!test_bit(BDI_registered, &bdi->state),
> +				     "bdi-%s not registered\n", bdi->name);
> +
> +				/*
> +				 * If this is the first dirty inode for this
> +				 * bdi, we have to wake-up the corresponding
> +				 * bdi thread to make sure background
> +				 * write-back happens later.
> +				 */
> +				if (!wb_has_dirty_io(&bdi->wb))
> +					wakeup_bdi = true;
>  			}
>  
>  			inode->dirtied_when = jiffies;
> -			list_move(&inode->i_list, &wb->b_dirty);
> +			list_move(&inode->i_list, &bdi->wb.b_dirty);
>  		}
>  	}
>  out:
>  	spin_unlock(&inode_lock);
> +
> +	if (wakeup_bdi) {
> +		spin_lock(&bdi->wb_lock);
> +		if (!bdi->wb.task)
> +			wake_up_process(default_backing_dev_info.wb.task);
> +		else
> +			wake_up_process(bdi->wb.task);
> +		spin_unlock(&bdi->wb_lock);
> +	}
>  }

We really want to wake up the bdi right away when first dirtying
the inode? I haven't looked at where the state of the bdi code is
now, but isn't it better to have a a delay there?

And rather than spreading details of how bdi tasks are managed
would you consider putting this into its own function?

Other than that, I like your patches. Out of interest, is 5 seconds
very detremental to power usage? What is a reasonable goal for
wakeups? (eg. 95%+ of possible efficiency)
--
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/