All of lore.kernel.org
 help / color / mirror / Atom feed
From: Josef Bacik <josef@toxicpanda.com>
To: Anand Jain <anand.jain@oracle.com>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com,
	David Sterba <dsterba@suse.com>
Subject: Re: [PATCH v2 3/7] btrfs: do not read super look for a device path
Date: Mon, 27 Sep 2021 11:32:34 -0400	[thread overview]
Message-ID: <ffe45a8d-7ff9-ddda-2c5a-b0e1aae1441a@toxicpanda.com> (raw)
In-Reply-To: <41d2c028-6af5-ab2b-91fa-1090d4258ba9@oracle.com>

On 8/24/21 10:00 PM, Anand Jain wrote:
> On 28/07/2021 05:01, Josef Bacik wrote:
>> For device removal and replace we call btrfs_find_device_by_devspec,
>> which if we give it a device path and nothing else will call
>> btrfs_find_device_by_path, which opens the block device and reads the
>> super block and then looks up our device based on that.
>>
>> However this is completely unnecessary because we have the path stored
>> in our device on our fsdevices.  All we need to do if we're given a path
>> is look through the fs_devices on our file system and use that device if
>> we find it, reading the super block is just silly.
> 
> The device path as stored in our fs_devices can differ from the path
> provided by the user for the same device (for example, dm, lvm).
> 
> btrfs-progs sanitize the device path but, others (for example, an ioctl
> test case) might not. And the path lookup would fail.
> 
> Also, btrfs dev scan <path> can update the device path anytime, even
> after it is mounted. Fixing that failed the subsequent subvolume mounts
> (if I remember correctly).
> 

This is a good point, that's kind of a big deal from a UX perspective.

>> This fixes the case where we end up with our sb write "lock" getting the
>> dependency of the block device ->open_mutex, which resulted in the
>> following lockdep splat
> 
> Can we do..
> 
> btrfs_exclop_start()
>   ::
> find device part (read sb)
>   ::
> mnt_want_write_file()?
> 
> 

I looked into this, but we'd have to re-order the exclop_start to above the 
mnt_want_write_file() part everywhere to be consistent, and this is mostly OK 
except for balance.  Balance the exclop is tied to the lifetime of the balance 
ctl, which can exist past the task running balance because we could pause the 
balance.

Could we get around this?  Sure, but in my head exclop == lock.  This means we 
have something akin to

exclop_start
mnt_want_write_file()

pause balance
mnt_drop_write()

resume balance

exclop_start magic stuff in balance to resume without doing the exclop
mnt_want_write_file()
<do balance>
exclop_finish
mnt_drop_write()

If we're OK with this then we can definitely do that.

The other option is simply to make userspace do the superblock read and use the 
devid thing for us.  Then we just eat the UX problem for older tools where you 
want to do btrfs rm device /dev/mapper/whatever and we have the pathname as 
/dev/dm-#.

Both options are unattractive in their own way.  I think the first option is 
only annoying to us, and maintains the UX expectations.  But I want more than me 
to make this decision, so if you and Dave are OK with that I'll go with 
re-ordering exclop+mnt_want_write_file(), and then put the device lookup between 
the two of them for device removal.  Thanks,

Josef


  reply	other threads:[~2021-09-27 15:32 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-27 21:01 [PATCH v2 0/7] Josef Bacik
2021-07-27 21:01 ` [PATCH v2 1/7] btrfs: do not call close_fs_devices in btrfs_rm_device Josef Bacik
2021-09-01  8:13   ` Anand Jain
2021-07-27 21:01 ` [PATCH v2 2/7] btrfs: do not take the uuid_mutex " Josef Bacik
2021-09-01 12:01   ` Anand Jain
2021-09-01 17:08     ` David Sterba
2021-09-01 17:10     ` Josef Bacik
2021-09-01 19:49       ` Anand Jain
2021-09-02 12:58   ` David Sterba
2021-09-02 14:10     ` Josef Bacik
2021-09-17 14:33       ` David Sterba
2021-09-20  7:45   ` Anand Jain
2021-09-20  8:26     ` David Sterba
2021-09-20  9:41       ` Anand Jain
2021-09-23  4:33         ` Anand Jain
2021-09-21 11:59   ` Filipe Manana
2021-09-21 12:17     ` Filipe Manana
2021-09-22 15:33       ` Filipe Manana
2021-09-23  4:15         ` Anand Jain
2021-09-23  3:58   ` [PATCH] btrfs: drop lockdep assert in close_fs_devices() Anand Jain
2021-09-23  4:04     ` Anand Jain
2021-07-27 21:01 ` [PATCH v2 3/7] btrfs: do not read super look for a device path Josef Bacik
2021-08-25  2:00   ` Anand Jain
2021-09-27 15:32     ` Josef Bacik [this message]
2021-09-28 11:50       ` Anand Jain
2021-07-27 21:01 ` [PATCH v2 4/7] btrfs: update the bdev time directly when closing Josef Bacik
2021-08-25  0:35   ` Anand Jain
2021-09-02 12:16   ` David Sterba
2021-07-27 21:01 ` [PATCH v2 5/7] btrfs: delay blkdev_put until after the device remove Josef Bacik
2021-08-25  1:00   ` Anand Jain
2021-09-02 12:16   ` David Sterba
2021-07-27 21:01 ` [PATCH v2 6/7] btrfs: unify common code for the v1 and v2 versions of " Josef Bacik
2021-08-25  1:19   ` Anand Jain
2021-09-01 14:05   ` Nikolay Borisov
2021-07-27 21:01 ` [PATCH v2 7/7] btrfs: do not take the device_list_mutex in clone_fs_devices Josef Bacik
2021-08-24 22:08   ` Anand Jain
2021-09-01 13:35   ` Nikolay Borisov
2021-09-02 12:59   ` David Sterba
2021-09-17 15:06 ` [PATCH v2 0/7] David Sterba

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=ffe45a8d-7ff9-ddda-2c5a-b0e1aae1441a@toxicpanda.com \
    --to=josef@toxicpanda.com \
    --cc=anand.jain@oracle.com \
    --cc=dsterba@suse.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --subject='Re: [PATCH v2 3/7] btrfs: do not read super look for a device path' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.