[PATCH v1 0/4] setting uuid of online filesystems

[PATCH v1 0/4] setting uuid of online filesystems

[Catherine Hoang]

Hi all,

This series of patches implements a new ioctl to set the uuid of mounted
filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
to allow userspace to update the uuid.

Comments and feedback appreciated!

Catherine

Catherine Hoang (4):
  xfs: refactor xfs_uuid_mount and xfs_uuid_unmount
  xfs: implement custom freeze/thaw functions
  xfs: add XFS_IOC_SETFSUUID ioctl
  xfs: export meta uuid via xfs_fsop_geom

 fs/xfs/libxfs/xfs_fs.h |   4 +-
 fs/xfs/libxfs/xfs_sb.c |   5 ++
 fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_log.c       |  19 +++++++
 fs/xfs/xfs_log.h       |   2 +
 fs/xfs/xfs_mount.c     |  30 +++++++++--
 fs/xfs/xfs_mount.h     |   2 +
 fs/xfs/xfs_super.c     | 112 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_super.h     |   5 ++
 9 files changed, 280 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v1 1/4] xfs: refactor xfs_uuid_mount and xfs_uuid_unmount

[Catherine Hoang]

Separate out the code that adds and removes a uuid from the uuid table. The
next patch uses these helpers to set the uuid of a mounted filesystem.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 fs/xfs/xfs_mount.c | 30 +++++++++++++++++++++++++-----
 fs/xfs/xfs_mount.h |  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index fb87ffb48f7f..434a67235fc9 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -58,7 +58,7 @@ xfs_uuid_mount(
 	struct xfs_mount	*mp)
 {
 	uuid_t			*uuid = &mp->m_sb.sb_uuid;
-	int			hole, i;
+	int			error;
 
 	/* Publish UUID in struct super_block */
 	uuid_copy(&mp->m_super->s_uuid, uuid);
@@ -71,6 +71,21 @@ xfs_uuid_mount(
 		return -EINVAL;
 	}
 
+	error = xfs_uuid_remember(uuid);
+	if (error) {
+		xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount", uuid);
+		return error;
+	}
+
+	return 0;
+}
+
+int
+xfs_uuid_remember(
+	const uuid_t		*uuid)
+{
+	int			hole, i;
+
 	mutex_lock(&xfs_uuid_table_mutex);
 	for (i = 0, hole = -1; i < xfs_uuid_table_size; i++) {
 		if (uuid_is_null(&xfs_uuid_table[i])) {
@@ -94,7 +109,6 @@ xfs_uuid_mount(
 
  out_duplicate:
 	mutex_unlock(&xfs_uuid_table_mutex);
-	xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount", uuid);
 	return -EINVAL;
 }
 
@@ -102,12 +116,18 @@ STATIC void
 xfs_uuid_unmount(
 	struct xfs_mount	*mp)
 {
-	uuid_t			*uuid = &mp->m_sb.sb_uuid;
-	int			i;
-
 	if (xfs_has_nouuid(mp))
 		return;
 
+	xfs_uuid_forget(&mp->m_sb.sb_uuid);
+}
+
+void
+xfs_uuid_forget(
+	const uuid_t		*uuid)
+{
+	int			i;
+
 	mutex_lock(&xfs_uuid_table_mutex);
 	for (i = 0; i < xfs_uuid_table_size; i++) {
 		if (uuid_is_null(&xfs_uuid_table[i]))
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index f3269c0626f0..ee08aeaf5430 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -484,6 +484,8 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
 int xfs_buf_hash_init(struct xfs_perag *pag);
 void xfs_buf_hash_destroy(struct xfs_perag *pag);
 
+int xfs_uuid_remember(const uuid_t *uuid);
+void xfs_uuid_forget(const uuid_t *uuid);
 extern void	xfs_uuid_table_free(void);
 extern uint64_t xfs_default_resblks(xfs_mount_t *mp);
 extern int	xfs_mountfs(xfs_mount_t *mp);
-- 
2.34.1

[PATCH v1 2/4] xfs: implement custom freeze/thaw functions

[Catherine Hoang]

Implement internal freeze/thaw functions and prevent other threads from changing
the freeze level by adding a new SB_FREEZE_ECLUSIVE level. This is required to
prevent concurrent transactions while we are updating the uuid.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 fs/xfs/xfs_super.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_super.h |   5 ++
 2 files changed, 117 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 2479b5cbd75e..6a52ae660810 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2279,6 +2279,118 @@ static inline int xfs_cpu_hotplug_init(void) { return 0; }
 static inline void xfs_cpu_hotplug_destroy(void) {}
 #endif
 
+/*
+ * We need to disable all writer threads, which means taking the first two
+ * freeze levels to put userspace to sleep, and the third freeze level to
+ * prevent background threads from starting new transactions.  Take one level
+ * more to prevent other callers from unfreezing the filesystem while we run.
+ */
+int
+xfs_internal_freeze(
+	struct xfs_mount	*mp)
+{
+	struct super_block	*sb = mp->m_super;
+	int			level;
+	int			error = 0;
+
+	/* Wait until we're ready to freeze. */
+	down_write(&sb->s_umount);
+	while (sb->s_writers.frozen != SB_UNFROZEN) {
+		up_write(&sb->s_umount);
+		delay(HZ / 10);
+		down_write(&sb->s_umount);
+	}
+
+	if (sb_rdonly(sb)) {
+		sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
+		goto done;
+	}
+
+	sb->s_writers.frozen = SB_FREEZE_WRITE;
+	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
+	up_write(&sb->s_umount);
+	percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_WRITE - 1);
+	down_write(&sb->s_umount);
+
+	/* Now we go and block page faults... */
+	sb->s_writers.frozen = SB_FREEZE_PAGEFAULT;
+	percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_PAGEFAULT - 1);
+
+	/*
+	 * All writers are done so after syncing there won't be dirty data.
+	 * Let xfs_fs_sync_fs flush dirty data so the VFS won't start writeback
+	 * and to disable the background gc workers.
+	 */
+	error = sync_filesystem(sb);
+	if (error) {
+		sb->s_writers.frozen = SB_UNFROZEN;
+		for (level = SB_FREEZE_PAGEFAULT - 1; level >= 0; level--)
+			percpu_up_write(sb->s_writers.rw_sem + level);
+		wake_up(&sb->s_writers.wait_unfrozen);
+		up_write(&sb->s_umount);
+		return error;
+	}
+
+	/* Now wait for internal filesystem counter */
+	sb->s_writers.frozen = SB_FREEZE_FS;
+	percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_FS - 1);
+
+	xfs_log_clean(mp);
+
+	/*
+	 * To prevent anyone else from unfreezing us, set the VFS freeze
+	 * level to one higher than FREEZE_COMPLETE.
+	 */
+	sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
+	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
+		percpu_rwsem_release(sb->s_writers.rw_sem + level, 0,
+				_THIS_IP_);
+done:
+	up_write(&sb->s_umount);
+	return 0;
+}
+
+void
+xfs_internal_unfreeze(
+	struct xfs_mount	*mp)
+{
+	struct super_block	*sb = mp->m_super;
+	int			level;
+
+	down_write(&sb->s_umount);
+	if (sb->s_writers.frozen != SB_FREEZE_EXCLUSIVE) {
+		/* somebody snuck in and unfroze us? */
+		ASSERT(0);
+		up_write(&sb->s_umount);
+		return;
+	}
+
+	if (sb_rdonly(sb)) {
+		sb->s_writers.frozen = SB_UNFROZEN;
+		goto out;
+	}
+
+	for (level = 0; level < SB_FREEZE_LEVELS; ++level)
+		percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0,
+				_THIS_IP_);
+
+	/*
+	 * We didn't call xfs_fs_freeze, so we can't call xfs_fs_thaw.  Start
+	 * the background gc workers that were shut down by xfs_fs_sync_fs
+	 * when we froze.
+	 */
+	xfs_blockgc_start(mp);
+	xfs_inodegc_start(mp);
+
+	sb->s_writers.frozen = SB_UNFROZEN;
+	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
+		percpu_up_write(sb->s_writers.rw_sem + level);
+out:
+	wake_up(&sb->s_writers.wait_unfrozen);
+	up_write(&sb->s_umount);
+	return;
+}
+
 STATIC int __init
 init_xfs_fs(void)
 {
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index 364e2c2648a8..0817cfc340ef 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -81,6 +81,8 @@ extern void xfs_qm_exit(void);
 # define XFS_WQFLAGS(wqflags)	(wqflags)
 #endif
 
+#define SB_FREEZE_EXCLUSIVE	(SB_FREEZE_COMPLETE + 1)
+
 struct xfs_inode;
 struct xfs_mount;
 struct xfs_buftarg;
@@ -98,6 +100,9 @@ extern void xfs_reinit_percpu_counters(struct xfs_mount *mp);
 
 extern struct workqueue_struct *xfs_discard_wq;
 
+extern int xfs_internal_freeze(struct xfs_mount *mp);
+extern void xfs_internal_unfreeze(struct xfs_mount *mp);
+
 #define XFS_M(sb)		((struct xfs_mount *)((sb)->s_fs_info))
 
 #endif	/* __XFS_SUPER_H__ */
-- 
2.34.1

Re: [PATCH v1 2/4] xfs: implement custom freeze/thaw functions

[Amir Goldstein]

On Tue, Mar 14, 2023 at 6:25 AM Catherine Hoang
<catherine.hoang@oracle.com> wrote:
>
> Implement internal freeze/thaw functions and prevent other threads from changing
> the freeze level by adding a new SB_FREEZE_ECLUSIVE level. This is required to

This looks troubling in several ways:
- Layering violation
- Duplication of subtle vfs code

> prevent concurrent transactions while we are updating the uuid.
>

Wouldn't it be easier to hold s_umount while updating the uuid?
Let userspace freeze before XFS_IOC_SETFSUUID and let
XFS_IOC_SETFSUUID take s_umount and verify that fs is frozen.

> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
>  fs/xfs/xfs_super.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_super.h |   5 ++
>  2 files changed, 117 insertions(+)
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 2479b5cbd75e..6a52ae660810 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2279,6 +2279,118 @@ static inline int xfs_cpu_hotplug_init(void) { return 0; }
>  static inline void xfs_cpu_hotplug_destroy(void) {}
>  #endif
>
> +/*
> + * We need to disable all writer threads, which means taking the first two
> + * freeze levels to put userspace to sleep, and the third freeze level to
> + * prevent background threads from starting new transactions.  Take one level
> + * more to prevent other callers from unfreezing the filesystem while we run.
> + */
> +int
> +xfs_internal_freeze(
> +       struct xfs_mount        *mp)
> +{
> +       struct super_block      *sb = mp->m_super;
> +       int                     level;
> +       int                     error = 0;
> +
> +       /* Wait until we're ready to freeze. */
> +       down_write(&sb->s_umount);
> +       while (sb->s_writers.frozen != SB_UNFROZEN) {
> +               up_write(&sb->s_umount);
> +               delay(HZ / 10);
> +               down_write(&sb->s_umount);
> +       }

That can easily wait forever, without any task holding any lock.

> +
> +       if (sb_rdonly(sb)) {
> +               sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
> +               goto done;
> +       }
> +
> +       sb->s_writers.frozen = SB_FREEZE_WRITE;
> +       /* Release s_umount to preserve sb_start_write -> s_umount ordering */
> +       up_write(&sb->s_umount);
> +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_WRITE - 1);
> +       down_write(&sb->s_umount);
> +
> +       /* Now we go and block page faults... */
> +       sb->s_writers.frozen = SB_FREEZE_PAGEFAULT;
> +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_PAGEFAULT - 1);
> +
> +       /*
> +        * All writers are done so after syncing there won't be dirty data.
> +        * Let xfs_fs_sync_fs flush dirty data so the VFS won't start writeback
> +        * and to disable the background gc workers.
> +        */
> +       error = sync_filesystem(sb);
> +       if (error) {
> +               sb->s_writers.frozen = SB_UNFROZEN;
> +               for (level = SB_FREEZE_PAGEFAULT - 1; level >= 0; level--)
> +                       percpu_up_write(sb->s_writers.rw_sem + level);
> +               wake_up(&sb->s_writers.wait_unfrozen);
> +               up_write(&sb->s_umount);
> +               return error;
> +       }
> +
> +       /* Now wait for internal filesystem counter */
> +       sb->s_writers.frozen = SB_FREEZE_FS;
> +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_FS - 1);
> +
> +       xfs_log_clean(mp);
> +
> +       /*
> +        * To prevent anyone else from unfreezing us, set the VFS freeze
> +        * level to one higher than FREEZE_COMPLETE.
> +        */
> +       sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
> +       for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
> +               percpu_rwsem_release(sb->s_writers.rw_sem + level, 0,
> +                               _THIS_IP_);

