linux-kernel - Re: [PATCH -stable] block: destroy bdi before blockdev is unregistered.

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]

Date:	Thu, 30 Apr 2015 10:06:31 +1000
From:	NeilBrown <neilb@...e.de>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Christoph Hellwig <hch@....de>, Mike Snitzer <snitzer@...hat.com>,
	Jens Axboe <axboe@...com>, Azat Khuzhin <a3at.mail@...il.com>,
	"Kernel.org-Linux-RAID" <linux-raid@...r.kernel.org>,
	Guoqing Jiang <GQJiang@...e.com>, Tejun Heo <tj@...nel.org>,
	Jan Kara <jack@...e.cz>, lkml <linux-kernel@...r.kernel.org>,
	device-mapper development <dm-devel@...hat.com>
Subject: Re: [PATCH -stable] block: destroy bdi before blockdev is
 unregistered.

On Wed, 29 Apr 2015 18:02:58 +0200 Peter Zijlstra <peterz@...radead.org>
wrote:

> On Wed, Apr 29, 2015 at 03:35:12PM +0200, Christoph Hellwig wrote:
> > On Wed, Apr 29, 2015 at 07:25:30AM +1000, NeilBrown wrote:
> > > As bdi_set_min_ratio doesn't touch bdi->dev, there seems to be no need for
> > > the test, or the warning.
> > > 
> > > I wonder if it would make sense to move the bdi_set_min_ratio() call to
> > > bdi_destroy, and discard bdi_unregister??
> > > There is a comment which suggests bdi_unregister might be of use later, but
> > > it might be best to have a clean slate in which to add whatever might be
> > > needed??
> > 
> > This seems fine to me from the block dev point of view.  I don't really
> > understand the bdi_min_ratio logic, but Peter might have a better idea.
> 
> Ah, that was a bit of digging, I've not looked at that in ages :-)
> 
> So if you look at bdi_dirty_limit()'s comment:
> 
>  * The bdi's share of dirty limit will be adapting to its throughput and
>  * bounded by the bdi->min_ratio and/or bdi->max_ratio parameters, if set.
> 
> So the min_ratio is a minimum guaranteed fraction of the total
> throughput.
> 
> Now the problem before commit ccb6108f5b0b ("mm/backing-dev.c: reset bdi
> min_ratio in bdi_unregister()") was that since bdi_set_min_ratio()
> keeps a global sum of bdi->min_ratio, you need to subtract from said
> global sum when taking the BDI away. Otherwise we loose/leak a fraction
> of the total throughput available (to the other BDIs).
> 
> Which is what that bdi_set_min_ratio(bdi, 0) in unregister does. It
> resets the min_ratio for the bdi being taken out and frees up the min
> allocated bandwidth for the others.
> 
> So I think moving that do destroy would be fine; assuming the delay
> between unregister and destroy is typically 'short'. Because without
> that you can 'leak' this min ratio for extended periods which means the
> bandwidth is unavailable for other BDIs.
> 
> Does that make sense?

Your assessment is almost exactly what I had come up with, so it definitely
makes sense :-)
'destroy' does come very shortly after 'unregister' (and immediately before
'blk_put_queue' which actually frees the struct).  However the driving force
for this patch was a desire to move blk_cleanup_queue(), which calls
'destroy', earlier.  So the net result is that bdi_set_min_ratio will be
called slightly sooner.

Thanks,
NeilBrown

Content of type "application/pgp-signature" skipped