On Wed, Oct 11, 2017 at 07:54:04PM +0200, David Sterba wrote: > On Tue, Oct 10, 2017 at 03:51:03PM -0600, Liu Bo wrote: > > It's pointless to defer it to a kthread helper as we're not under any > > special context. > > I agree the doubly deferred freeing is pointless. It's a weird mix of > RCU and workques and understanding all the interactions turned out to be > hard, last time I tried. > > The RCU stuff needs the rcu_barriers, and the callback can be served > from any process context. While the workqueus have their dedicated > kthreads. > > Calling free_device() is quick, it just adds the work to the queue and > returns. This makes __btrfs_close_devices/btrfs_rm_device/... and all > other callers fast, at the cost that there must be some explicit barrier > or waiting done when we want to make sure all the device resources have > been freed. > > I can't say the quick return is wrong, but it makes the device lifetime > hard to understand. The device freeing callback (__free_device) is > lightweight, but also calls "rcu_free" for the device name. >
I did check the history, at the time that this mix was introduced, it was not under from a non-sleep context, so I'm not sure why it needs to be deferred. rcu_barrier() was needed because we used to do blkdev_put() in this free_device(), which means the bdev's refcnt is held until the rcu callback runs. Later we found this is unnecessary because rcu protected resources and blkdev_put() can be done separated. > I have WIP patches to clean up the rcu and locking around devices and > actually document the rules, but with unreviewed pile in the mailinglist > I can't tell when this is going to land. If you want to simplify at > least the device freeing, please go on, and explain in the changelog > that it's not going to break anything. The hand-wavy sentence is not > what I'd expect :) I'll update the changelog to clarify the concern. Thanks, -liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
