mbox series

[0/4] btrfs: sysfs and unsupported temp-fsid features for clones

Message ID cover.1696431315.git.anand.jain@oracle.com (mailing list archive)
Headers show
Series btrfs: sysfs and unsupported temp-fsid features for clones | expand

Message

Anand Jain Oct. 4, 2023, 3 p.m. UTC
Seed and device-add are the two features that must be unsupported
if a cloned device is using temp-fsid to mount, as they conflict
with multi-device functionality.

Additionally, add sysfs files for the temp-fsid feature.

Anand Jain (4):
  btrfs: comment for temp-fsid, fsid, and metadata_uuid
  btrfs: disable seed feature for temp-fsid
  btrfs: disable the device add feature for temp-fsid
  btrfs: show temp_fsid feature in sysfs

 fs/btrfs/ioctl.c   |  6 ++++++
 fs/btrfs/sysfs.c   | 20 ++++++++++++++++++++
 fs/btrfs/volumes.c |  8 ++++++++
 fs/btrfs/volumes.h |  4 ++++
 4 files changed, 38 insertions(+)

Comments

David Sterba Oct. 6, 2023, 3:07 p.m. UTC | #1
On Wed, Oct 04, 2023 at 11:00:23PM +0800, Anand Jain wrote:
> Seed and device-add are the two features that must be unsupported
> if a cloned device is using temp-fsid to mount, as they conflict
> with multi-device functionality.
> 
> Additionally, add sysfs files for the temp-fsid feature.
> 
> Anand Jain (4):
>   btrfs: comment for temp-fsid, fsid, and metadata_uuid
>   btrfs: disable seed feature for temp-fsid
>   btrfs: disable the device add feature for temp-fsid
>   btrfs: show temp_fsid feature in sysfs

Added to misc-next, thanks. The eventual change to the sysfs file can be
done later.

How are we going to proceed with the patch from Guilherme?
Anand Jain Oct. 7, 2023, 10:30 a.m. UTC | #2
On 10/6/23 23:07, David Sterba wrote:
> On Wed, Oct 04, 2023 at 11:00:23PM +0800, Anand Jain wrote:
>> Seed and device-add are the two features that must be unsupported
>> if a cloned device is using temp-fsid to mount, as they conflict
>> with multi-device functionality.
>>
>> Additionally, add sysfs files for the temp-fsid feature.
>>
>> Anand Jain (4):
>>    btrfs: comment for temp-fsid, fsid, and metadata_uuid
>>    btrfs: disable seed feature for temp-fsid
>>    btrfs: disable the device add feature for temp-fsid
>>    btrfs: show temp_fsid feature in sysfs
> 
> Added to misc-next, thanks. The eventual change to the sysfs file can be
> done later.
> 
> How are we going to proceed with the patch from Guilherme?

The last step is to ensure that the temp-fsid feature is restricted
with the temp-fsid superblock flag. Guilherme's patches
(kernel, mkfs, and tune) already handle it but need a rebase.
Can Guilherme send an RFC patch for feedback from others and
copy suggested-by. Because, I haven't found a compelling reason
for the restriction, except to improve the user experience.

His fstests patch will be accepted. And progressively we should
add more coverage when the fstests configuration does not include
the temp-fsid environment.

Lastly, in fact even for this patchset:

   Co-developed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Thanks, Anand
Guilherme G. Piccoli Oct. 9, 2023, 7 a.m. UTC | #3
On 07/10/2023 12:30, Anand Jain wrote:
> [...]
>> How are we going to proceed with the patch from Guilherme?
> 
> The last step is to ensure that the temp-fsid feature is restricted
> with the temp-fsid superblock flag. Guilherme's patches
> (kernel, mkfs, and tune) already handle it but need a rebase.
> Can Guilherme send an RFC patch for feedback from others and
> copy suggested-by. Because, I haven't found a compelling reason
> for the restriction, except to improve the user experience.
> 
> His fstests patch will be accepted. And progressively we should
> add more coverage when the fstests configuration does not include
> the temp-fsid environment.
> 
> Lastly, in fact even for this patchset:
> 
>    Co-developed-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> Thanks, Anand
> 