If you really must introduce a new freeze level, you should do it in vfs
and not inside xfs, even if xfs is the only current user of the new leve.

Thanks,
Amir.

Re: [PATCH v1 2/4] xfs: implement custom freeze/thaw functions

[Darrick J. Wong]

On Tue, Mar 14, 2023 at 07:11:56AM +0200, Amir Goldstein wrote:
> On Tue, Mar 14, 2023 at 6:25 AM Catherine Hoang
> <catherine.hoang@oracle.com> wrote:
> >
> > Implement internal freeze/thaw functions and prevent other threads from changing
> > the freeze level by adding a new SB_FREEZE_ECLUSIVE level. This is required to
> 
> This looks troubling in several ways:
> - Layering violation
> - Duplication of subtle vfs code
> 
> > prevent concurrent transactions while we are updating the uuid.
> >
> 
> Wouldn't it be easier to hold s_umount while updating the uuid?

Why?  Userspace holds an open file descriptor, the fs won't get
unmounted.

> Let userspace freeze before XFS_IOC_SETFSUUID and let
> XFS_IOC_SETFSUUID take s_umount and verify that fs is frozen.

Ugh, no, I don't want *userspace* to have to know how to do that.

> > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > ---
> >  fs/xfs/xfs_super.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_super.h |   5 ++
> >  2 files changed, 117 insertions(+)
> >
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 2479b5cbd75e..6a52ae660810 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2279,6 +2279,118 @@ static inline int xfs_cpu_hotplug_init(void) { return 0; }
> >  static inline void xfs_cpu_hotplug_destroy(void) {}
> >  #endif
> >
> > +/*
> > + * We need to disable all writer threads, which means taking the first two
> > + * freeze levels to put userspace to sleep, and the third freeze level to
> > + * prevent background threads from starting new transactions.  Take one level
> > + * more to prevent other callers from unfreezing the filesystem while we run.
> > + */
> > +int
> > +xfs_internal_freeze(
> > +       struct xfs_mount        *mp)
> > +{
> > +       struct super_block      *sb = mp->m_super;
> > +       int                     level;
> > +       int                     error = 0;
> > +
> > +       /* Wait until we're ready to freeze. */
> > +       down_write(&sb->s_umount);
> > +       while (sb->s_writers.frozen != SB_UNFROZEN) {
> > +               up_write(&sb->s_umount);
> > +               delay(HZ / 10);
> > +               down_write(&sb->s_umount);
> > +       }
> 
> That can easily wait forever, without any task holding any lock.

Indeed, this needs at a bare minimum some kind of fatal_signal_pending
check every time through the loop.

> > +
> > +       if (sb_rdonly(sb)) {
> > +               sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
> > +               goto done;
> > +       }
> > +
> > +       sb->s_writers.frozen = SB_FREEZE_WRITE;
> > +       /* Release s_umount to preserve sb_start_write -> s_umount ordering */
> > +       up_write(&sb->s_umount);
> > +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_WRITE - 1);
> > +       down_write(&sb->s_umount);
> > +
> > +       /* Now we go and block page faults... */
> > +       sb->s_writers.frozen = SB_FREEZE_PAGEFAULT;
> > +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_PAGEFAULT - 1);
> > +
> > +       /*
> > +        * All writers are done so after syncing there won't be dirty data.
> > +        * Let xfs_fs_sync_fs flush dirty data so the VFS won't start writeback
> > +        * and to disable the background gc workers.
> > +        */
> > +       error = sync_filesystem(sb);
> > +       if (error) {
> > +               sb->s_writers.frozen = SB_UNFROZEN;
> > +               for (level = SB_FREEZE_PAGEFAULT - 1; level >= 0; level--)
> > +                       percpu_up_write(sb->s_writers.rw_sem + level);
> > +               wake_up(&sb->s_writers.wait_unfrozen);
> > +               up_write(&sb->s_umount);
> > +               return error;
> > +       }
> > +
> > +       /* Now wait for internal filesystem counter */
> > +       sb->s_writers.frozen = SB_FREEZE_FS;
> > +       percpu_down_write(sb->s_writers.rw_sem + SB_FREEZE_FS - 1);
> > +
> > +       xfs_log_clean(mp);

Hmm... some of these calls really ought to be returning errors.

