From: Nikolay Borisov <nborisov@suse.com>
To: Anand Jain <anand.jain@oracle.com>,
linux-btrfs@vger.kernel.org, dsterba@suse.com
Subject: Re: [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent
Date: Tue, 21 Sep 2021 14:01:33 +0300 [thread overview]
Message-ID: <840713c4-48ef-b4e6-91e3-f92158448b7c@suse.com> (raw)
In-Reply-To: <ec3ecca596bf5d9de5e152942a277ab48915f0cf.1632179897.git.anand.jain@oracle.com>
On 21.09.21 г. 7:33, Anand Jain wrote:
> btrfs_prepare_sprout() splices seed devices into its own struct fs_devices,
> so that its parent function btrfs_init_new_device() can add the new sprout
> device to fs_info->fs_devices.
>
> Both btrfs_prepare_sprout() and btrfs_init_new_device() needs
> device_list_mutex. But they are holding it sequentially, thus creates a
> small window to an opportunity to race. Close this opportunity and hold
> device_list_mutex common to both btrfs_init_new_device() and
> btrfs_prepare_sprout().
>
> This patch splits btrfs_prepare_sprout() into btrfs_alloc_sprout() and
> btrfs_splice_sprout(). This split is essential because device_list_mutex
> shouldn't be held for btrfs_alloc_sprout() but must be held for
> btrfs_splice_sprout(). So now a common device_list_mutex can be used
> between btrfs_init_new_device() and btrfs_splice_sprout().
>
> This patch also moves the lockdep_assert_held(&uuid_mutex) from the
> starting of the function to just above the line where we need this lock.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v6:
> Remove RFC.
> Split btrfs_prepare_sprout so that the allocation part can be outside
> of the device_list_mutex in the parent function btrfs_init_new_device().
>
> fs/btrfs/volumes.c | 46 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e4079e25db70..b21eac32ec98 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2376,19 +2376,13 @@ struct btrfs_device *btrfs_find_device_by_devspec(
> return btrfs_find_device_by_path(fs_info, device_path);
> }
>
> -/*
> - * does all the dirty work required for changing file system's UUID.
> - */
> -static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
> +static int btrfs_alloc_sprout(struct btrfs_fs_info *fs_info,
> + struct btrfs_fs_devices **seed_devices_ret)
Nope, make the function return a struct btrfs_fs_devices *.
> {
> struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> struct btrfs_fs_devices *old_devices;
> struct btrfs_fs_devices *seed_devices;
> - struct btrfs_super_block *disk_super = fs_info->super_copy;
> - struct btrfs_device *device;
> - u64 super_flags;
>
> - lockdep_assert_held(&uuid_mutex);
> if (!fs_devices->seeding)
> return -EINVAL;
>
> @@ -2412,6 +2406,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
> return PTR_ERR(old_devices);
> }
>
> + lockdep_assert_held(&uuid_mutex);
> list_add(&old_devices->fs_list, &fs_uuids);
>
> memcpy(seed_devices, fs_devices, sizeof(*seed_devices));
> @@ -2419,7 +2414,23 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
> INIT_LIST_HEAD(&seed_devices->devices);
> INIT_LIST_HEAD(&seed_devices->alloc_list);
>
> - mutex_lock(&fs_devices->device_list_mutex);
> + *seed_devices_ret = seed_devices;
> +
> + return 0;
> +}
> +
> +/*
> + * Splice seed devices into the sprout fs_devices.
> + * Generate a new fsid for the sprouted readwrite btrfs.
> + */
> +static void btrfs_splice_sprout(struct btrfs_fs_info *fs_info,
> + struct btrfs_fs_devices *seed_devices)
> +{
This function is missing a lockdep_assert_held annotation and it depends
on the device_list_mutex being held.
However looking at the resulting code it doesn't look good, because
btrfs_splice_sporut suggests you simply add the seed device to a bunch
of places, yet looking at the function's body it's evident it actually
finishes some parts of the initialization, changes the uuid of the
fs_devices. I'm not convinced it really makes the code better or at the
very least the 'splice_sprout' needs to be changed, because splicing is
a minot part of what this function really does.
<snip>
next prev parent reply other threads:[~2021-09-21 11:01 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-21 4:33 [PATCH v6 0/3] btrfs: cleanup prepare_sprout Anand Jain
2021-09-21 4:33 ` [PATCH v6 1/3] btrfs: declare seeding_dev in init_new_device as bool Anand Jain
2021-09-21 11:01 ` Nikolay Borisov
2021-10-13 8:00 ` Anand Jain
2021-10-28 4:39 ` Anand Jain
2021-11-08 20:06 ` David Sterba
2021-09-21 4:33 ` [PATCH v6 2/3] btrfs: remove unused device_list_mutex for seed fs_devices Anand Jain
2021-09-21 6:44 ` Nikolay Borisov
2021-09-22 11:29 ` Anand Jain
2021-09-21 4:33 ` [PATCH v6 3/3] btrfs: consolidate device_list_mutex in prepare_sprout to its parent Anand Jain
2021-09-21 11:01 ` Nikolay Borisov [this message]
2021-09-22 11:41 ` Anand Jain
2021-09-23 6:52 ` Nikolay Borisov
2021-09-23 11:55 ` Anand Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=840713c4-48ef-b4e6-91e3-f92158448b7c@suse.com \
--to=nborisov@suse.com \
--cc=anand.jain@oracle.com \
--cc=dsterba@suse.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).