Thanks Anand and David. The first thing I need to do, is to build
misc-next and test it if if works with my setup, to double-check Anand's
approach fits the use-case (it seems to, so far). Then, I guess we'll
need to see if there's a missing piece on that - in the other thread
Anand mentioned maybe the superblock flag would be useful after all, so
if that's necessary, I can send it of course.

Finally, this week I'm away from my regular system and cannot provide
the test results, next week I can do that for sure.

Thank you both for looping me in and for the details and clarifications!
Cheers,


Guilherme
Anand Jain Oct. 9, 2023, 8:07 a.m. UTC | #4
>> Can Guilherme send an RFC patch for feedback from others and
>> copy suggested-by. Because, I haven't found a compelling reason
>> for the restriction, except to improve the user experience.

My comments about the superblock flag are above.

User experiences are subjective, so we need others to comment;
an RFC will help.


> Thanks Anand and David. The first thing I need to do, is to build
> misc-next and test it if if works with my setup, to double-check Anand's
> approach fits the use-case (it seems to, so far). Then, I guess we'll
> need to see if there's a missing piece on that - in the other thread
> Anand mentioned maybe the superblock flag would be useful after all, so
> if that's necessary, I can send it of course.
>  > Finally, this week I'm away from my regular system and cannot provide
> the test results,


> next week I can do that for sure.

That's great; much appreciated.

Thanks, Anand
David Sterba Oct. 9, 2023, 11:59 p.m. UTC | #5
On Mon, Oct 09, 2023 at 01:37:22PM +0530, Anand Jain wrote:
> >> Can Guilherme send an RFC patch for feedback from others and
> >> copy suggested-by. Because, I haven't found a compelling reason
> >> for the restriction, except to improve the user experience.
> 
> My comments about the superblock flag are above.
> 
> User experiences are subjective, so we need others to comment;
> an RFC will help.

A few things changed, the incompat bit was supposed to prevent
accidentally duplicated fsids but with your recent changes this is safe.
This would need to let Guilherme check if the A/B use case still works
but this seems to be so as I'm reading the changelog.

In a controlled environment the incompat bit will not bring much value
other than yet another sanity check preventing some user error, but
related only to the multiple devices.
Anand Jain Oct. 10, 2023, 1:22 a.m. UTC | #6
On 10/10/23 05:29, David Sterba wrote:
> On Mon, Oct 09, 2023 at 01:37:22PM +0530, Anand Jain wrote:
>>>> Can Guilherme send an RFC patch for feedback from others and
>>>> copy suggested-by. Because, I haven't found a compelling reason
>>>> for the restriction, except to improve the user experience.
>>
>> My comments about the superblock flag are above.
>>
>> User experiences are subjective, so we need others to comment;
>> an RFC will help.
> 
> A few things changed, the incompat bit was supposed to prevent
> accidentally duplicated fsids but with your recent changes this is safe.
> This would need to let Guilherme check if the A/B use case still works
> but this seems to be so as I'm reading the changelog.
> 


> In a controlled environment the incompat bit will not bring much value
> other than yet another sanity check preventing some user error, > but
> related only to the multiple devices.


Agreed. Anyway, the kernel would continue to fail the mount of the
duplicate-fsid for a multi-device filesystem.

And

    $ mkfs.btrfs -U <duplicate-fsid> /dev/sda1 /dev/sda2 ..

for a multi-device filesystem will also fail.

Therefore, the only avenue for the user to make a mistake is
by using dd to copy for a multi-device setup.

