| Message ID | 20171010215103.20828-2-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/11/2017 05:51 AM, Liu Bo wrote: > It's pointless to defer it to a kthread helper as we're not under any > special context. Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/volumes.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index d983cea..4a72c45 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -836,26 +836,16 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step) > mutex_unlock(&uuid_mutex); > } > > -static void __free_device(struct work_struct *work) > +static void free_device(struct rcu_head *head) > { > struct btrfs_device *device; > > - device = container_of(work, struct btrfs_device, rcu_work); > + device = container_of(head, struct btrfs_device, rcu); > rcu_string_free(device->name); > bio_put(device->flush_bio); > kfree(device); > } > > -static void free_device(struct rcu_head *head) > -{ > - struct btrfs_device *device; > - > - device = container_of(head, struct btrfs_device, rcu); > - > - INIT_WORK(&device->rcu_work, __free_device); > - schedule_work(&device->rcu_work); > -} > - > static void btrfs_close_bdev(struct btrfs_device *device) > { > if (device->bdev && device->writeable) { > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index d983cea..4a72c45 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -836,26 +836,16 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step) mutex_unlock(&uuid_mutex); } -static void __free_device(struct work_struct *work) +static void free_device(struct rcu_head *head) { struct btrfs_device *device; - device = container_of(work, struct btrfs_device, rcu_work); + device = container_of(head, struct btrfs_device, rcu); rcu_string_free(device->name); bio_put(device->flush_bio); kfree(device); } -static void free_device(struct rcu_head *head) -{ - struct btrfs_device *device; - - device = container_of(head, struct btrfs_device, rcu); - - INIT_WORK(&device->rcu_work, __free_device); - schedule_work(&device->rcu_work); -} - static void btrfs_close_bdev(struct btrfs_device *device) { if (device->bdev && device->writeable) {
It's pointless to defer it to a kthread helper as we're not under any special context. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/volumes.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)