[PATCH v2] statx: stx_subvol

[PATCH v2] statx: stx_subvol

[Kent Overstreet]

Add a new statx field for (sub)volume identifiers, as implemented by
btrfs and bcachefs.

This includes bcachefs support; we'll definitely want btrfs support as
well.

Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: Miklos Szeredi <mszeredi@redhat.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
---
 fs/bcachefs/fs.c          | 3 +++
 fs/stat.c                 | 1 +
 include/linux/stat.h      | 1 +
 include/uapi/linux/stat.h | 4 +++-
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 3f073845bbd7..6a542ed43e2c 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
 	stat->blksize	= block_bytes(c);
 	stat->blocks	= inode->v.i_blocks;
 
+	stat->subvol	= inode->ei_subvol;
+	stat->result_mask |= STATX_SUBVOL;
+
 	if (request_mask & STATX_BTIME) {
 		stat->result_mask |= STATX_BTIME;
 		stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
diff --git a/fs/stat.c b/fs/stat.c
index 77cdc69eb422..70bd3e888cfa 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
 	tmp.stx_mnt_id = stat->mnt_id;
 	tmp.stx_dio_mem_align = stat->dio_mem_align;
 	tmp.stx_dio_offset_align = stat->dio_offset_align;
+	tmp.stx_subvol = stat->subvol;
 
 	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
 }
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 52150570d37a..bf92441dbad2 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -53,6 +53,7 @@ struct kstat {
 	u32		dio_mem_align;
 	u32		dio_offset_align;
 	u64		change_cookie;
+	u64		subvol;
 };
 
 /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 2f2ee82d5517..67626d535316 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -126,8 +126,9 @@ struct statx {
 	__u64	stx_mnt_id;
 	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
 	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
+	__u64	stx_subvol;	/* Subvolume identifier */
 	/* 0xa0 */
-	__u64	__spare3[12];	/* Spare space for future expansion */
+	__u64	__spare3[11];	/* Spare space for future expansion */
 	/* 0x100 */
 };
 