Thanks, Anand
Guilherme G. Piccoli Oct. 18, 2023, 1:37 p.m. UTC | #7
On 10/10/2023 01:59, David Sterba wrote:
> On Mon, Oct 09, 2023 at 01:37:22PM +0530, Anand Jain wrote:
>>>> Can Guilherme send an RFC patch for feedback from others and
>>>> copy suggested-by. Because, I haven't found a compelling reason
>>>> for the restriction, except to improve the user experience.
>>
>> My comments about the superblock flag are above.
>>
>> User experiences are subjective, so we need others to comment;
>> an RFC will help.
> 
> A few things changed, the incompat bit was supposed to prevent
> accidentally duplicated fsids but with your recent changes this is safe.
> This would need to let Guilherme check if the A/B use case still works
> but this seems to be so as I'm reading the changelog.
> 
> In a controlled environment the incompat bit will not bring much value
> other than yet another sanity check preventing some user error, but
> related only to the multiple devices.

Hi David and Anand, I've manage to test misc-next of today, that
includes both this patchset as well as the "support cloned-device mount
capability​" one.

It seems to be working fine for our use case, though I'll test a bit
more on Deck. I was able to mount the same filesystem (spread in 2 nvme
devices) at the same time, in any order...the second one always get the
temp-fsid. Tested also re-mounting the devices on other locations, and
it seems all consistent, with no error observed.

I also question the value of the incompat flag, not seeing much use for
that..looping Qu Wenruo as they first suggested this flag-based
approach, in case there is some more feedback...

Anyway, thanks for your improved approach Anand and to David: is it
expected to land on 6.7?
Cheers,


Guilherme
David Sterba Oct. 18, 2023, 11:04 p.m. UTC | #8
On Wed, Oct 18, 2023 at 03:37:54PM +0200, Guilherme G. Piccoli wrote:
> On 10/10/2023 01:59, David Sterba wrote:
> > On Mon, Oct 09, 2023 at 01:37:22PM +0530, Anand Jain wrote:
> >>>> Can Guilherme send an RFC patch for feedback from others and
> >>>> copy suggested-by. Because, I haven't found a compelling reason
> >>>> for the restriction, except to improve the user experience.
> >>
> >> My comments about the superblock flag are above.
> >>
> >> User experiences are subjective, so we need others to comment;
> >> an RFC will help.
> > 
> > A few things changed, the incompat bit was supposed to prevent
> > accidentally duplicated fsids but with your recent changes this is safe.
> > This would need to let Guilherme check if the A/B use case still works
> > but this seems to be so as I'm reading the changelog.
> > 
> > In a controlled environment the incompat bit will not bring much value
> > other than yet another sanity check preventing some user error, but
> > related only to the multiple devices.
> 
> Hi David and Anand, I've manage to test misc-next of today, that
> includes both this patchset as well as the "support cloned-device mount
> capability​" one.
> 
> It seems to be working fine for our use case, though I'll test a bit
> more on Deck. I was able to mount the same filesystem (spread in 2 nvme
> devices) at the same time, in any order...the second one always get the
> temp-fsid. Tested also re-mounting the devices on other locations, and
> it seems all consistent, with no error observed.

Great, thanks.

> I also question the value of the incompat flag, not seeing much use for
> that..looping Qu Wenruo as they first suggested this flag-based
> approach, in case there is some more feedback...

Yeah at this point I don't see the need for the incompat bit, which is
the better outcome.

> Anyway, thanks for your improved approach Anand and to David: is it
> expected to land on 6.7?

Yes, what's in misc-next is queued for 6.7, also we have the whole
development cycle to fix remaining bugs.
Guilherme G. Piccoli Oct. 19, 2023, 8:06 a.m. UTC | #9
On 19/10/2023 01:04, David Sterba wrote:
> [...]
>> Anyway, thanks for your improved approach Anand and to David: is it
>> expected to land on 6.7?
> 
> Yes, what's in misc-next is queued for 6.7, also we have the whole
> development cycle to fix remaining bugs.

Thanks a bunch for confirming and all guidance through this feature process.