[V2,0/3] Supporting same fsid mounting through a compat_ro feature

Message

Guilherme G. Piccoli Aug. 3, 2023, 3:43 p.m. UTC

Hi all, this is the 2nd attempt of supporting same fsid mounting
on btrfs. V1 is here:
https://lore.kernel.org/linux-btrfs/[e-mail]/

The mechanism used to achieve that in V2 was a mix between the suggestion
from JohnS (spoofed fsid) and Qu (a single-dev compat_ro flag) - it is
still based in the metadata_uuid feature, leveraging that infrastructure
since it prevents lots of corner cases, like sysfs same-fsid crashes.

The patches are based on kernel v6.5-rc3 with Anand's metadata_uuid refactor
part 2 on top of it [0]; the btrfs-progs patch is based on "v6.3.3".

Comments/suggestions and overall feedback is much appreciated - tnx in advance!
Cheers,

Guilherme


[0] https://lore.kernel.org/linux-btrfs/[e-mail]/


Guilherme G. Piccoli (3):
  btrfs-progs: Add the single-dev feature (to both mkfs/tune)
  btrfs: Introduce the single-dev feature
  btrfs: Add parameter to force devices behave as single-dev ones

btrfs-progs:
 common/fsfeatures.c        |  7 ++++
 kernel-shared/ctree.h      |  3 +-
 kernel-shared/uapi/btrfs.h |  7 ++++
 mkfs/main.c                |  4 ++-
 tune/main.c                | 72 +++++++++++++++++++++++---------------
 5 files changed, 63 insertions(+), 30 deletions(-)

kernel:
 fs/btrfs/disk-io.c         |  19 +++++-
 fs/btrfs/fs.h              |   3 +-
 fs/btrfs/ioctl.c           |  18 +++++
 fs/btrfs/super.c           |  13 ++--
 fs/btrfs/super.h           |   2 +
 fs/btrfs/volumes.c         | 136 +++++++++++++++++++++++++++++++------
 fs/btrfs/volumes.h         |   5 +-
 include/uapi/linux/btrfs.h |   7 ++
 8 files changed, 175 insertions(+), 28 deletions(-)

Comments

Guilherme G. Piccoli Aug. 17, 2023, 1:56 p.m. UTC | #1

On 03/08/2023 12:43, Guilherme G. Piccoli wrote:
> Hi all, this is the 2nd attempt of supporting same fsid mounting
> on btrfs. V1 is here:
> https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> The mechanism used to achieve that in V2 was a mix between the suggestion
> from JohnS (spoofed fsid) and Qu (a single-dev compat_ro flag) - it is
> still based in the metadata_uuid feature, leveraging that infrastructure
> since it prevents lots of corner cases, like sysfs same-fsid crashes.
> 
> The patches are based on kernel v6.5-rc3 with Anand's metadata_uuid refactor
> part 2 on top of it [0]; the btrfs-progs patch is based on "v6.3.3".
> 
> Comments/suggestions and overall feedback is much appreciated - tnx in advance!
> Cheers,
> 
> Guilherme
> 
> 
> [0] https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> 
> Guilherme G. Piccoli (3):
>   btrfs-progs: Add the single-dev feature (to both mkfs/tune)
>   btrfs: Introduce the single-dev feature
>   btrfs: Add parameter to force devices behave as single-dev ones


Hi David, friendly ping about this one - do you think we could have it
merged, or do you have suggestions to improve it maybe?

Thanks in advance and apologies for the ping!
Cheers,


Guilherme

Josef Bacik Aug. 17, 2023, 2:19 p.m. UTC | #2

On Thu, Aug 03, 2023 at 12:43:38PM -0300, Guilherme G. Piccoli wrote:
> Hi all, this is the 2nd attempt of supporting same fsid mounting
> on btrfs. V1 is here:
> https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> The mechanism used to achieve that in V2 was a mix between the suggestion
> from JohnS (spoofed fsid) and Qu (a single-dev compat_ro flag) - it is
> still based in the metadata_uuid feature, leveraging that infrastructure
> since it prevents lots of corner cases, like sysfs same-fsid crashes.
> 
> The patches are based on kernel v6.5-rc3 with Anand's metadata_uuid refactor
> part 2 on top of it [0]; the btrfs-progs patch is based on "v6.3.3".
> 
> Comments/suggestions and overall feedback is much appreciated - tnx in advance!
> Cheers,
> 
> Guilherme
> 

In general the concept is fine with me, and the implementation seems reasonable.

With new features we want fstests to accompany them so we know they work
correctly, and we don't accidentally break them in the future.

I'd like to see tests that validate all the behaviors you're trying to
accomplish work as advertised, and that all the failure cases do in fact fail
properly.

Ideally a test that creates a single device fs image and mounts it in multiple
places as would be used in the Steam Deck.

Then a test that tries to add a device to it, replace, etc.  All the cases that
you expect to fail, and validate that they actually fail.

Then any other corner cases you can think of that I haven't thought of.

Make sure these new tests skip appropriately if the btrfs-progs support doesn't
exist, I'd likely throw the fstests into our CI before the code is merged to
make sure it's ready to be tested if/when it is merged.

Thanks,

Josef

Guilherme G. Piccoli Aug. 17, 2023, 2:23 p.m. UTC | #3

On 17/08/2023 11:19, Josef Bacik wrote:
> [...]
> In general the concept is fine with me, and the implementation seems reasonable.
> 
> With new features we want fstests to accompany them so we know they work
> correctly, and we don't accidentally break them in the future.
> 
> I'd like to see tests that validate all the behaviors you're trying to
> accomplish work as advertised, and that all the failure cases do in fact fail
> properly.
> 
> Ideally a test that creates a single device fs image and mounts it in multiple
> places as would be used in the Steam Deck.
> 
> Then a test that tries to add a device to it, replace, etc.  All the cases that
> you expect to fail, and validate that they actually fail.
> 
> Then any other corner cases you can think of that I haven't thought of.
> 
> Make sure these new tests skip appropriately if the btrfs-progs support doesn't
> exist, I'd likely throw the fstests into our CI before the code is merged to
> make sure it's ready to be tested if/when it is merged.
> 
> Thanks,
> 
> Josef
> 

Hi Josef, thanks a lot for your comprehensive response, it was pretty
helpful for me.

I agree with you, test cases are important indeed and I'll work them,
re-submitting a V3 with tests included.
Cheers,


Guilherme