Re: [PATCH] Btrfs: free btrfs_device in place

2017-10-11 Thread Liu Bo 
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

Re: [PATCH] Btrfs: free btrfs_device in place

2017-10-11 Thread David Sterba 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: free btrfs_device in place

2017-10-11 Thread Anand Jain 



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 

Thanks, Anand


Signed-off-by: Liu Bo 
---
  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(_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(>rcu_work, __free_device);
-   schedule_work(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Btrfs: free btrfs_device in place

2017-10-10 Thread Liu Bo 
It's pointless to defer it to a kthread helper as we're not under any
special context.

Signed-off-by: Liu Bo 
---
 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(_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(>rcu_work, __free_device);
-   schedule_work(>rcu_work);
-}
-
 static void btrfs_close_bdev(struct btrfs_device *device)
 {
if (device->bdev && device->writeable) {
-- 
2.9.4

--
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