> > +
> > +       /*
> > +        * To prevent anyone else from unfreezing us, set the VFS freeze
> > +        * level to one higher than FREEZE_COMPLETE.
> > +        */
> > +       sb->s_writers.frozen = SB_FREEZE_EXCLUSIVE;
> > +       for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
> > +               percpu_rwsem_release(sb->s_writers.rw_sem + level, 0,
> > +                               _THIS_IP_);
> 
> If you really must introduce a new freeze level, you should do it in vfs
> and not inside xfs, even if xfs is the only current user of the new leve.

Luis is already trying to do something similar to this.  So far Jan and
I seem to be the only ones who have taken a look at this fs-internal
freeze...

https://lore.kernel.org/linux-fsdevel/20230114003409.1168311-4-mcgrof@kernel.org/

--D

> Thanks,
> Amir.

Re: [PATCH v1 2/4] xfs: implement custom freeze/thaw functions

[Amir Goldstein]

On Tue, Mar 14, 2023 at 7:25 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Mar 14, 2023 at 07:11:56AM +0200, Amir Goldstein wrote:
> > On Tue, Mar 14, 2023 at 6:25 AM Catherine Hoang
> > <catherine.hoang@oracle.com> wrote:
> > >
> > > Implement internal freeze/thaw functions and prevent other threads from changing
> > > the freeze level by adding a new SB_FREEZE_ECLUSIVE level. This is required to
> >
> > This looks troubling in several ways:
> > - Layering violation
> > - Duplication of subtle vfs code
> >
> > > prevent concurrent transactions while we are updating the uuid.
> > >
> >
> > Wouldn't it be easier to hold s_umount while updating the uuid?
>
> Why?  Userspace holds an open file descriptor, the fs won't get
> unmounted.

"Implement internal freeze/thaw functions and prevent other threads
from changing
the freeze level..."

holding s_umount prevents changing freeze levels.

The special thing about the frozen state is that userspace is allowed
to leave the fs frozen without holding any open fd, but because there
is no such requirement from SET_FSUUID I don't understand the need
for a new freeze level.

Thanks,
Amir.

Re: [PATCH v1 2/4] xfs: implement custom freeze/thaw functions

[Darrick J. Wong]

On Tue, Mar 14, 2023 at 08:00:21AM +0200, Amir Goldstein wrote:
> On Tue, Mar 14, 2023 at 7:25 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Mar 14, 2023 at 07:11:56AM +0200, Amir Goldstein wrote:
> > > On Tue, Mar 14, 2023 at 6:25 AM Catherine Hoang
> > > <catherine.hoang@oracle.com> wrote:
> > > >
> > > > Implement internal freeze/thaw functions and prevent other threads from changing
> > > > the freeze level by adding a new SB_FREEZE_ECLUSIVE level. This is required to
> > >
> > > This looks troubling in several ways:
> > > - Layering violation
> > > - Duplication of subtle vfs code
> > >
> > > > prevent concurrent transactions while we are updating the uuid.
> > > >
> > >
> > > Wouldn't it be easier to hold s_umount while updating the uuid?
> >
> > Why?  Userspace holds an open file descriptor, the fs won't get
> > unmounted.
> 
> "Implement internal freeze/thaw functions and prevent other threads
> from changing
> the freeze level..."
> 
> holding s_umount prevents changing freeze levels.
> 
> The special thing about the frozen state is that userspace is allowed
> to leave the fs frozen without holding any open fd, but because there
> is no such requirement from SET_FSUUID I don't understand the need
> for a new freeze level.

Hm.  I think you could be right, since we can certainly run transactions
with s_umount held.  I wonder if we're going to run into problems with
the weird xlog things that fsuuid setting will want, but I guess we have
lockdep on our side, right? ;)

--D

> Thanks,
> Amir.

[PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Catherine Hoang]

Add a new ioctl to set the uuid of a mounted filesystem.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 fs/xfs/libxfs/xfs_fs.h |   1 +
 fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_log.c       |  19 ++++++++
 fs/xfs/xfs_log.h       |   2 +
 4 files changed, 129 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 1cfd5bc6520a..a350966cce99 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
 #define XFS_IOC_FSGEOMETRY	     _IOR ('X', 126, struct xfs_fsop_geom)
 #define XFS_IOC_BULKSTAT	     _IOR ('X', 127, struct xfs_bulkstat_req)
 #define XFS_IOC_INUMBERS	     _IOR ('X', 128, struct xfs_inumbers_req)
+#define XFS_IOC_SETFSUUID	     _IOR ('X', 129, uuid_t)
 /*	XFS_IOC_GETFSUUID ---------- deprecated 140	 */
 
 
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 55bb01173cde..f0699a7169e4 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -38,6 +38,7 @@
 #include "xfs_reflink.h"
 #include "xfs_ioctl.h"
 #include "xfs_xattr.h"
+#include "xfs_log.h"
 
 #include <linux/mount.h>
 #include <linux/namei.h>
@@ -1861,6 +1862,109 @@ xfs_fs_eofblocks_from_user(
 	return 0;
 }
 
+static int
+xfs_ioc_setfsuuid(
+	struct file			*filp,
+	struct xfs_mount		*mp,
+	uuid_t				__user *uuid)
+{
+	uuid_t				old_uuid;
+	uuid_t				new_uuid;
+	uuid_t				*forget_uuid = NULL;
+	int				error;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!xfs_sb_is_v5(&mp->m_sb))
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&new_uuid, uuid, sizeof(uuid_t)))
+		return -EFAULT;
+	if (uuid_is_null(&new_uuid))
+		return -EINVAL;
+
+	/* Check that the uuid is unique and save a slot in the uuid table. */
+	if (!(xfs_has_nouuid(mp))) {
+		error = xfs_uuid_remember(&new_uuid);
+		if (error)
+			return error;
+		forget_uuid = &new_uuid;
+	}
+
+	error = xfs_internal_freeze(mp);
+	if (error)
+		goto out_drop_uuid;
+
+	spin_lock(&mp->m_sb_lock);
+	uuid_copy(&old_uuid, &mp->m_sb.sb_uuid);
+
+	/*
+	 * On a v5 filesystem, every metadata object has a uuid stamped into
+	 * the header.  The particular uuid used is either sb_uuid or
+	 * sb_meta_uuid, depending on whether the meta_uuid feature is set.
+	 *
+	 * If the meta_uuid feature is set:
+	 * - The user visible uuid is set in sb_uuid
+	 * - The uuid used for metadata blocks is set in sb_meta_uuid
+	 * - If new_uuid == sb_meta_uuid, then we'll deactivate the feature
+	 *   and set sb_uuid to the new uuid
+	 *
+	 * If the meta_uuid feature is not set:
+	 * - The user visible uuid is set in sb_uuid
+	 * - The uuid used for meta blocks should match sb_uuid
+	 * - If new_uuid != sb_uuid, we need to copy sb_uuid to sb_meta_uuid,
+	 *   set the meta_uuid feature bit, and set sb_uuid to the new uuid
+	 */
+	if (xfs_has_metauuid(mp) &&
+	    uuid_equal(&new_uuid, &mp->m_sb.sb_meta_uuid)) {
+		mp->m_sb.sb_features_incompat &= ~XFS_SB_FEAT_INCOMPAT_META_UUID;
+		mp->m_features &= ~XFS_FEAT_META_UUID;
+	} else if (!xfs_has_metauuid(mp) &&
+	    !uuid_equal(&new_uuid, &mp->m_sb.sb_uuid)) {
+		uuid_copy(&mp->m_sb.sb_meta_uuid, &mp->m_sb.sb_uuid);
+		mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_META_UUID;
+		mp->m_features |= XFS_FEAT_META_UUID;
+	}
+
+	uuid_copy(&mp->m_sb.sb_uuid, &new_uuid);
+	spin_unlock(&mp->m_sb_lock);
+
+	xlog_iclog_update_uuid(mp);
+
+	xfs_buf_lock(mp->m_sb_bp);
+	xfs_buf_hold(mp->m_sb_bp);
+
+	xfs_sb_to_disk(mp->m_sb_bp->b_addr, &mp->m_sb);
+	error = xfs_bwrite(mp->m_sb_bp);
+	xfs_buf_relse(mp->m_sb_bp);
+	if (error)
+		goto out_drop_freeze;
+
+	/* Update incore state and prepare to drop the old uuid. */
+	uuid_copy(&mp->m_super->s_uuid, &new_uuid);
+	if (!(xfs_has_nouuid(mp)))
+		forget_uuid = &old_uuid;
+
+	/*
+	 * Update the secondary supers, being aware that growfs also updates
+	 * backup supers so we need to lock against that.
+	 */
+	mutex_lock(&mp->m_growlock);
+	error = xfs_update_secondary_sbs(mp);
+	mutex_unlock(&mp->m_growlock);
+
+	invalidate_bdev(mp->m_ddev_targp->bt_bdev);
+	xfs_log_clean(mp);
+
+out_drop_freeze:
+	xfs_internal_unfreeze(mp);
+out_drop_uuid:
+	if (forget_uuid)
+		xfs_uuid_forget(forget_uuid);
+	return error;
+}
+
 /*
  * These long-unused ioctls were removed from the official ioctl API in 5.17,
  * but retain these definitions so that we can log warnings about them.
@@ -2149,6 +2253,9 @@ xfs_file_ioctl(
 		return error;
 	}
 
+	case XFS_IOC_SETFSUUID:
+		return xfs_ioc_setfsuuid(filp, mp, arg);
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index fc61cc024023..d79b6065ee9c 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3921,3 +3921,22 @@ xlog_drop_incompat_feat(
 {
 	up_read(&log->l_incompat_users);
 }
+
+/*
+ * Cycle all the iclog buffers and update the uuid.
+ */
+void
+xlog_iclog_update_uuid(
+	struct xfs_mount	*mp)
+{
+	int			i;
+	struct xlog		*log = mp->m_log;
+	struct xlog_in_core	*iclog = log->l_iclog;
+	xlog_rec_header_t	*head;
+
+	for (i = 0; i < log->l_iclog_bufs; i++) {
+		head = &iclog->ic_header;
+		memcpy(&head->h_fs_uuid, &mp->m_sb.sb_uuid, sizeof(uuid_t));
+		iclog = iclog->ic_next;
+	}
+}
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 2728886c2963..6b607619163e 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -163,4 +163,6 @@ void xlog_use_incompat_feat(struct xlog *log);
 void xlog_drop_incompat_feat(struct xlog *log);
 int xfs_attr_use_log_assist(struct xfs_mount *mp);
 
