From patchwork Sun Nov 30 08:26:49 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omar Sandoval X-Patchwork-Id: 5407321 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 9CBA4BEEA8 for ; Sun, 30 Nov 2014 08:27:19 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8A25620120 for ; Sun, 30 Nov 2014 08:27:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6464020131 for ; Sun, 30 Nov 2014 08:27:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752048AbaK3I1H (ORCPT ); Sun, 30 Nov 2014 03:27:07 -0500 Received: from mail-pd0-f178.google.com ([209.85.192.178]:38090 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752018AbaK3I1D (ORCPT ); Sun, 30 Nov 2014 03:27:03 -0500 Received: by mail-pd0-f178.google.com with SMTP id g10so8956895pdj.37 for ; Sun, 30 Nov 2014 00:27:02 -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=8UvMrk7opUbB7idQ0OWTG/I10oI343GhsbbAvTiRfQQ=; b=Zzv9j5LEUEjO/vKqUNSBz1WBOi1039QOyWZYS+ESkJO8llwD9/nxpkEk9zHXdADOQV 1t14jdsZ0ovtfzH9sfLGYZGtHjqrXlsaZp6oJupaJBIhiLMm85roQc1LCdI+DW3Ow2uG ktRSfzrUFBxQyNEmnpLmpQ53178PPJ4Xx2bHFvBg7cLwzlaavmiD0c9FBTp6nxqbdnad nAfgFKggHrrmwCiqfXxWM65CipON1+vdiSopH33HyhZkk24DmggWwLfuzBLOJMUh1fYk T45u/ixzpXxASC56b9WgDAUKSjzW2U1LAyI8XQoVjNUAw/5CAo/kUmteXLyVXtItw21v IHBQ== X-Gm-Message-State: ALoCoQnqm18De0h5BJvsj0702h//yONOsu7fNJVKefQgSxus02t45/8vbEssx38lrM6YqsgTapND X-Received: by 10.66.232.168 with SMTP id tp8mr88325016pac.132.1417336022871; Sun, 30 Nov 2014 00:27:02 -0800 (PST) Received: from mew.localdomain ([72.192.100.38]) by mx.google.com with ESMTPSA id k10sm14383316pdm.3.2014.11.30.00.27.01 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 30 Nov 2014 00:27:02 -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 3/3] btrfs: refactor btrfs_device->name updates Date: Sun, 30 Nov 2014 00:26:49 -0800 Message-Id: <8881abdb8836e2d70f705baacc0fe491e75d66d4.1417335583.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 d13b253..6913bed 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; @@ -2146,7 +2181,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; @@ -2283,7 +2318,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 6e04f27..2298a70 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;