@@ -155,6 +156,7 @@ struct statx {
 #define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
 #define STATX_DIOALIGN		0x00002000U	/* Want/got direct I/O alignment info */
 #define STATX_MNT_ID_UNIQUE	0x00004000U	/* Want/got extended stx_mount_id */
+#define STATX_SUBVOL		0x00008000U	/* Want/got stx_subvol */
 
 #define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 
-- 
2.43.0

Re: [PATCH v2] statx: stx_subvol

[Neal Gompa]

On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> Add a new statx field for (sub)volume identifiers, as implemented by
> btrfs and bcachefs.
>
> This includes bcachefs support; we'll definitely want btrfs support as
> well.
>
> Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: Miklos Szeredi <mszeredi@redhat.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> ---
>  fs/bcachefs/fs.c          | 3 +++
>  fs/stat.c                 | 1 +
>  include/linux/stat.h      | 1 +
>  include/uapi/linux/stat.h | 4 +++-
>  4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index 3f073845bbd7..6a542ed43e2c 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
>         stat->blksize   = block_bytes(c);
>         stat->blocks    = inode->v.i_blocks;
>
> +       stat->subvol    = inode->ei_subvol;
> +       stat->result_mask |= STATX_SUBVOL;
> +
>         if (request_mask & STATX_BTIME) {
>                 stat->result_mask |= STATX_BTIME;
>                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> diff --git a/fs/stat.c b/fs/stat.c
> index 77cdc69eb422..70bd3e888cfa 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
>         tmp.stx_mnt_id = stat->mnt_id;
>         tmp.stx_dio_mem_align = stat->dio_mem_align;
>         tmp.stx_dio_offset_align = stat->dio_offset_align;
> +       tmp.stx_subvol = stat->subvol;
>
>         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
>  }
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index 52150570d37a..bf92441dbad2 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -53,6 +53,7 @@ struct kstat {
>         u32             dio_mem_align;
>         u32             dio_offset_align;
>         u64             change_cookie;
> +       u64             subvol;
>  };
>
>  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 2f2ee82d5517..67626d535316 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -126,8 +126,9 @@ struct statx {
>         __u64   stx_mnt_id;
>         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
>         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> +       __u64   stx_subvol;     /* Subvolume identifier */
>         /* 0xa0 */
> -       __u64   __spare3[12];   /* Spare space for future expansion */
> +       __u64   __spare3[11];   /* Spare space for future expansion */
>         /* 0x100 */
>  };
>
> @@ -155,6 +156,7 @@ struct statx {
>  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
>  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
>  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
>
>  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
>
> --
> 2.43.0
>
>

I think it's generally expected that patches that touch different
layers are split up. That is, we should have a patch that adds the
capability and a separate patch that enables it in bcachefs. This also
helps make it clearer to others how a new feature should be plumbed
into a filesystem.

I would prefer it to be split up in this manner for this reason.



--
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Fri, Mar 08, 2024 at 06:42:27AM -0500, Neal Gompa wrote:
> On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > Add a new statx field for (sub)volume identifiers, as implemented by
> > btrfs and bcachefs.
> >
> > This includes bcachefs support; we'll definitely want btrfs support as
> > well.
> >
> > Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Josef Bacik <josef@toxicpanda.com>
> > Cc: Miklos Szeredi <mszeredi@redhat.com>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: David Howells <dhowells@redhat.com>
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > ---
> >  fs/bcachefs/fs.c          | 3 +++
> >  fs/stat.c                 | 1 +
> >  include/linux/stat.h      | 1 +
> >  include/uapi/linux/stat.h | 4 +++-
> >  4 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > index 3f073845bbd7..6a542ed43e2c 100644
> > --- a/fs/bcachefs/fs.c
> > +++ b/fs/bcachefs/fs.c
> > @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
> >         stat->blksize   = block_bytes(c);
> >         stat->blocks    = inode->v.i_blocks;
> >
> > +       stat->subvol    = inode->ei_subvol;
> > +       stat->result_mask |= STATX_SUBVOL;
> > +
> >         if (request_mask & STATX_BTIME) {
> >                 stat->result_mask |= STATX_BTIME;
> >                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 77cdc69eb422..70bd3e888cfa 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> >         tmp.stx_mnt_id = stat->mnt_id;
> >         tmp.stx_dio_mem_align = stat->dio_mem_align;
> >         tmp.stx_dio_offset_align = stat->dio_offset_align;
> > +       tmp.stx_subvol = stat->subvol;
> >
> >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> >  }
> > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > index 52150570d37a..bf92441dbad2 100644
> > --- a/include/linux/stat.h
> > +++ b/include/linux/stat.h
> > @@ -53,6 +53,7 @@ struct kstat {
> >         u32             dio_mem_align;
> >         u32             dio_offset_align;
> >         u64             change_cookie;
> > +       u64             subvol;
> >  };
> >
> >  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > index 2f2ee82d5517..67626d535316 100644
> > --- a/include/uapi/linux/stat.h
> > +++ b/include/uapi/linux/stat.h
> > @@ -126,8 +126,9 @@ struct statx {
> >         __u64   stx_mnt_id;
> >         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
> >         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> > +       __u64   stx_subvol;     /* Subvolume identifier */
> >         /* 0xa0 */
> > -       __u64   __spare3[12];   /* Spare space for future expansion */
> > +       __u64   __spare3[11];   /* Spare space for future expansion */
> >         /* 0x100 */
> >  };
> >
> > @@ -155,6 +156,7 @@ struct statx {
> >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> >  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
> >  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> > +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
> >
> >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> >
> > --
> > 2.43.0
> >
> >
> 
> I think it's generally expected that patches that touch different
> layers are split up. That is, we should have a patch that adds the
> capability and a separate patch that enables it in bcachefs. This also
> helps make it clearer to others how a new feature should be plumbed
> into a filesystem.
> 
> I would prefer it to be split up in this manner for this reason.

I'll do it that way if the patch is big enough that it ought to be
split up. For something this small, seeing how it's used is relevant
context for both reviewers and people looking at it afterwards.

Re: [PATCH v2] statx: stx_subvol

[Neal Gompa]

On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> On Fri, Mar 08, 2024 at 06:42:27AM -0500, Neal Gompa wrote:
> > On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > >
> > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > btrfs and bcachefs.
> > >
> > > This includes bcachefs support; we'll definitely want btrfs support as
> > > well.
> > >
> > > Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > Cc: Josef Bacik <josef@toxicpanda.com>
> > > Cc: Miklos Szeredi <mszeredi@redhat.com>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > ---
> > >  fs/bcachefs/fs.c          | 3 +++
> > >  fs/stat.c                 | 1 +
> > >  include/linux/stat.h      | 1 +
> > >  include/uapi/linux/stat.h | 4 +++-
> > >  4 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > > index 3f073845bbd7..6a542ed43e2c 100644
> > > --- a/fs/bcachefs/fs.c
> > > +++ b/fs/bcachefs/fs.c
> > > @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
> > >         stat->blksize   = block_bytes(c);
> > >         stat->blocks    = inode->v.i_blocks;
> > >
> > > +       stat->subvol    = inode->ei_subvol;
> > > +       stat->result_mask |= STATX_SUBVOL;
> > > +
> > >         if (request_mask & STATX_BTIME) {
> > >                 stat->result_mask |= STATX_BTIME;
> > >                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index 77cdc69eb422..70bd3e888cfa 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> > >         tmp.stx_mnt_id = stat->mnt_id;
> > >         tmp.stx_dio_mem_align = stat->dio_mem_align;
> > >         tmp.stx_dio_offset_align = stat->dio_offset_align;
> > > +       tmp.stx_subvol = stat->subvol;
> > >
> > >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> > >  }
> > > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > > index 52150570d37a..bf92441dbad2 100644
> > > --- a/include/linux/stat.h
> > > +++ b/include/linux/stat.h
> > > @@ -53,6 +53,7 @@ struct kstat {
> > >         u32             dio_mem_align;
> > >         u32             dio_offset_align;
> > >         u64             change_cookie;
> > > +       u64             subvol;
> > >  };
> > >
> > >  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> > > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > > index 2f2ee82d5517..67626d535316 100644
> > > --- a/include/uapi/linux/stat.h
> > > +++ b/include/uapi/linux/stat.h
> > > @@ -126,8 +126,9 @@ struct statx {
> > >         __u64   stx_mnt_id;
> > >         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
> > >         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> > > +       __u64   stx_subvol;     /* Subvolume identifier */
> > >         /* 0xa0 */
> > > -       __u64   __spare3[12];   /* Spare space for future expansion */
> > > +       __u64   __spare3[11];   /* Spare space for future expansion */
> > >         /* 0x100 */
> > >  };
> > >
> > > @@ -155,6 +156,7 @@ struct statx {
> > >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> > >  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
> > >  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> > > +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
> > >
> > >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> > >
> > > --
> > > 2.43.0
> > >
> > >
> >
> > I think it's generally expected that patches that touch different
> > layers are split up. That is, we should have a patch that adds the
> > capability and a separate patch that enables it in bcachefs. This also
> > helps make it clearer to others how a new feature should be plumbed
> > into a filesystem.
> >
> > I would prefer it to be split up in this manner for this reason.
>
> I'll do it that way if the patch is big enough that it ought to be
> split up. For something this small, seeing how it's used is relevant
> context for both reviewers and people looking at it afterwards.
>

It needs to also be split up because fs/ and fs/bcachefs are
maintained differently. And while right now bcachefs is the only
consumer of the API, btrfs will add it right after it's committed, and
for people who are cherry-picking/backporting accordingly, having to
chop out part of a patch would be unpleasant.


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Fri, Mar 08, 2024 at 11:44:48AM -0500, Neal Gompa wrote:
> On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > On Fri, Mar 08, 2024 at 06:42:27AM -0500, Neal Gompa wrote:
> > > On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
> > > <kent.overstreet@linux.dev> wrote:
> > > >
> > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > btrfs and bcachefs.
> > > >
> > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > well.
> > > >
> > > > Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > Cc: Josef Bacik <josef@toxicpanda.com>
> > > > Cc: Miklos Szeredi <mszeredi@redhat.com>
> > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > ---
> > > >  fs/bcachefs/fs.c          | 3 +++
> > > >  fs/stat.c                 | 1 +
> > > >  include/linux/stat.h      | 1 +
> > > >  include/uapi/linux/stat.h | 4 +++-
> > > >  4 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > > > index 3f073845bbd7..6a542ed43e2c 100644
> > > > --- a/fs/bcachefs/fs.c
> > > > +++ b/fs/bcachefs/fs.c
> > > > @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
> > > >         stat->blksize   = block_bytes(c);
> > > >         stat->blocks    = inode->v.i_blocks;
> > > >
> > > > +       stat->subvol    = inode->ei_subvol;
> > > > +       stat->result_mask |= STATX_SUBVOL;
> > > > +
> > > >         if (request_mask & STATX_BTIME) {
> > > >                 stat->result_mask |= STATX_BTIME;
> > > >                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > index 77cdc69eb422..70bd3e888cfa 100644
> > > > --- a/fs/stat.c
> > > > +++ b/fs/stat.c
> > > > @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> > > >         tmp.stx_mnt_id = stat->mnt_id;
> > > >         tmp.stx_dio_mem_align = stat->dio_mem_align;
> > > >         tmp.stx_dio_offset_align = stat->dio_offset_align;
> > > > +       tmp.stx_subvol = stat->subvol;
> > > >
> > > >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> > > >  }
> > > > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > > > index 52150570d37a..bf92441dbad2 100644
> > > > --- a/include/linux/stat.h
> > > > +++ b/include/linux/stat.h
> > > > @@ -53,6 +53,7 @@ struct kstat {
> > > >         u32             dio_mem_align;
> > > >         u32             dio_offset_align;
> > > >         u64             change_cookie;
> > > > +       u64             subvol;
> > > >  };
> > > >
> > > >  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> > > > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > > > index 2f2ee82d5517..67626d535316 100644
> > > > --- a/include/uapi/linux/stat.h
> > > > +++ b/include/uapi/linux/stat.h
> > > > @@ -126,8 +126,9 @@ struct statx {
> > > >         __u64   stx_mnt_id;
> > > >         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
> > > >         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> > > > +       __u64   stx_subvol;     /* Subvolume identifier */
> > > >         /* 0xa0 */
> > > > -       __u64   __spare3[12];   /* Spare space for future expansion */
> > > > +       __u64   __spare3[11];   /* Spare space for future expansion */
> > > >         /* 0x100 */
> > > >  };
> > > >
> > > > @@ -155,6 +156,7 @@ struct statx {
> > > >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> > > >  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
> > > >  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> > > > +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
> > > >
> > > >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> > > >
> > > > --
> > > > 2.43.0
> > > >
> > > >
> > >
> > > I think it's generally expected that patches that touch different
> > > layers are split up. That is, we should have a patch that adds the
> > > capability and a separate patch that enables it in bcachefs. This also
> > > helps make it clearer to others how a new feature should be plumbed
> > > into a filesystem.
> > >
> > > I would prefer it to be split up in this manner for this reason.
> >
> > I'll do it that way if the patch is big enough that it ought to be
> > split up. For something this small, seeing how it's used is relevant
> > context for both reviewers and people looking at it afterwards.
> >
> 
> It needs to also be split up because fs/ and fs/bcachefs are
> maintained differently. And while right now bcachefs is the only
> consumer of the API, btrfs will add it right after it's committed, and
> for people who are cherry-picking/backporting accordingly, having to
> chop out part of a patch would be unpleasant.

It's a new feature, not a bugfix, this should never get backported. And
I the bcachefs maintainer wrote the patch, and I'm submitting it to the
VFS maintainer, so if it's fine with him it's fine with me.

Re: [PATCH v2] statx: stx_subvol

[Darrick J. Wong]

On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> On Fri, Mar 08, 2024 at 11:44:48AM -0500, Neal Gompa wrote:
> > On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > >
> > > On Fri, Mar 08, 2024 at 06:42:27AM -0500, Neal Gompa wrote:
> > > > On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
> > > > <kent.overstreet@linux.dev> wrote:
> > > > >
> > > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > > btrfs and bcachefs.
> > > > >
> > > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > > well.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > > Cc: Josef Bacik <josef@toxicpanda.com>
> > > > > Cc: Miklos Szeredi <mszeredi@redhat.com>
> > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > > ---
> > > > >  fs/bcachefs/fs.c          | 3 +++
> > > > >  fs/stat.c                 | 1 +
> > > > >  include/linux/stat.h      | 1 +
> > > > >  include/uapi/linux/stat.h | 4 +++-
> > > > >  4 files changed, 8 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > > > > index 3f073845bbd7..6a542ed43e2c 100644
> > > > > --- a/fs/bcachefs/fs.c
> > > > > +++ b/fs/bcachefs/fs.c
> > > > > @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
> > > > >         stat->blksize   = block_bytes(c);
> > > > >         stat->blocks    = inode->v.i_blocks;
> > > > >
> > > > > +       stat->subvol    = inode->ei_subvol;
> > > > > +       stat->result_mask |= STATX_SUBVOL;
> > > > > +
> > > > >         if (request_mask & STATX_BTIME) {
> > > > >                 stat->result_mask |= STATX_BTIME;
> > > > >                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> > > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > > index 77cdc69eb422..70bd3e888cfa 100644
> > > > > --- a/fs/stat.c
> > > > > +++ b/fs/stat.c
> > > > > @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> > > > >         tmp.stx_mnt_id = stat->mnt_id;
> > > > >         tmp.stx_dio_mem_align = stat->dio_mem_align;
> > > > >         tmp.stx_dio_offset_align = stat->dio_offset_align;
> > > > > +       tmp.stx_subvol = stat->subvol;
> > > > >
> > > > >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> > > > >  }
> > > > > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > > > > index 52150570d37a..bf92441dbad2 100644
> > > > > --- a/include/linux/stat.h
> > > > > +++ b/include/linux/stat.h
> > > > > @@ -53,6 +53,7 @@ struct kstat {
> > > > >         u32             dio_mem_align;
> > > > >         u32             dio_offset_align;
> > > > >         u64             change_cookie;
> > > > > +       u64             subvol;
> > > > >  };
> > > > >
> > > > >  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> > > > > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > > > > index 2f2ee82d5517..67626d535316 100644
> > > > > --- a/include/uapi/linux/stat.h
> > > > > +++ b/include/uapi/linux/stat.h
> > > > > @@ -126,8 +126,9 @@ struct statx {
> > > > >         __u64   stx_mnt_id;
> > > > >         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
> > > > >         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> > > > > +       __u64   stx_subvol;     /* Subvolume identifier */
> > > > >         /* 0xa0 */
> > > > > -       __u64   __spare3[12];   /* Spare space for future expansion */
> > > > > +       __u64   __spare3[11];   /* Spare space for future expansion */
> > > > >         /* 0x100 */
> > > > >  };
> > > > >
> > > > > @@ -155,6 +156,7 @@ struct statx {
> > > > >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> > > > >  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
> > > > >  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> > > > > +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
> > > > >
> > > > >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> > > > >
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > >
> > > >
> > > > I think it's generally expected that patches that touch different
> > > > layers are split up. That is, we should have a patch that adds the
> > > > capability and a separate patch that enables it in bcachefs. This also
> > > > helps make it clearer to others how a new feature should be plumbed
> > > > into a filesystem.
> > > >
> > > > I would prefer it to be split up in this manner for this reason.
> > >
> > > I'll do it that way if the patch is big enough that it ought to be
> > > split up. For something this small, seeing how it's used is relevant
> > > context for both reviewers and people looking at it afterwards.
> > >
> > 
> > It needs to also be split up because fs/ and fs/bcachefs are
> > maintained differently. And while right now bcachefs is the only
> > consumer of the API, btrfs will add it right after it's committed, and
> > for people who are cherry-picking/backporting accordingly, having to
> > chop out part of a patch would be unpleasant.
> 
> It's a new feature, not a bugfix, this should never get backported. And
> I the bcachefs maintainer wrote the patch, and I'm submitting it to the
> VFS maintainer, so if it's fine with him it's fine with me.

But then how am I supposed to bikeshed the structure of the V2 patchset
by immediately asking you to recombine the patches and spit out a V3?

</sarcasm>

But, seriously, can you update the manpage too?  Is stx_subvol a u64
cookie where userspace mustn't try to read anything into its contents?
Just like st_ino and st_dev are (supposed) to be?

Should the XFS data and rt volumes be reported with different stx_vol
values?

--D

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> > It's a new feature, not a bugfix, this should never get backported. And
> > I the bcachefs maintainer wrote the patch, and I'm submitting it to the
> > VFS maintainer, so if it's fine with him it's fine with me.
> 
> But then how am I supposed to bikeshed the structure of the V2 patchset
> by immediately asking you to recombine the patches and spit out a V3?
> 
> </sarcasm>
> 
> But, seriously, can you update the manpage too?

yeah, where's that at?

> Is stx_subvol a u64
> cookie where userspace mustn't try to read anything into its contents?
> Just like st_ino and st_dev are (supposed) to be?

Actually, that's up for debate. I'm considering having the readdir()
equivalent for walking subvolumes return subvolume IDs, and then there'd
be a separate call to open by ID.

Al's idea was to return open fds to child subvolumes, then userspace can
get the path from /proc; that's also a possibility.

The key thing is that with subvolumes it's actually possible to do an
open_by_id() call with correct security checks on pathwalking - because
we don't have hardlinks so there's no ambiguity.

Or we might do it getdents() style and return the path directly.

But I think userspace is going to want to work with the volume
identifiers directly, which is partly why I'm considering why other
options might be cleaner.

Another thing to consider: where we're going with this is giving
userspace a good efficient interrface for recursive tree traversal of
subvolumes, but it might not be a bad idea to do that for mountpoints as
well - similar problems, similar scalability issues that we might want
to solve eventually.

> Should the XFS data and rt volumes be reported with different stx_vol
> values?

Maybe?

Re: [PATCH v2] statx: stx_subvol

[Jeff Layton]

On Fri, 2024-03-08 at 12:13 -0500, Kent Overstreet wrote:
> On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> > On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> > > It's a new feature, not a bugfix, this should never get backported. And
> > > I the bcachefs maintainer wrote the patch, and I'm submitting it to the
> > > VFS maintainer, so if it's fine with him it's fine with me.
> > 
> > But then how am I supposed to bikeshed the structure of the V2 patchset
> > by immediately asking you to recombine the patches and spit out a V3?
> > 
> > </sarcasm>
> > 
> > But, seriously, can you update the manpage too?
> 
> yeah, where's that at?
> 

    https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git


> > Is stx_subvol a u64
> > cookie where userspace mustn't try to read anything into its contents?
> > Just like st_ino and st_dev are (supposed) to be?
> 
> Actually, that's up for debate. I'm considering having the readdir()
> equivalent for walking subvolumes return subvolume IDs, and then there'd
> be a separate call to open by ID.
> 
> Al's idea was to return open fds to child subvolumes, then userspace can
> get the path from /proc; that's also a possibility.
> 
> The key thing is that with subvolumes it's actually possible to do an
> open_by_id() call with correct security checks on pathwalking - because
> we don't have hardlinks so there's no ambiguity.
> 
> Or we might do it getdents() style and return the path directly.
> 
> But I think userspace is going to want to work with the volume
> identifiers directly, which is partly why I'm considering why other
> options might be cleaner.
> 
> Another thing to consider: where we're going with this is giving
> userspace a good efficient interrface for recursive tree traversal of
> subvolumes, but it might not be a bad idea to do that for mountpoints as
> well - similar problems, similar scalability issues that we might want
> to solve eventually.
> 

All of that's fine, but Darrick's question is about whether we should
ensure that these IDs are considered _opaque_. I think they should be.

We don't want to anyone to fall into the trap of trying to convey extra
info to userland about the volumes via this value. It should only be
good for uniquely identifying the volume.

We'll also need to document the scope of uniqueness. I assume we'll want
to define this as only being unique within a single filesystem? IOW, if
I have 2 bcachefs filesystems that are on independent devices, these
values may collide? Someone wanting to uniquely identify a subvolume on
a system will need to check both the st_dev and the st_vol, correct?


> > Should the XFS data and rt volumes be reported with different stx_vol
> > values?
> 
> Maybe?
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Sat, Mar 09, 2024 at 06:46:54AM -0500, Jeff Layton wrote:
> On Fri, 2024-03-08 at 12:13 -0500, Kent Overstreet wrote:
> > On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> > > On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> > > > It's a new feature, not a bugfix, this should never get backported. And
> > > > I the bcachefs maintainer wrote the patch, and I'm submitting it to the
> > > > VFS maintainer, so if it's fine with him it's fine with me.
> > > 
> > > But then how am I supposed to bikeshed the structure of the V2 patchset
> > > by immediately asking you to recombine the patches and spit out a V3?
> > > 
> > > </sarcasm>
> > > 
> > > But, seriously, can you update the manpage too?
> > 
> > yeah, where's that at?
> > 
> 
>     https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
> 
> 
> > > Is stx_subvol a u64
> > > cookie where userspace mustn't try to read anything into its contents?
> > > Just like st_ino and st_dev are (supposed) to be?
> > 
> > Actually, that's up for debate. I'm considering having the readdir()
> > equivalent for walking subvolumes return subvolume IDs, and then there'd
> > be a separate call to open by ID.
> > 
> > Al's idea was to return open fds to child subvolumes, then userspace can
> > get the path from /proc; that's also a possibility.
> > 
> > The key thing is that with subvolumes it's actually possible to do an
> > open_by_id() call with correct security checks on pathwalking - because
> > we don't have hardlinks so there's no ambiguity.
> > 
> > Or we might do it getdents() style and return the path directly.
> > 
> > But I think userspace is going to want to work with the volume
> > identifiers directly, which is partly why I'm considering why other
> > options might be cleaner.
> > 
> > Another thing to consider: where we're going with this is giving
> > userspace a good efficient interrface for recursive tree traversal of
> > subvolumes, but it might not be a bad idea to do that for mountpoints as
> > well - similar problems, similar scalability issues that we might want
> > to solve eventually.
> > 
> 
> All of that's fine, but Darrick's question is about whether we should
> ensure that these IDs are considered _opaque_. I think they should be.
> 
> We don't want to anyone to fall into the trap of trying to convey extra
> info to userland about the volumes via this value. It should only be
> good for uniquely identifying the volume.
> 
> We'll also need to document the scope of uniqueness. I assume we'll want
> to define this as only being unique within a single filesystem? IOW, if
> I have 2 bcachefs filesystems that are on independent devices, these
> values may collide? Someone wanting to uniquely identify a subvolume on
> a system will need to check both the st_dev and the st_vol, correct?

they're small integers, not UUIDs, so yes

Re: [PATCH v2] statx: stx_subvol

[Dave Chinner]

On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> Should the XFS data and rt volumes be reported with different stx_vol
> values?

No, because all the inodes are on the data volume and the same inode
can have data on the data volume or the rt volume. i.e. "data on rt,
truncate, clear rt, copy data back into data dev".  It's still the
same inode, and may have exactly the same data, so why should change
stx_vol and make it appear to userspace as being a different inode?

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

Re: [PATCH v2] statx: stx_subvol

[Miklos Szeredi]

On Mon, 11 Mar 2024 at 03:17, Dave Chinner <david@fromorbit.com> wrote:
>
> On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> > Should the XFS data and rt volumes be reported with different stx_vol
> > values?
>
> No, because all the inodes are on the data volume and the same inode
> can have data on the data volume or the rt volume. i.e. "data on rt,
> truncate, clear rt, copy data back into data dev".  It's still the
> same inode, and may have exactly the same data, so why should change
> stx_vol and make it appear to userspace as being a different inode?

Because stx_vol must not be used by userspace to distinguish between
unique inodes.  To determine if two inodes are distinct within a
filesystem (which may have many volumes) it should query the file
handle and compare that.

If we'll have a filesystem that has a different stx_vol but the same
fh, all the better.

Thanks,
Miklos

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Mon, Mar 11, 2024 at 06:30:21AM +0100, Miklos Szeredi wrote:
> On Mon, 11 Mar 2024 at 03:17, Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> > > Should the XFS data and rt volumes be reported with different stx_vol
> > > values?
> >
> > No, because all the inodes are on the data volume and the same inode
> > can have data on the data volume or the rt volume. i.e. "data on rt,
> > truncate, clear rt, copy data back into data dev".  It's still the
> > same inode, and may have exactly the same data, so why should change
> > stx_vol and make it appear to userspace as being a different inode?
> 
> Because stx_vol must not be used by userspace to distinguish between
> unique inodes.  To determine if two inodes are distinct within a
> filesystem (which may have many volumes) it should query the file
> handle and compare that.
> 
> If we'll have a filesystem that has a different stx_vol but the same
> fh, all the better.

I agree that stx_vol should not be used for uniqueness testing, but
that's a non sequitar here; Dave's talking about the fact that volume
isn't a constatn for a given inode on XFS. And that's a good point;
volumes on XFS don't map to the filesystem path heirarchy in a nice
clean way like on btrfs and bcachefs (and presumably ZFS).

Subvolumes on btrfs and bcachefs form a tree, and that's something we
should document about stx_subvol - recursively enumerable things are
quite nice to work with.

Re: [PATCH v2] statx: stx_subvol

[Christian Brauner]

On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> > On Fri, Mar 08, 2024 at 11:44:48AM -0500, Neal Gompa wrote:
> > > On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
> > > <kent.overstreet@linux.dev> wrote:
> > > >
> > > > On Fri, Mar 08, 2024 at 06:42:27AM -0500, Neal Gompa wrote:
> > > > > On Thu, Mar 7, 2024 at 9:29 PM Kent Overstreet
> > > > > <kent.overstreet@linux.dev> wrote:
> > > > > >
> > > > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > > > btrfs and bcachefs.
> > > > > >
> > > > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > > > well.
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-fsdevel/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > > > Cc: Josef Bacik <josef@toxicpanda.com>
> > > > > > Cc: Miklos Szeredi <mszeredi@redhat.com>
> > > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > > > > > ---
> > > > > >  fs/bcachefs/fs.c          | 3 +++
> > > > > >  fs/stat.c                 | 1 +
> > > > > >  include/linux/stat.h      | 1 +
> > > > > >  include/uapi/linux/stat.h | 4 +++-
> > > > > >  4 files changed, 8 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > > > > > index 3f073845bbd7..6a542ed43e2c 100644
> > > > > > --- a/fs/bcachefs/fs.c
> > > > > > +++ b/fs/bcachefs/fs.c
> > > > > > @@ -840,6 +840,9 @@ static int bch2_getattr(struct mnt_idmap *idmap,
> > > > > >         stat->blksize   = block_bytes(c);
> > > > > >         stat->blocks    = inode->v.i_blocks;
> > > > > >
> > > > > > +       stat->subvol    = inode->ei_subvol;
> > > > > > +       stat->result_mask |= STATX_SUBVOL;
> > > > > > +
> > > > > >         if (request_mask & STATX_BTIME) {
> > > > > >                 stat->result_mask |= STATX_BTIME;
> > > > > >                 stat->btime = bch2_time_to_timespec(c, inode->ei_inode.bi_otime);
> > > > > > diff --git a/fs/stat.c b/fs/stat.c
> > > > > > index 77cdc69eb422..70bd3e888cfa 100644
> > > > > > --- a/fs/stat.c
> > > > > > +++ b/fs/stat.c
> > > > > > @@ -658,6 +658,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> > > > > >         tmp.stx_mnt_id = stat->mnt_id;
> > > > > >         tmp.stx_dio_mem_align = stat->dio_mem_align;
> > > > > >         tmp.stx_dio_offset_align = stat->dio_offset_align;
> > > > > > +       tmp.stx_subvol = stat->subvol;
> > > > > >
> > > > > >         return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
> > > > > >  }
> > > > > > diff --git a/include/linux/stat.h b/include/linux/stat.h
> > > > > > index 52150570d37a..bf92441dbad2 100644
> > > > > > --- a/include/linux/stat.h
> > > > > > +++ b/include/linux/stat.h
> > > > > > @@ -53,6 +53,7 @@ struct kstat {
> > > > > >         u32             dio_mem_align;
> > > > > >         u32             dio_offset_align;
> > > > > >         u64             change_cookie;
> > > > > > +       u64             subvol;
> > > > > >  };
> > > > > >
> > > > > >  /* These definitions are internal to the kernel for now. Mainly used by nfsd. */
> > > > > > diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> > > > > > index 2f2ee82d5517..67626d535316 100644
> > > > > > --- a/include/uapi/linux/stat.h
> > > > > > +++ b/include/uapi/linux/stat.h
> > > > > > @@ -126,8 +126,9 @@ struct statx {
> > > > > >         __u64   stx_mnt_id;
> > > > > >         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
> > > > > >         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
> > > > > > +       __u64   stx_subvol;     /* Subvolume identifier */
> > > > > >         /* 0xa0 */
> > > > > > -       __u64   __spare3[12];   /* Spare space for future expansion */
> > > > > > +       __u64   __spare3[11];   /* Spare space for future expansion */
> > > > > >         /* 0x100 */
> > > > > >  };
> > > > > >
> > > > > > @@ -155,6 +156,7 @@ struct statx {
> > > > > >  #define STATX_MNT_ID           0x00001000U     /* Got stx_mnt_id */
> > > > > >  #define STATX_DIOALIGN         0x00002000U     /* Want/got direct I/O alignment info */
> > > > > >  #define STATX_MNT_ID_UNIQUE    0x00004000U     /* Want/got extended stx_mount_id */
> > > > > > +#define STATX_SUBVOL           0x00008000U     /* Want/got stx_subvol */
> > > > > >
> > > > > >  #define STATX__RESERVED                0x80000000U     /* Reserved for future struct statx expansion */
> > > > > >
> > > > > > --
> > > > > > 2.43.0
> > > > > >
> > > > > >
> > > > >
> > > > > I think it's generally expected that patches that touch different
> > > > > layers are split up. That is, we should have a patch that adds the
> > > > > capability and a separate patch that enables it in bcachefs. This also
> > > > > helps make it clearer to others how a new feature should be plumbed
> > > > > into a filesystem.
> > > > >
> > > > > I would prefer it to be split up in this manner for this reason.
> > > >
> > > > I'll do it that way if the patch is big enough that it ought to be
> > > > split up. For something this small, seeing how it's used is relevant
> > > > context for both reviewers and people looking at it afterwards.
> > > >
> > > 
> > > It needs to also be split up because fs/ and fs/bcachefs are
> > > maintained differently. And while right now bcachefs is the only
> > > consumer of the API, btrfs will add it right after it's committed, and
> > > for people who are cherry-picking/backporting accordingly, having to
> > > chop out part of a patch would be unpleasant.
> > 
> > It's a new feature, not a bugfix, this should never get backported. And
> > I the bcachefs maintainer wrote the patch, and I'm submitting it to the
> > VFS maintainer, so if it's fine with him it's fine with me.
> 
> But then how am I supposed to bikeshed the structure of the V2 patchset
> by immediately asking you to recombine the patches and spit out a V3?
> 
> </sarcasm>

I see no reason to split this up. Especially given how small the patch
is. If there's a good reason such as meaningful merge conflicts then we
can always move to a stable branch for the infrastructure changes that
everyone else can pull from.

> 
> But, seriously, can you update the manpage too?  Is stx_subvol a u64
> cookie where userspace mustn't try to read anything into its contents?
> Just like st_ino and st_dev are (supposed) to be?

I was honestly hoping for a more elaborate patch description that would
have explained this. But I can just add this and yes, this is how I
conceptualized it and how we've always discussed it before.

Re: [PATCH v2] statx: stx_subvol

[Johannes Thumshirn]

On 08.03.24 03:29, Kent Overstreet wrote:
> Add a new statx field for (sub)volume identifiers, as implemented by
> btrfs and bcachefs.
> 
> This includes bcachefs support; we'll definitely want btrfs support as
> well.

For btrfs you can add the following:


 From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Date: Mon, 11 Mar 2024 09:09:36 +0100
Subject: [PATCH] btrfs: provide subvolume id for statx

Add the inode's subvolume id to the newly proposed statx subvol field.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
  fs/btrfs/inode.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 37701531eeb1..8cf692c708d7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8779,6 +8779,9 @@ static int btrfs_getattr(struct mnt_idmap *idmap,
  	generic_fillattr(idmap, request_mask, inode, stat);
  	stat->dev = BTRFS_I(inode)->root->anon_dev;

+	stat->subvol = BTRFS_I(inode)->root->root_key.objectid;
+	stat->result_mask |= STATX_SUBVOL;
+
  	spin_lock(&BTRFS_I(inode)->lock);
  	delalloc_bytes = BTRFS_I(inode)->new_delalloc_bytes;
  	inode_bytes = inode_get_bytes(inode);
-- 
2.35.3

Re: [PATCH v2] statx: stx_subvol

[Christian Brauner]

On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> On 08.03.24 03:29, Kent Overstreet wrote:
> > Add a new statx field for (sub)volume identifiers, as implemented by
> > btrfs and bcachefs.
> > 
> > This includes bcachefs support; we'll definitely want btrfs support as
> > well.
> 
> For btrfs you can add the following:
> 
> 
>  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Date: Mon, 11 Mar 2024 09:09:36 +0100
> Subject: [PATCH] btrfs: provide subvolume id for statx
> 
> Add the inode's subvolume id to the newly proposed statx subvol field.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---

Thanks, will fold, once I hear from Josef.

Re: [PATCH v2] statx: stx_subvol

[Kent Overstreet]

On Mon, Mar 11, 2024 at 02:43:11PM +0100, Christian Brauner wrote:
> On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> > On 08.03.24 03:29, Kent Overstreet wrote:
> > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > btrfs and bcachefs.
> > > 
> > > This includes bcachefs support; we'll definitely want btrfs support as
> > > well.
> > 
> > For btrfs you can add the following:
> > 
> > 
> >  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > Date: Mon, 11 Mar 2024 09:09:36 +0100
> > Subject: [PATCH] btrfs: provide subvolume id for statx
> > 
> > Add the inode's subvolume id to the newly proposed statx subvol field.
> > 
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > ---
> 
> Thanks, will fold, once I hear from Josef.

Can we try to make 6.9? Need to know what to put in the manpage, and
I've got userspace tooling that wants to use it.

Re: [PATCH v2] statx: stx_subvol

[Christian Brauner]

On Mon, Mar 11, 2024 at 04:15:04PM -0400, Kent Overstreet wrote:
> On Mon, Mar 11, 2024 at 02:43:11PM +0100, Christian Brauner wrote:
> > On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> > > On 08.03.24 03:29, Kent Overstreet wrote:
> > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > btrfs and bcachefs.
> > > > 
> > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > well.
> > > 
> > > For btrfs you can add the following:
> > > 
> > > 
> > >  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > Date: Mon, 11 Mar 2024 09:09:36 +0100
> > > Subject: [PATCH] btrfs: provide subvolume id for statx
> > > 
> > > Add the inode's subvolume id to the newly proposed statx subvol field.
> > > 
> > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > ---
> > 
> > Thanks, will fold, once I hear from Josef.
> 
> Can we try to make 6.9? Need to know what to put in the manpage, and
> I've got userspace tooling that wants to use it.

Hm, I understand that you want to see this done but I think it's just
too tight. If this would've been Acked last week then we could've done
it the second week of the merge window. So my reaction is to wait for
v6.10. What do others think?

Re: [PATCH v2] statx: stx_subvol

[David Sterba]

On Mon, Mar 11, 2024 at 02:43:11PM +0100, Christian Brauner wrote:
> On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> > On 08.03.24 03:29, Kent Overstreet wrote:
> > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > btrfs and bcachefs.
> > > 
> > > This includes bcachefs support; we'll definitely want btrfs support as
> > > well.
> > 
> > For btrfs you can add the following:
> > 
> > 
> >  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > Date: Mon, 11 Mar 2024 09:09:36 +0100
> > Subject: [PATCH] btrfs: provide subvolume id for statx
> > 
> > Add the inode's subvolume id to the newly proposed statx subvol field.
> > 
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > ---
> 
> Thanks, will fold, once I hear from Josef.

We're OK with it.

Re: [PATCH v2] statx: stx_subvol

[Christian Brauner]

On Mon, Mar 11, 2024 at 11:43:00PM +0100, David Sterba wrote:
> On Mon, Mar 11, 2024 at 02:43:11PM +0100, Christian Brauner wrote:
> > On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> > > On 08.03.24 03:29, Kent Overstreet wrote:
> > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > btrfs and bcachefs.
> > > > 
> > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > well.
> > > 
> > > For btrfs you can add the following:
> > > 
> > > 
> > >  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > Date: Mon, 11 Mar 2024 09:09:36 +0100
> > > Subject: [PATCH] btrfs: provide subvolume id for statx
> > > 
> > > Add the inode's subvolume id to the newly proposed statx subvol field.
> > > 
> > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > ---
> > 
> > Thanks, will fold, once I hear from Josef.
> 
> We're OK with it.

Thanks!

Re: [PATCH v2] statx: stx_subvol

[Neal Gompa]

On Tue, Mar 12, 2024 at 7:27 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Mar 11, 2024 at 11:43:00PM +0100, David Sterba wrote:
> > On Mon, Mar 11, 2024 at 02:43:11PM +0100, Christian Brauner wrote:
> > > On Mon, Mar 11, 2024 at 08:12:33AM +0000, Johannes Thumshirn wrote:
> > > > On 08.03.24 03:29, Kent Overstreet wrote:
> > > > > Add a new statx field for (sub)volume identifiers, as implemented by
> > > > > btrfs and bcachefs.
> > > > >
> > > > > This includes bcachefs support; we'll definitely want btrfs support as
> > > > > well.
> > > >
> > > > For btrfs you can add the following:
> > > >
> > > >
> > > >  From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
> > > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > > Date: Mon, 11 Mar 2024 09:09:36 +0100
> > > > Subject: [PATCH] btrfs: provide subvolume id for statx
> > > >
> > > > Add the inode's subvolume id to the newly proposed statx subvol field.
> > > >
> > > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > > > ---
> > >
> > > Thanks, will fold, once I hear from Josef.
> >
> > We're OK with it.
>
> Thanks!


Well, I guess I'm fine with the whole thing then if everyone else is.

Reviewed-by: Neal Gompa <neal@gompa.dev>



--
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v2] statx: stx_subvol

[Eric Biggers]

On Thu, Mar 07, 2024 at 09:29:12PM -0500, Kent Overstreet wrote:
>  	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
>  	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
> +	__u64	stx_subvol;	/* Subvolume identifier */
>  	/* 0xa0 */
> -	__u64	__spare3[12];	/* Spare space for future expansion */
> +	__u64	__spare3[11];	/* Spare space for future expansion */
>  	/* 0x100 */

The /* 0xa0 */ comment needs to be updated (or deleted).

- Eric