+void xlog_iclog_update_uuid(struct xfs_mount *mp);
+
 #endif	/* __XFS_LOG_H__ */
-- 
2.34.1

Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Amir Goldstein]

On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
<catherine.hoang@oracle.com> wrote:
>
> Add a new ioctl to set the uuid of a mounted filesystem.
>
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_fs.h |   1 +
>  fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_log.c       |  19 ++++++++
>  fs/xfs/xfs_log.h       |   2 +
>  4 files changed, 129 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index 1cfd5bc6520a..a350966cce99 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
>  #define XFS_IOC_FSGEOMETRY          _IOR ('X', 126, struct xfs_fsop_geom)
>  #define XFS_IOC_BULKSTAT            _IOR ('X', 127, struct xfs_bulkstat_req)
>  #define XFS_IOC_INUMBERS            _IOR ('X', 128, struct xfs_inumbers_req)
> +#define XFS_IOC_SETFSUUID           _IOR ('X', 129, uuid_t)

Should be _IOW.

Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
so that other fs could implement it later on, instead of hoisting it later?

It would be easy to add support for FS_IOC_SETFSUUID to ext4
by generalizing ext4_ioctl_setuuid().

Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
to fs.h and use that ioctl also for xfs.

Using an extensible struct with flags for that ioctl may turn out to be useful,
for example, to verify that the new uuid is unique, despite the fact
that xfs was
mounted with -onouuid (useful IMO) or to explicitly request a restore of old
uuid that would fail if new_uuid != meta uuid.

Thanks,
Amir.

Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Catherine Hoang]

> On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> <catherine.hoang@oracle.com> wrote:
>> 
>> Add a new ioctl to set the uuid of a mounted filesystem.
>> 
>> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_fs.h |   1 +
>> fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
>> fs/xfs/xfs_log.c       |  19 ++++++++
>> fs/xfs/xfs_log.h       |   2 +
>> 4 files changed, 129 insertions(+)
>> 
>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> index 1cfd5bc6520a..a350966cce99 100644
>> --- a/fs/xfs/libxfs/xfs_fs.h
>> +++ b/fs/xfs/libxfs/xfs_fs.h
>> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
>> #define XFS_IOC_FSGEOMETRY          _IOR ('X', 126, struct xfs_fsop_geom)
>> #define XFS_IOC_BULKSTAT            _IOR ('X', 127, struct xfs_bulkstat_req)
>> #define XFS_IOC_INUMBERS            _IOR ('X', 128, struct xfs_inumbers_req)
>> +#define XFS_IOC_SETFSUUID           _IOR ('X', 129, uuid_t)
> 
> Should be _IOW.

Ok, will fix that.
> 
> Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> so that other fs could implement it later on, instead of hoisting it later?
> 
> It would be easy to add support for FS_IOC_SETFSUUID to ext4
> by generalizing ext4_ioctl_setuuid().
> 
> Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> to fs.h and use that ioctl also for xfs.

I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
to a consensus on the implementation.

https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@oracle.com/

I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
fsdevel bikeshedding.
> 
> Using an extensible struct with flags for that ioctl may turn out to be useful,
> for example, to verify that the new uuid is unique, despite the fact
> that xfs was
> mounted with -onouuid (useful IMO) or to explicitly request a restore of old
> uuid that would fail if new_uuid != meta uuid.

I think using a struct is probably a good idea, I can add that in the next version.
> 
> Thanks,
> Amir.

Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Amir Goldstein]

On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
<catherine.hoang@oracle.com> wrote:
>
> > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> > <catherine.hoang@oracle.com> wrote:
> >>
> >> Add a new ioctl to set the uuid of a mounted filesystem.
> >>
> >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> >> ---
> >> fs/xfs/libxfs/xfs_fs.h |   1 +
> >> fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
> >> fs/xfs/xfs_log.c       |  19 ++++++++
> >> fs/xfs/xfs_log.h       |   2 +
> >> 4 files changed, 129 insertions(+)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> >> index 1cfd5bc6520a..a350966cce99 100644
> >> --- a/fs/xfs/libxfs/xfs_fs.h
> >> +++ b/fs/xfs/libxfs/xfs_fs.h
> >> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
> >> #define XFS_IOC_FSGEOMETRY          _IOR ('X', 126, struct xfs_fsop_geom)
> >> #define XFS_IOC_BULKSTAT            _IOR ('X', 127, struct xfs_bulkstat_req)
> >> #define XFS_IOC_INUMBERS            _IOR ('X', 128, struct xfs_inumbers_req)
> >> +#define XFS_IOC_SETFSUUID           _IOR ('X', 129, uuid_t)
> >
> > Should be _IOW.
>
> Ok, will fix that.
> >
> > Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> > so that other fs could implement it later on, instead of hoisting it later?
> >
> > It would be easy to add support for FS_IOC_SETFSUUID to ext4
> > by generalizing ext4_ioctl_setuuid().
> >
> > Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> > to fs.h and use that ioctl also for xfs.
>
> I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
> to a consensus on the implementation.
>
> https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@oracle.com/
>
> I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
> fsdevel bikeshedding.

For the greater good, please do try to have this bikeshedding, before giving up.
The discussion you pointed to wasn't so far from consensus IMO except
fsdevel was not CCed.

> >
> > Using an extensible struct with flags for that ioctl may turn out to be useful,
> > for example, to verify that the new uuid is unique, despite the fact
> > that xfs was
> > mounted with -onouuid (useful IMO) or to explicitly request a restore of old
> > uuid that would fail if new_uuid != meta uuid.
>
> I think using a struct is probably a good idea, I can add that in the next version.

Well, if you agree about a struct and agree about flags then the only thing
left is Dave's concern about variable size arrays in ioctl and that could be
addressed in a way that is compatible with ext4.

See untested patch below.

Thanks,
Amir.

diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7b56871029c..143a4735486e 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -215,6 +215,17 @@ struct fsxattr {
 #define FS_IOC_FSSETXATTR              _IOW('X', 32, struct fsxattr)
 #define FS_IOC_GETFSLABEL              _IOR(0x94, 49, char[FSLABEL_MAX])
 #define FS_IOC_SETFSLABEL              _IOW(0x94, 50, char[FSLABEL_MAX])
+#define FS_IOC_GETFSUUID               _IOR('v', 44, struct fsuuid)
+#define FS_IOC_SETFSUUID               _IOW('v', 44, struct fsuuid)
+
+/*
+ * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
+ */
+struct fsuuid {
+       __u32   fsu_len; /* for backward compat has to be 16 */
+       __u32   fsu_flags;
+       __u8    fsu_uuid[16];
+};

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 140e1eb300d1..c4ded5d5e421 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -722,8 +722,8 @@ enum {
 #define EXT4_IOC_GETSTATE              _IOW('f', 41, __u32)
 #define EXT4_IOC_GET_ES_CACHE          _IOWR('f', 42, struct fiemap)
 #define EXT4_IOC_CHECKPOINT            _IOW('f', 43, __u32)
-#define EXT4_IOC_GETFSUUID             _IOR('f', 44, struct fsuuid)
-#define EXT4_IOC_SETFSUUID             _IOW('f', 44, struct fsuuid)
+#define EXT4_IOC_GETFSUUID             _IOR('f', 44, struct fsuuid_bdr)
+#define EXT4_IOC_SETFSUUID             _IOW('f', 44, struct fsuuid_hdr)

 #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)

@@ -756,7 +756,7 @@ enum {
 /*
  * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
  */
-struct fsuuid {
+struct fsuuid_hdr {
        __u32       fsu_len;
        __u32       fsu_flags;
        __u8        fsu_uuid[];

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 8067ccda34e4..fc744231ad24 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1149,7 +1149,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
        struct fsuuid fsuuid;
        __u8 uuid[UUID_SIZE];

-       if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+       if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
                return -EFAULT;

        if (fsuuid.fsu_len == 0) {
@@ -1168,7 +1168,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
        unlock_buffer(sbi->s_sbh);

        fsuuid.fsu_len = UUID_SIZE;
-       if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
+       if (copy_to_user(ufsuuid, &fsuuid, offsetof(fsuuid, fsu_uuid)) ||
            copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
                return -EFAULT;
        return 0;
@@ -1194,7 +1194,7 @@ static int ext4_ioctl_setuuid(struct file *filp,
                || ext4_has_feature_stable_inodes(sb))
                return -EOPNOTSUPP;

-       if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
+       if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
                return -EFAULT;

        if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
@@ -1596,8 +1596,10 @@ static long __ext4_ioctl(struct file *filp,
unsigned int cmd, unsigned long arg)
                return ext4_ioctl_setlabel(filp,
                                           (const void __user *)arg);

+       case FS_IOC_GETFSUUID:
        case EXT4_IOC_GETFSUUID:
                return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg);
+       case FS_IOC_SETFSUUID:
        case EXT4_IOC_SETFSUUID:
                return ext4_ioctl_setuuid(filp, (const void __user *)arg);
        default:

Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Darrick J. Wong]

On Thu, Mar 16, 2023 at 10:09:56AM +0200, Amir Goldstein wrote:
> On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
> <catherine.hoang@oracle.com> wrote:
> >
> > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> > > <catherine.hoang@oracle.com> wrote:
> > >>
> > >> Add a new ioctl to set the uuid of a mounted filesystem.
> > >>
> > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > >> ---
> > >> fs/xfs/libxfs/xfs_fs.h |   1 +
> > >> fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
> > >> fs/xfs/xfs_log.c       |  19 ++++++++
> > >> fs/xfs/xfs_log.h       |   2 +
> > >> 4 files changed, 129 insertions(+)
> > >>
> > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > >> index 1cfd5bc6520a..a350966cce99 100644
> > >> --- a/fs/xfs/libxfs/xfs_fs.h
> > >> +++ b/fs/xfs/libxfs/xfs_fs.h
> > >> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
> > >> #define XFS_IOC_FSGEOMETRY          _IOR ('X', 126, struct xfs_fsop_geom)
> > >> #define XFS_IOC_BULKSTAT            _IOR ('X', 127, struct xfs_bulkstat_req)
> > >> #define XFS_IOC_INUMBERS            _IOR ('X', 128, struct xfs_inumbers_req)
> > >> +#define XFS_IOC_SETFSUUID           _IOR ('X', 129, uuid_t)
> > >
> > > Should be _IOW.
> >
> > Ok, will fix that.
> > >
> > > Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> > > so that other fs could implement it later on, instead of hoisting it later?
> > >
> > > It would be easy to add support for FS_IOC_SETFSUUID to ext4
> > > by generalizing ext4_ioctl_setuuid().
> > >
> > > Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> > > to fs.h and use that ioctl also for xfs.
> >
> > I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
> > to a consensus on the implementation.
> >
> > https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@oracle.com/
> >
> > I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
> > fsdevel bikeshedding.
> 
> For the greater good, please do try to have this bikeshedding, before giving up.
> The discussion you pointed to wasn't so far from consensus IMO except
> fsdevel was not CCed.

Why?  fsdevel bikeshedding is a pointless waste of time.  Jeremy ran
four rounds of proposing the new api on linux-api, linux-fsdevel, and
linux-ext4.  Matthew Wilcox and I sent in our comments, including adding
some flexibility for shorter or longer uuids, so he updated the proposal
and it got merged:

https://lore.kernel.org/linux-api/?q=Bongio

The instant Catherine started talking about using this new API, Dave
came in and said no, flex arrays for uuids are stupid, and told
Catherine she ought to "fix" the landmines by changing the structure
definition:

https://lore.kernel.org/linux-xfs/20221121211437.GK3600936@dread.disaster.area/ 

Never mind that changing the struct size causes the output of _IOR to
change, which means a new ioctl command number, which is effectively a
new interface.  I think we'll just put new ioctls in xfs_fs_staging.h,
merge the code, let people kick the tires for a few months, and only
then make it permanent.

Though really, that's the least of the problems, especially since Dave
had a pretty good list of questions elsewhere in this thread.

--D

> > >
> > > Using an extensible struct with flags for that ioctl may turn out to be useful,
> > > for example, to verify that the new uuid is unique, despite the fact
> > > that xfs was
> > > mounted with -onouuid (useful IMO) or to explicitly request a restore of old
> > > uuid that would fail if new_uuid != meta uuid.
> >
> > I think using a struct is probably a good idea, I can add that in the next version.
> 
> Well, if you agree about a struct and agree about flags then the only thing
> left is Dave's concern about variable size arrays in ioctl and that could be
> addressed in a way that is compatible with ext4.
> 
> See untested patch below.
> 
> Thanks,
> Amir.
> 
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index b7b56871029c..143a4735486e 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -215,6 +215,17 @@ struct fsxattr {
>  #define FS_IOC_FSSETXATTR              _IOW('X', 32, struct fsxattr)
>  #define FS_IOC_GETFSLABEL              _IOR(0x94, 49, char[FSLABEL_MAX])
>  #define FS_IOC_SETFSLABEL              _IOW(0x94, 50, char[FSLABEL_MAX])
> +#define FS_IOC_GETFSUUID               _IOR('v', 44, struct fsuuid)
> +#define FS_IOC_SETFSUUID               _IOW('v', 44, struct fsuuid)
> +
> +/*
> + * Structure for FS_IOC_GETFSUUID/FS_IOC_SETFSUUID
> + */
> +struct fsuuid {
> +       __u32   fsu_len; /* for backward compat has to be 16 */
> +       __u32   fsu_flags;
> +       __u8    fsu_uuid[16];

Um, these two ioctls have different namespace /and/ different structure
sizes.  This is literally defining a new interface.

> +};
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 140e1eb300d1..c4ded5d5e421 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -722,8 +722,8 @@ enum {
>  #define EXT4_IOC_GETSTATE              _IOW('f', 41, __u32)
>  #define EXT4_IOC_GET_ES_CACHE          _IOWR('f', 42, struct fiemap)
>  #define EXT4_IOC_CHECKPOINT            _IOW('f', 43, __u32)
> -#define EXT4_IOC_GETFSUUID             _IOR('f', 44, struct fsuuid)
> -#define EXT4_IOC_SETFSUUID             _IOW('f', 44, struct fsuuid)
> +#define EXT4_IOC_GETFSUUID             _IOR('f', 44, struct fsuuid_bdr)
> +#define EXT4_IOC_SETFSUUID             _IOW('f', 44, struct fsuuid_hdr)
> 
>  #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
> 
> @@ -756,7 +756,7 @@ enum {
>  /*
>   * Structure for EXT4_IOC_GETFSUUID/EXT4_IOC_SETFSUUID
>   */
> -struct fsuuid {
> +struct fsuuid_hdr {
>         __u32       fsu_len;
>         __u32       fsu_flags;
>         __u8        fsu_uuid[];
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 8067ccda34e4..fc744231ad24 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1149,7 +1149,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
>         struct fsuuid fsuuid;
>         __u8 uuid[UUID_SIZE];
> 
> -       if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +       if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
>                 return -EFAULT;
> 
>         if (fsuuid.fsu_len == 0) {
> @@ -1168,7 +1168,7 @@ static int ext4_ioctl_getuuid(struct ext4_sb_info *sbi,
>         unlock_buffer(sbi->s_sbh);
> 
>         fsuuid.fsu_len = UUID_SIZE;
> -       if (copy_to_user(ufsuuid, &fsuuid, sizeof(fsuuid)) ||
> +       if (copy_to_user(ufsuuid, &fsuuid, offsetof(fsuuid, fsu_uuid)) ||
>             copy_to_user(&ufsuuid->fsu_uuid[0], uuid, UUID_SIZE))
>                 return -EFAULT;
>         return 0;
> @@ -1194,7 +1194,7 @@ static int ext4_ioctl_setuuid(struct file *filp,
>                 || ext4_has_feature_stable_inodes(sb))
>                 return -EOPNOTSUPP;
> 
> -       if (copy_from_user(&fsuuid, ufsuuid, sizeof(fsuuid)))
> +       if (copy_from_user(&fsuuid, ufsuuid, offsetof(fsuuid, fsu_uuid)))
>                 return -EFAULT;
> 
>         if (fsuuid.fsu_len != UUID_SIZE || fsuuid.fsu_flags != 0)
> @@ -1596,8 +1596,10 @@ static long __ext4_ioctl(struct file *filp,
> unsigned int cmd, unsigned long arg)
>                 return ext4_ioctl_setlabel(filp,
>                                            (const void __user *)arg);
> 
> +       case FS_IOC_GETFSUUID:
>         case EXT4_IOC_GETFSUUID:
>                 return ext4_ioctl_getuuid(EXT4_SB(sb), (void __user *)arg);
> +       case FS_IOC_SETFSUUID:
>         case EXT4_IOC_SETFSUUID:
>                 return ext4_ioctl_setuuid(filp, (const void __user *)arg);
>         default:

Re: [PATCH v1 3/4] xfs: add XFS_IOC_SETFSUUID ioctl

[Amir Goldstein]

On Sat, Mar 18, 2023 at 2:39 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Mar 16, 2023 at 10:09:56AM +0200, Amir Goldstein wrote:
> > On Thu, Mar 16, 2023 at 1:13 AM Catherine Hoang
> > <catherine.hoang@oracle.com> wrote:
> > >
> > > > On Mar 13, 2023, at 10:50 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Mar 14, 2023 at 6:27 AM Catherine Hoang
> > > > <catherine.hoang@oracle.com> wrote:
> > > >>
> > > >> Add a new ioctl to set the uuid of a mounted filesystem.
> > > >>
> > > >> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > > >> ---
> > > >> fs/xfs/libxfs/xfs_fs.h |   1 +
> > > >> fs/xfs/xfs_ioctl.c     | 107 +++++++++++++++++++++++++++++++++++++++++
> > > >> fs/xfs/xfs_log.c       |  19 ++++++++
> > > >> fs/xfs/xfs_log.h       |   2 +
> > > >> 4 files changed, 129 insertions(+)
> > > >>
> > > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > >> index 1cfd5bc6520a..a350966cce99 100644
> > > >> --- a/fs/xfs/libxfs/xfs_fs.h
> > > >> +++ b/fs/xfs/libxfs/xfs_fs.h
> > > >> @@ -831,6 +831,7 @@ struct xfs_scrub_metadata {
> > > >> #define XFS_IOC_FSGEOMETRY          _IOR ('X', 126, struct xfs_fsop_geom)
> > > >> #define XFS_IOC_BULKSTAT            _IOR ('X', 127, struct xfs_bulkstat_req)
> > > >> #define XFS_IOC_INUMBERS            _IOR ('X', 128, struct xfs_inumbers_req)
> > > >> +#define XFS_IOC_SETFSUUID           _IOR ('X', 129, uuid_t)
> > > >
> > > > Should be _IOW.
> > >
> > > Ok, will fix that.
> > > >
> > > > Would you consider defining that as FS_IOC_SETFSUUID in fs.h,
> > > > so that other fs could implement it later on, instead of hoisting it later?
> > > >
> > > > It would be easy to add support for FS_IOC_SETFSUUID to ext4
> > > > by generalizing ext4_ioctl_setuuid().
> > > >
> > > > Alternatively, we could hoist EXT4_IOC_SETFSUUID and struct fsuuid
> > > > to fs.h and use that ioctl also for xfs.
> > >
> > > I actually did try to hoist the ext4 ioctls previously, but we weren’t able to come
> > > to a consensus on the implementation.
> > >
> > > https://lore.kernel.org/linux-xfs/20221118211408.72796-2-catherine.hoang@oracle.com/
> > >
> > > I would prefer to keep this defined as an xfs specific ioctl to avoid all of the
> > > fsdevel bikeshedding.
> >
> > For the greater good, please do try to have this bikeshedding, before giving up.
> > The discussion you pointed to wasn't so far from consensus IMO except
> > fsdevel was not CCed.
>
> Why?  fsdevel bikeshedding is a pointless waste of time.  Jeremy ran

[+ linux-api]

I do not think that it was a waste of time at all...

> four rounds of proposing the new api on linux-api, linux-fsdevel, and
> linux-ext4.  Matthew Wilcox and I sent in our comments, including adding
> some flexibility for shorter or longer uuids, so he updated the proposal
> and it got merged:
>
> https://lore.kernel.org/linux-api/?q=Bongio
>
> The instant Catherine started talking about using this new API, Dave
> came in and said no, flex arrays for uuids are stupid, and told
> Catherine she ought to "fix" the landmines by changing the structure
> definition:
>
> https://lore.kernel.org/linux-xfs/20221121211437.GK3600936@dread.disaster.area/
>
> Never mind that changing the struct size causes the output of _IOR to
> change, which means a new ioctl command number, which is effectively a
> new interface.  I think we'll just put new ioctls in xfs_fs_staging.h,
> merge the code, let people kick the tires for a few months, and only
> then make it permanent.
>

What you perceive as a waste of time, I perceive as a healthy process.
This is what I see when reading the threads:

- Serious and responsible API discussion with several important review
  inputs incorporated.
- New API was added as "staging" in ext4 only
- Less than 1 year later, another fs wants to use the new API
- Dave (who may have missed the original API discussion?) points out a problem
- In my understanding, in the original discussion there was a consensus
  that uuid size is limited to 16 bytes [1] so the fact that fsu_uuid[]
  ended up as a variable array is just a human mistake?

[1] https://lore.kernel.org/linux-api/YthI9qp+VeNbFQP3@casper.infradead.org/

So we had a design discussion, we had a staging API and we found a problem
in the staging API. This is what I call a healthy process.

When that happens it is not too late to fix the API problem.
and the fix is simple - increase the size of the struct to maximal uuid size
and change the ioctl numbers.

Yes, ext4 tools and kernel driver have to pay the penalty of backward compat
support for the V1 API, but as I showed in the sketch patch, that will be
quite easy to do and that is the price that one has to pay for being a
pioneer ;)

So for a workplan, Catherine can use the extended fsuuid struct and add it to
staging in xfs as you proposed, with the intention of hoisting it to
fs.h later on.
And that plan should be published to linux-api and linux-fsdevel.

Assuming that ext4 developers are fine with this plan, they could already
adjust ext4 and e2fsprogs to the V2 API before being hoisted, because the
adjustments are pretty simple.

Thanks,
Amir.

[PATCH v1 4/4] xfs: export meta uuid via xfs_fsop_geom

[Catherine Hoang]

Export the meta uuid to userspace so that we can restore the original uuid after
it has been changed.

Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
---
 fs/xfs/libxfs/xfs_fs.h | 3 ++-
 fs/xfs/libxfs/xfs_sb.c | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index a350966cce99..d88adaf9369f 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -186,7 +186,8 @@ struct xfs_fsop_geom {
 	__u32		logsunit;	/* log stripe unit, bytes	*/
 	uint32_t	sick;		/* o: unhealthy fs & rt metadata */
 	uint32_t	checked;	/* o: checked fs & rt metadata	*/
-	__u64		reserved[17];	/* reserved space		*/
+	unsigned char	metauuid[16];	/* metadata id of the filesystem*/
+	__u64		reserved[15];	/* reserved space		*/
 };
 
 #define XFS_FSOP_GEOM_SICK_COUNTERS	(1 << 0)  /* summary counters */
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 99cc03a298e2..4c24f3314122 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1213,6 +1213,11 @@ xfs_fs_geometry(
 		return;
 
 	geo->version = XFS_FSOP_GEOM_VERSION_V5;
+
+	if (xfs_has_metauuid(mp)) {
+		BUILD_BUG_ON(sizeof(geo->metauuid) != sizeof(sbp->sb_meta_uuid));
+		memcpy(geo->metauuid, &sbp->sb_meta_uuid, sizeof(sbp->sb_meta_uuid));
+	}
 }
 
 /* Read a secondary superblock. */
-- 
2.34.1

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Dave Chinner]

On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
> Hi all,
> 
> This series of patches implements a new ioctl to set the uuid of mounted
> filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
> to allow userspace to update the uuid.
> 
> Comments and feedback appreciated!

What's the use case for this?

What are the pro's and cons of the implementation?

Some problems I see:

* How does this interact with pNFS exports (i.e.
CONFIG_EXPORTFS_BLOCK_OPS). XFS hands the sb_uuid directly to pNFS
server (and remote clients, I think) so that incoming mapping
requests can be directed to the correct block device hosting the XFS
filesystem the mapping information is for. IIRC, the pNFS data path
is just given a byte offset into the device where the UUID in the
superblock lives, and if it matches it allows the remote IO to go
ahead - it doesn't actually know that there is a filesystem on that
device at all...

* IIRC, the nfsd export table can also use the filesystem uuid to
identify the filesystem being exported, and IIRC that gets encoded
in the filehandle. Does changing the filesystem UUID then cause
problems with export indentification and/or file handle
creation/resolution?

* Is the VFS prepared for sb->s_uuid changing underneath running
operations on mounted filesystems? I can see that this might cause
problems with any sort of fscrypt implementation as it may encode
the s_uuid into encryption keys held in xattrs, similarly IMA and
EVM integrity protection keys.

* Should the VFS superblock sb->s_uuid use the XFS
sb->sb_meta_uuid value and never change because we can encode it
into other objects stored in the active filesystem? What does that
mean for tools that identify block devices or filesystems by the VFS
uuid rather than the filesystem uuid?

There's a whole can-o-worms here - the ability to dynamically change
the UUID of a filesystem while it is mounted mean we need to think
about these things - it's no longer as simple as "can only do it
offline" which is typically only used to relabel a writeable
snapshot of a golden image file during new VM deployment
preparation.....

> 
> Catherine
> 
> Catherine Hoang (4):
>   xfs: refactor xfs_uuid_mount and xfs_uuid_unmount
>   xfs: implement custom freeze/thaw functions

The "custom" parts that XFS requires need to be added to the VFS
level freeze/thaw functions, not duplicate the VFS functionality
within XFS and then slight modify it for this additional feature.
Doing this results in unmaintainable code over the long term.

>   xfs: add XFS_IOC_SETFSUUID ioctl
>   xfs: export meta uuid via xfs_fsop_geom

For what purpose does userspace ever need to know the sb_meta_uuid?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Catherine Hoang]

> On Mar 13, 2023, at 11:28 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
>> Hi all,
>> 
>> This series of patches implements a new ioctl to set the uuid of mounted
>> filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
>> to allow userspace to update the uuid.
>> 
>> Comments and feedback appreciated!
> 
> What's the use case for this?

We want to be able to change the uuid on newly mounted clone vm images
so that each deployed system has a different uuid. We need to do this the
first time the system boots, but after the root fs is mounted so that fsuuid
can run in parallel with other service startup to minimize deployment times.
> 
> What are the pro's and cons of the implementation?
> 
> Some problems I see:
> 
> * How does this interact with pNFS exports (i.e.
> CONFIG_EXPORTFS_BLOCK_OPS). XFS hands the sb_uuid directly to pNFS
> server (and remote clients, I think) so that incoming mapping
> requests can be directed to the correct block device hosting the XFS
> filesystem the mapping information is for. IIRC, the pNFS data path
> is just given a byte offset into the device where the UUID in the
> superblock lives, and if it matches it allows the remote IO to go
> ahead - it doesn't actually know that there is a filesystem on that
> device at all...
> 
> * IIRC, the nfsd export table can also use the filesystem uuid to
> identify the filesystem being exported, and IIRC that gets encoded
> in the filehandle. Does changing the filesystem UUID then cause
> problems with export indentification and/or file handle
> creation/resolution?
> 
> * Is the VFS prepared for sb->s_uuid changing underneath running
> operations on mounted filesystems? I can see that this might cause
> problems with any sort of fscrypt implementation as it may encode
> the s_uuid into encryption keys held in xattrs, similarly IMA and
> EVM integrity protection keys.
> 
> * Should the VFS superblock sb->s_uuid use the XFS
> sb->sb_meta_uuid value and never change because we can encode it
> into other objects stored in the active filesystem? What does that
> mean for tools that identify block devices or filesystems by the VFS
> uuid rather than the filesystem uuid?
> 
> There's a whole can-o-worms here - the ability to dynamically change
> the UUID of a filesystem while it is mounted mean we need to think
> about these things - it's no longer as simple as "can only do it
> offline" which is typically only used to relabel a writeable
> snapshot of a golden image file during new VM deployment
> preparation.....
> 
>> 
>> Catherine
>> 
>> Catherine Hoang (4):
>>  xfs: refactor xfs_uuid_mount and xfs_uuid_unmount
>>  xfs: implement custom freeze/thaw functions
> 
> The "custom" parts that XFS requires need to be added to the VFS
> level freeze/thaw functions, not duplicate the VFS functionality
> within XFS and then slight modify it for this additional feature.
> Doing this results in unmaintainable code over the long term.
> 
>>  xfs: add XFS_IOC_SETFSUUID ioctl
>>  xfs: export meta uuid via xfs_fsop_geom
> 
> For what purpose does userspace ever need to know the sb_meta_uuid?

Userspace would need to know the meta uuid if we want to restore
the original uuid after it has been changed.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Dave Chinner]

On Thu, Mar 16, 2023 at 08:41:14PM +0000, Catherine Hoang wrote:
> > On Mar 13, 2023, at 11:28 PM, Dave Chinner <david@fromorbit.com> wrote:
> > 
> > On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
> >> Hi all,
> >> 
> >> This series of patches implements a new ioctl to set the uuid of mounted
> >> filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
> >> to allow userspace to update the uuid.
> >> 
> >> Comments and feedback appreciated!
> > 
> > What's the use case for this?
> 
> We want to be able to change the uuid on newly mounted clone vm images
> so that each deployed system has a different uuid. We need to do this the
> first time the system boots, but after the root fs is mounted so that fsuuid
> can run in parallel with other service startup to minimize deployment times.

Why can't you do it offline immediately after the offline clone of
the golden image? I mean, cloning images and setting up their
contents is something the external orchestration software does
and will always have to do, so i don't really understand why UUID
needs to be modified at first mount vs at clone time. Can you
describe why it actually needs to be done after first mount?

> >>  xfs: add XFS_IOC_SETFSUUID ioctl
> >>  xfs: export meta uuid via xfs_fsop_geom
> > 
> > For what purpose does userspace ever need to know the sb_meta_uuid?
> 
> Userspace would need to know the meta uuid if we want to restore
> the original uuid after it has been changed.

I don't understand why you'd want to restore the original UUID given
the use case you've describe. Can you explain the situation where
you want to return a cloned image to the original golden image UUID?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Darrick J. Wong]

On Sun, Mar 19, 2023 at 11:16:48AM +1100, Dave Chinner wrote:
> On Thu, Mar 16, 2023 at 08:41:14PM +0000, Catherine Hoang wrote:
> > > On Mar 13, 2023, at 11:28 PM, Dave Chinner <david@fromorbit.com> wrote:
> > > 
> > > On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
> > >> Hi all,
> > >> 
> > >> This series of patches implements a new ioctl to set the uuid of mounted
> > >> filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
> > >> to allow userspace to update the uuid.
> > >> 
> > >> Comments and feedback appreciated!
> > > 
> > > What's the use case for this?
> > 
> > We want to be able to change the uuid on newly mounted clone vm images
> > so that each deployed system has a different uuid. We need to do this the
> > first time the system boots, but after the root fs is mounted so that fsuuid
> > can run in parallel with other service startup to minimize deployment times.
> 
> Why can't you do it offline immediately after the offline clone of
> the golden image? I mean, cloning images and setting up their

That /is/ how they do it currently.  The goal here was to reduce the
number of scripts that then must be worked into every distro's bespoke
initrd generation system (update-initramfs, dracut, etc.).  Doing this
from systemd bypasses that, and it means it can get done in the
background during first boot, instead of rewriting superblocks in the
singlethreaded initramfs hot path during deployments.

They need the fsuuid to change after the VM starts up, but it doesn't
have to get done until just before we start handing out NFS leases.
Granted that was before we hit the wall of "lots of subsystems encode
the fs uuid into things and hand them out"...

> contents is something the external orchestration software does
> and will always have to do, so i don't really understand why UUID
> needs to be modified at first mount vs at clone time. Can you
> describe why it actually needs to be done after first mount?
> 
> > >>  xfs: add XFS_IOC_SETFSUUID ioctl
> > >>  xfs: export meta uuid via xfs_fsop_geom
> > > 
> > > For what purpose does userspace ever need to know the sb_meta_uuid?
> > 
> > Userspace would need to know the meta uuid if we want to restore
> > the original uuid after it has been changed.
> 
> I don't understand why you'd want to restore the original UUID given
> the use case you've describe. Can you explain the situation where
> you want to return a cloned image to the original golden image UUID?

I can't think of any, and was assuming that Catherine did that to
maintain parity with the offline fsuuid setting command...

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Darrick J. Wong]

On Tue, Mar 14, 2023 at 05:28:47PM +1100, Dave Chinner wrote:
> On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
> > Hi all,
> > 
> > This series of patches implements a new ioctl to set the uuid of mounted
> > filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
> > to allow userspace to update the uuid.
> > 
> > Comments and feedback appreciated!
> 
> What's the use case for this?
> 
> What are the pro's and cons of the implementation?
> 
> Some problems I see:
> 
> * How does this interact with pNFS exports (i.e.
> CONFIG_EXPORTFS_BLOCK_OPS). XFS hands the sb_uuid directly to pNFS
> server (and remote clients, I think) so that incoming mapping
> requests can be directed to the correct block device hosting the XFS
> filesystem the mapping information is for. IIRC, the pNFS data path
> is just given a byte offset into the device where the UUID in the
> superblock lives, and if it matches it allows the remote IO to go
> ahead - it doesn't actually know that there is a filesystem on that
> device at all...

I think we're going to have to detect the presence of pNFS exports and
EAGAIN if there are any active.  That probably involves incrementing a
counter during ->map_blocks and decreasing it during ->commit blocks.

That might still invite races between someone setting the fsuuid and
exporting via NFS.  

> * IIRC, the nfsd export table can also use the filesystem uuid to
> identify the filesystem being exported, and IIRC that gets encoded
> in the filehandle. Does changing the filesystem UUID then cause
> problems with export indentification and/or file handle
> creation/resolution?

I thought NFS (when not being used for block layouts and pnfs stuff)
still used the fsid that's also in the superblock?  Presumably we'd
leave the old m_fixedfsid unchanged while the fs is still mounted, and
document the caveat that handles will not work after a remount.

On some level, we might simply have to document that changing the
user-visible uuid will break file handles and pnfs exports, so sysadmins
had better get that done early.

OTOH that is an argument for leaving the xfs_db-based version that we
have now.

> * Is the VFS prepared for sb->s_uuid changing underneath running
> operations on mounted filesystems? I can see that this might cause
> problems with any sort of fscrypt implementation as it may encode
> the s_uuid into encryption keys held in xattrs, similarly IMA and
> EVM integrity protection keys.

I would hope it's not so hard to detect that EVM/fscrypt are active on a
given xfs mount.  EVM seems pretty hard to detect since it operates
above the fs.

fscrypt might not be so difficult, since we likely need to add support
in the ondisk metadata, which means xfs will know.

IMA seems to be using it for rule matching, so I'd say that if the
sysadmin changes the fsuuid, they had better update the IMA rulebook
too.

Come to think of it, perhaps reading a filesystem's uuid (whether via
handle export, direct read of s_uuid, nfs, evm, ima, or fscrypt) should
set a superblock flag that someone has accessed it; and only if nobody's
yet accessed it do we allow the fsuuid update?

> * Should the VFS superblock sb->s_uuid use the XFS
> sb->sb_meta_uuid value and never change because we can encode it
> into other objects stored in the active filesystem? What does that
> mean for tools that identify block devices or filesystems by the VFS
> uuid rather than the filesystem uuid?

I don't know of any other than the ones you found.

> There's a whole can-o-worms here - the ability to dynamically change
> the UUID of a filesystem while it is mounted mean we need to think
> about these things - it's no longer as simple as "can only do it
> offline" which is typically only used to relabel a writeable
> snapshot of a golden image file during new VM deployment
> preparation.....

Agreed.

> > 
> > Catherine
> > 
> > Catherine Hoang (4):
> >   xfs: refactor xfs_uuid_mount and xfs_uuid_unmount
> >   xfs: implement custom freeze/thaw functions
> 
> The "custom" parts that XFS requires need to be added to the VFS
> level freeze/thaw functions, not duplicate the VFS functionality
> within XFS and then slight modify it for this additional feature.
> Doing this results in unmaintainable code over the long term.

Yeah, I think for the next iteration we ought to try to pull in Luis's
kernel-initiated-exclusive-freeze patch.

> >   xfs: add XFS_IOC_SETFSUUID ioctl
> >   xfs: export meta uuid via xfs_fsop_geom
> 
> For what purpose does userspace ever need to know the sb_meta_uuid?

(No idea; once someone sets a fsuuid they usually don't want to go
back.)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v1 0/4] setting uuid of online filesystems

[Amir Goldstein]

On Sat, Mar 18, 2023 at 2:24 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Mar 14, 2023 at 05:28:47PM +1100, Dave Chinner wrote:
> > On Mon, Mar 13, 2023 at 09:21:05PM -0700, Catherine Hoang wrote:
> > > Hi all,
> > >
> > > This series of patches implements a new ioctl to set the uuid of mounted
> > > filesystems. Eventually this will be used by the 'xfs_io fsuuid' command
> > > to allow userspace to update the uuid.
> > >
> > > Comments and feedback appreciated!
> >
> > What's the use case for this?
> >
> > What are the pro's and cons of the implementation?
> >
> > Some problems I see:
> >
> > * How does this interact with pNFS exports (i.e.
> > CONFIG_EXPORTFS_BLOCK_OPS). XFS hands the sb_uuid directly to pNFS
> > server (and remote clients, I think) so that incoming mapping
> > requests can be directed to the correct block device hosting the XFS
> > filesystem the mapping information is for. IIRC, the pNFS data path
> > is just given a byte offset into the device where the UUID in the
> > superblock lives, and if it matches it allows the remote IO to go
> > ahead - it doesn't actually know that there is a filesystem on that
> > device at all...
>
> I think we're going to have to detect the presence of pNFS exports and
> EAGAIN if there are any active.  That probably involves incrementing a
> counter during ->map_blocks and decreasing it during ->commit blocks.
>
> That might still invite races between someone setting the fsuuid and
> exporting via NFS.
>
> > * IIRC, the nfsd export table can also use the filesystem uuid to
> > identify the filesystem being exported, and IIRC that gets encoded
> > in the filehandle. Does changing the filesystem UUID then cause
> > problems with export indentification and/or file handle
> > creation/resolution?
>
> I thought NFS (when not being used for block layouts and pnfs stuff)
> still used the fsid that's also in the superblock?  Presumably we'd
> leave the old m_fixedfsid unchanged while the fs is still mounted, and
> document the caveat that handles will not work after a remount.
>
> On some level, we might simply have to document that changing the
> user-visible uuid will break file handles and pnfs exports, so sysadmins
> had better get that done early.
>
> OTOH that is an argument for leaving the xfs_db-based version that we
> have now.
>
> > * Is the VFS prepared for sb->s_uuid changing underneath running
> > operations on mounted filesystems? I can see that this might cause
> > problems with any sort of fscrypt implementation as it may encode
> > the s_uuid into encryption keys held in xattrs, similarly IMA and
> > EVM integrity protection keys.
>
> I would hope it's not so hard to detect that EVM/fscrypt are active on a
> given xfs mount.  EVM seems pretty hard to detect since it operates
> above the fs.
>
> fscrypt might not be so difficult, since we likely need to add support
> in the ondisk metadata, which means xfs will know.
>
> IMA seems to be using it for rule matching, so I'd say that if the
> sysadmin changes the fsuuid, they had better update the IMA rulebook
> too.
>
> Come to think of it, perhaps reading a filesystem's uuid (whether via
> handle export, direct read of s_uuid, nfs, evm, ima, or fscrypt) should
> set a superblock flag that someone has accessed it; and only if nobody's
> yet accessed it do we allow the fsuuid update?
>
> > * Should the VFS superblock sb->s_uuid use the XFS
> > sb->sb_meta_uuid value and never change because we can encode it
> > into other objects stored in the active filesystem? What does that
> > mean for tools that identify block devices or filesystems by the VFS
> > uuid rather than the filesystem uuid?
>
> I don't know of any other than the ones you found.

FWIW, overlayfs also uses s_uuid in a similar manner to nfsd
as a persistent reference to the origin lower file after copy up.

Like nfsd, the overlayfs persistent origin handle (fsuuid+filehandle) will break
when changing lower fs uuid offline, but I don't see much difference from
changing s_uuid online - worse case that I can think of is that an overlayfs
inode will change its st_ino after evict/lookup on a mounted overlayfs.

Thanks,
Amir.