Re: [PATCH] Btrfs: free btrfs_device in place

[Liu Bo <bo.li.liu@oracle.com>]

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