From patchwork Mon Dec 8 03:40:58 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omar Sandoval X-Patchwork-Id: 5453981 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id BDE22BEEA8 for ; Mon, 8 Dec 2014 03:43:08 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BCF2B20120 for ; Mon, 8 Dec 2014 03:43:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8F10D2011E for ; Mon, 8 Dec 2014 03:43:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753997AbaLHDnA (ORCPT ); Sun, 7 Dec 2014 22:43:00 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:38282 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753786AbaLHDlu (ORCPT ); Sun, 7 Dec 2014 22:41:50 -0500 Received: by mail-pa0-f43.google.com with SMTP id kx10so4374758pab.16 for ; Sun, 07 Dec 2014 19:41:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:in-reply-to:references; bh=Nu3btQAGaaopi1Z+nwCUXi7ntEEs4+ZK8hOrIUd+XGE=; b=XuYUrDWum97f/BQbSTEhQIHXN46HdexyIqjlyUyqU7RYlbBPpPetLq1IDXSydAAYml +06wJLaUPgJtAv8675PWBGZsW9R9TtaIOyctjbYfb28sKQmeF6hKQswl+eY0+KLycnpg eD9wuJL2scu/+8qEH4Wtxj5IypnPJZ9XpiR73+augPNleiACJ3qZUBhJF+Em5uxVhthD pY32CBnPp9geVxZ7eLTOvcaFfNy8RlzSs2eBmbBrR/inCZyrt0KQzkz/IbnPcOeD+pbZ 39qDs5Rf+0nzGr1FB2YITZnNVB3wqB4eAwZAkU5NoxZ9wxZY0/IRmXCKvo/tth1Zru82 hNQg== X-Gm-Message-State: ALoCoQl/xPihwGsC7wuFXgQ3gPM/eEB7RjwFt1DuZYYpjWXg+hQo0NinGxbuSW3sjUWzcBx9pawY X-Received: by 10.68.229.193 with SMTP id ss1mr58071239pbc.16.1418010109682; Sun, 07 Dec 2014 19:41:49 -0800 (PST) Received: from mew.localdomain (c-24-19-133-29.hsd1.wa.comcast.net. [24.19.133.29]) by mx.google.com with ESMTPSA id w5sm35041043pds.25.2014.12.07.19.41.47 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 07 Dec 2014 19:41:49 -0800 (PST) From: Omar Sandoval To: Chris Mason , Josef Bacik , Joe Perches , "Paul E. McKenney" , Josh Triplett , Steven Rostedt , Mathieu Desnoyers , linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org Cc: Omar Sandoval Subject: [PATCH v8 2/3] btrfs: refactor btrfs_device->name updates Date: Sun, 7 Dec 2014 19:40:58 -0800 Message-Id: <71255c85f10a0bfa8bc5aa4d41c9772d3fe48c1c.1418007402.git.osandov@osandov.com> X-Mailer: git-send-email 2.1.3 In-Reply-To: References: In-Reply-To: References: Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The rcu_string API introduced some new sparse errors but also revealed existing ones. First of all, the name in struct btrfs_device should be annotated as __rcu to prevent unsafe reads. Additionally, updates should go through rcu_dereference_protected to make it clear what's going on. This introduces some helper functions that factor out this functionality. Signed-off-by: Omar Sandoval --- fs/btrfs/volumes.c | 93 +++++++++++++++++++++++++++++++++++++----------------- fs/btrfs/volumes.h | 2 +- 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 2ad043d..359ea38 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -53,6 +53,45 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *device); DEFINE_MUTEX(uuid_mutex); static LIST_HEAD(fs_uuids); +/* + * Dereference the device name under the uuid_mutex. + */ +static inline struct rcu_string * +btrfs_dev_rcu_protected_name(struct btrfs_device *dev) +__must_hold(&uuid_mutex) +{ + return rcu_dereference_protected(dev->name, + lockdep_is_held(&uuid_mutex)); +} + +/* + * Use when the caller is the only possible updater. + */ +static inline struct rcu_string * +btrfs_dev_rcu_only_name(struct btrfs_device *dev) +{ + return rcu_dereference_protected(dev->name, 1); +} + +/* + * Rename a device under the uuid_mutex. + */ +static inline int btrfs_dev_rename(struct btrfs_device *dev, const char *name) +__must_hold(&uuid_mutex) +{ + struct rcu_string *old_name, *new_name; + + new_name = rcu_string_strdup(name, GFP_NOFS); + if (!new_name) + return -ENOMEM; + + old_name = btrfs_dev_rcu_protected_name(dev); + rcu_assign_pointer(dev->name, new_name); + rcu_string_free(old_name); + + return 0; +} + static void lock_chunks(struct btrfs_root *root) { mutex_lock(&root->fs_info->chunk_mutex); @@ -114,7 +153,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices) device = list_entry(fs_devices->devices.next, struct btrfs_device, dev_list); list_del(&device->dev_list); - rcu_string_free(device->name); + rcu_string_free(btrfs_dev_rcu_only_name(device)); kfree(device); } kfree(fs_devices); @@ -495,12 +534,10 @@ static noinline int device_list_add(const char *path, return PTR_ERR(device); } - name = rcu_string_strdup(path, GFP_NOFS); - if (!name) { + if (btrfs_dev_rename(device, path)) { kfree(device); return -ENOMEM; } - rcu_assign_pointer(device->name, name); mutex_lock(&fs_devices->device_list_mutex); list_add_rcu(&device->dev_list, &fs_devices->devices); @@ -509,7 +546,11 @@ static noinline int device_list_add(const char *path, ret = 1; device->fs_devices = fs_devices; - } else if (!device->name || strcmp(device->name->str, path)) { + } else { + name = btrfs_dev_rcu_protected_name(device); + if (name && strcmp(name->str, path) == 0) + goto out; + /* * When FS is already mounted. * 1. If you are here and if the device->name is NULL that @@ -547,17 +588,15 @@ static noinline int device_list_add(const char *path, return -EEXIST; } - name = rcu_string_strdup(path, GFP_NOFS); - if (!name) + if (btrfs_dev_rename(device, path)) return -ENOMEM; - rcu_string_free(device->name); - rcu_assign_pointer(device->name, name); if (device->missing) { fs_devices->missing_devices--; device->missing = 0; } } +out: /* * Unmount does not free the btrfs_device struct but would zero * generation along with most of the other members. So just update @@ -594,17 +633,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) if (IS_ERR(device)) goto error; - /* - * This is ok to do without rcu read locked because we hold the - * uuid mutex so nothing we touch in here is going to disappear. - */ - if (orig_dev->name) { - name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS); - if (!name) { + name = btrfs_dev_rcu_protected_name(orig_dev); + if (name) { + if (btrfs_dev_rename(device, name->str)) { kfree(device); goto error; } - rcu_assign_pointer(device->name, name); } list_add(&device->dev_list, &fs_devices->devices); @@ -666,7 +700,7 @@ again: } list_del_init(&device->dev_list); fs_devices->num_devices--; - rcu_string_free(device->name); + rcu_string_free(btrfs_dev_rcu_only_name(device)); kfree(device); } @@ -689,7 +723,7 @@ static void __free_device(struct work_struct *work) if (device->bdev) blkdev_put(device->bdev, device->mode); - rcu_string_free(device->name); + rcu_string_free(btrfs_dev_rcu_only_name(device)); kfree(device); } @@ -731,11 +765,10 @@ static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices) device->uuid); BUG_ON(IS_ERR(new_device)); /* -ENOMEM */ - /* Safe because we are under uuid_mutex */ - if (device->name) { - name = rcu_string_strdup(device->name->str, GFP_NOFS); - BUG_ON(!name); /* -ENOMEM */ - rcu_assign_pointer(new_device->name, name); + name = btrfs_dev_rcu_protected_name(device); + if (name) { + if (btrfs_dev_rename(new_device, name->str)) + BUG_ON(1); /* -ENOMEM */ } list_replace_rcu(&device->dev_list, &new_device->dev_list); @@ -794,18 +827,20 @@ static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices, u64 devid; int seeding = 1; int ret = 0; + struct rcu_string *name; flags |= FMODE_EXCL; list_for_each_entry(device, head, dev_list) { if (device->bdev) continue; - if (!device->name) + name = btrfs_dev_rcu_protected_name(device); + if (!name) continue; /* Just open everything we can; ignore failures here */ - if (btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1, - &bdev, &bh)) + if (btrfs_get_bdev_and_sb(name->str, flags, holder, 1, &bdev, + &bh)) continue; disk_super = (struct btrfs_super_block *)bh->b_data; @@ -2140,7 +2175,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path) trans = btrfs_start_transaction(root, 0); if (IS_ERR(trans)) { - rcu_string_free(device->name); + rcu_string_free(btrfs_dev_rcu_only_name(device)); kfree(device); ret = PTR_ERR(trans); goto error; @@ -2277,7 +2312,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path) error_trans: btrfs_end_transaction(trans, root); - rcu_string_free(device->name); + rcu_string_free(btrfs_dev_rcu_only_name(device)); btrfs_kobj_rm_device(root->fs_info, device); kfree(device); error: diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 08980fa..32e890a2 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -54,7 +54,7 @@ struct btrfs_device { struct btrfs_root *dev_root; - struct rcu_string *name; + struct rcu_string __rcu *name; u64 generation;