| Message ID | 20190327122418.24027-9-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | FITRIM improvement | expand |
On Wed, Mar 27, 2019 at 02:24:11PM +0200, Nikolay Borisov wrote: > btrfs_device structs are freed from RCU context since device iteration > is protected by RCU. Currently this is achieved by using call_rcu since > no blocking functions are called within btrfs_free_device. Future > refactoring of pending/pinned chunks will require calling sleeping > functions. This patch is in preparation for these changes by simply > switching from RCU callbacks to explicit calls of synchronize_rcu and > calling btrfs_free_device directly. A paragraph why this transition is correct would be good. It looks correct to me, so this is a matter of documentation.
On 1.04.19 г. 20:07 ч., David Sterba wrote: > On Wed, Mar 27, 2019 at 02:24:11PM +0200, Nikolay Borisov wrote: >> btrfs_device structs are freed from RCU context since device iteration >> is protected by RCU. Currently this is achieved by using call_rcu since >> no blocking functions are called within btrfs_free_device. Future >> refactoring of pending/pinned chunks will require calling sleeping >> functions. This patch is in preparation for these changes by simply >> switching from RCU callbacks to explicit calls of synchronize_rcu and >> calling btrfs_free_device directly. > > A paragraph why this transition is correct would be good. It looks > correct to me, so this is a matter of documentation. > Well call_rcu and synchronize_rcu() are functionally equivalent in that they run the code that follows them after a grace period has expired.
On Mon, Apr 01, 2019 at 08:20:56PM +0300, Nikolay Borisov wrote: > > > On 1.04.19 г. 20:07 ч., David Sterba wrote: > > On Wed, Mar 27, 2019 at 02:24:11PM +0200, Nikolay Borisov wrote: > >> btrfs_device structs are freed from RCU context since device iteration > >> is protected by RCU. Currently this is achieved by using call_rcu since > >> no blocking functions are called within btrfs_free_device. Future > >> refactoring of pending/pinned chunks will require calling sleeping > >> functions. This patch is in preparation for these changes by simply > >> switching from RCU callbacks to explicit calls of synchronize_rcu and > >> calling btrfs_free_device directly. > > > > A paragraph why this transition is correct would be good. It looks > > correct to me, so this is a matter of documentation. > > Well call_rcu and synchronize_rcu() are functionally equivalent in that > they run the code that follows them after a grace period has expired. This is enough to make the point about the correctness a bit more explicit, patch updated. When there's too much implicit assumptions, eg. about how RCU is used, then one sentence is ok to show that the patch author knows what he's doing. It's like pointing to the right direction, but repeating the whole mechanism is not necessary for the typical usage patterns.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index d2d37adbc6fd..90eff8452c31 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1230,14 +1230,6 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) mutex_unlock(&uuid_mutex); } -static void free_device_rcu(struct rcu_head *head) -{ - struct btrfs_device *device; - - device = container_of(head, struct btrfs_device, rcu); - btrfs_free_device(device); -} - static void btrfs_close_bdev(struct btrfs_device *device) { if (!device->bdev) @@ -1285,7 +1277,8 @@ static void btrfs_close_one_device(struct btrfs_device *device) list_replace_rcu(&device->dev_list, &new_device->dev_list); new_device->fs_devices = device->fs_devices; - call_rcu(&device->rcu, free_device_rcu); + synchronize_rcu(); + btrfs_free_device(device); } static int close_fs_devices(struct btrfs_fs_devices *fs_devices) @@ -2242,7 +2235,8 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, btrfs_scratch_superblocks(device->bdev, device->name->str); btrfs_close_bdev(device); - call_rcu(&device->rcu, free_device_rcu); + synchronize_rcu(); + btrfs_free_device(device); if (cur_devices->open_devices == 0) { while (fs_devices) { @@ -2310,7 +2304,8 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info, } btrfs_close_bdev(srcdev); - call_rcu(&srcdev->rcu, free_device_rcu); + synchronize_rcu(); + btrfs_free_device(srcdev); /* if this is no devs we rather delete the fs_devices */ if (!fs_devices->num_devices) { @@ -2368,7 +2363,8 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev) btrfs_scratch_superblocks(tgtdev->bdev, tgtdev->name->str); btrfs_close_bdev(tgtdev); - call_rcu(&tgtdev->rcu, free_device_rcu); + synchronize_rcu(); + btrfs_free_device(tgtdev); } static struct btrfs_device *btrfs_find_device_by_path( diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index a0f09aad3770..81784b68ca12 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -118,7 +118,6 @@ struct btrfs_device { struct scrub_ctx *scrub_ctx; struct btrfs_work work; - struct rcu_head rcu; /* readahead state */ atomic_t reada_in_flight;
btrfs_device structs are freed from RCU context since device iteration is protected by RCU. Currently this is achieved by using call_rcu since no blocking functions are called within btrfs_free_device. Future refactoring of pending/pinned chunks will require calling sleeping functions. This patch is in preparation for these changes by simply switching from RCU callbacks to explicit calls of synchronize_rcu and calling btrfs_free_device directly. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/volumes.c | 20 ++++++++------------ fs/btrfs/volumes.h | 1 - 2 files changed, 8 insertions(+), 13 deletions(-)