---
 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(-)

fs/bcachefs/fs.c +3 -0

index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);

fs/stat.c +1 -0

index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }

include/linux/stat.h +1 -0

index 52150570d37a..bf92441dbad2 100644
@@ -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. */

include/uapi/linux/stat.h +3 -1

index 2f2ee82d5517..67626d535316 100644
@@ -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 */

Signature · 1 line

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!

Kent Overstreet

author

2024-03-08 16:34 UTC #2 lore raw

Quoted 95 lines · Neal Gompa

Quoted 83 lines · 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/ open here

Signed-off-by Kent Overstreet <[email protected]>

Cc Josef Bacik <[email protected]>

Cc Miklos Szeredi <[email protected]>

Cc Christian Brauner <[email protected]>

Cc David Howells <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

---
 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(-)

fs/bcachefs/fs.c +3 -0

index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);

fs/stat.c +1 -0

index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }

include/linux/stat.h +1 -0

index 52150570d37a..bf92441dbad2 100644
@@ -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. */

include/uapi/linux/stat.h +3 -2

index 2f2ee82d5517..67626d535316 100644
@@ -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 */
 
--

Quoted 3 lines

2.43.0

Quoted 1 line

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.

Neal Gompa

2024-03-08 16:45 UTC #3 lore raw

Quoted 97 lines · Kent Overstreet

Quoted 95 lines · Neal Gompa

Quoted 83 lines · 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/ open here

Signed-off-by Kent Overstreet <[email protected]>

Cc Josef Bacik <[email protected]>

Cc Miklos Szeredi <[email protected]>

Cc Christian Brauner <[email protected]>

Cc David Howells <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

---
 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(-)

fs/bcachefs/fs.c +3 -0

index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);

fs/stat.c +1 -0

index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }

include/linux/stat.h +1 -0

index 52150570d37a..bf92441dbad2 100644
@@ -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. */

include/uapi/linux/stat.h +3 -2

index 2f2ee82d5517..67626d535316 100644
@@ -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 */
 
--

Quoted 3 lines

2.43.0

Quoted 6 lines

Quoted 1 line
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.

Signature · 1 line

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

Kent Overstreet

author

2024-03-08 16:48 UTC #4 lore raw

Quoted 106 lines · Neal Gompa

Quoted 97 lines · Kent Overstreet

Quoted 95 lines · Neal Gompa

Quoted 83 lines · 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/ open here

Signed-off-by Kent Overstreet <[email protected]>

Cc Josef Bacik <[email protected]>

Cc Miklos Szeredi <[email protected]>

Cc Christian Brauner <[email protected]>

Cc David Howells <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

---
 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(-)

fs/bcachefs/fs.c +3 -0

index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);

fs/stat.c +1 -0

index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }

include/linux/stat.h +1 -0

index 52150570d37a..bf92441dbad2 100644
@@ -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. */

include/uapi/linux/stat.h +3 -2

index 2f2ee82d5517..67626d535316 100644
@@ -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 */
 
--

Quoted 3 lines

2.43.0

Quoted 6 lines

Quoted 1 line
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.

Quoted 5 lines

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.

Darrick J. Wong

2024-03-08 16:56 UTC #5 lore raw

Quoted 113 lines · Kent Overstreet

Quoted 106 lines · Neal Gompa
Quoted 97 lines · Kent Overstreet
Quoted 95 lines · Neal Gompa
Quoted 83 lines · 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/ open here
Signed-off-by Kent Overstreet <[email protected]>
Cc Josef Bacik <[email protected]>
Cc Miklos Szeredi <[email protected]>
Cc Christian Brauner <[email protected]>
Cc David Howells <[email protected]>
Signed-off-by Kent Overstreet <[email protected]>
---
 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(-)
fs/bcachefs/fs.c +3 -0
index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);
fs/stat.c +1 -0
index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }
include/linux/stat.h +1 -0
index 52150570d37a..bf92441dbad2 100644
@@ -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. */
include/uapi/linux/stat.h +3 -2
index 2f2ee82d5517..67626d535316 100644
@@ -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 */
 
--
Quoted 3 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 5 lines
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.

Quoted 3 lines

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

Kent Overstreet

author

2024-03-08 17:13 UTC #6 lore raw

Quoted 11 lines · Darrick J. Wong

Quoted 3 lines · Kent Overstreet
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?

Quoted 3 lines

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.

Quoted 2 lines

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

Maybe?

Jeff Layton

2024-03-09 11:46 UTC #7 lore raw

Quoted 6 lines · Kent Overstreet

Quoted 5 lines · Darrick J. Wong
Quoted 3 lines · Kent Overstreet
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.

Quoted 9 lines

Quoted 6 lines
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

Quoted 21 lines

Quoted 3 lines
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.

Quoted 6 lines

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?

Quoted 5 lines

Quoted 2 lines
Should the XFS data and rt volumes be reported with different stx_vol values?
Maybe?

Signature · 1 line

Jeff Layton <[email protected]>

Kent Overstreet

author

2024-03-09 12:15 UTC #8 lore raw

Quoted 55 lines · Jeff Layton

Quoted 6 lines · Kent Overstreet
Quoted 5 lines · Darrick J. Wong
Quoted 3 lines · Kent Overstreet
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.
Quoted 9 lines
Quoted 6 lines
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
Quoted 21 lines
Quoted 3 lines
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.
Quoted 6 lines
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.

Quoted 5 lines

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

Dave Chinner

2024-03-11 02:17 UTC #9 lore raw

In reply to Darrick J. Wong

Quoted 2 lines · Darrick J. Wong

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.

Signature · 2 lines

Dave Chinner
[email protected]

Miklos Szeredi

2024-03-11 05:30 UTC #10 lore raw

Quoted 10 lines · Dave Chinner

Quoted 2 lines · Darrick J. Wong
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

Kent Overstreet

author

2024-03-11 05:49 UTC #11 lore raw

Quoted 12 lines · Miklos Szeredi

Quoted 10 lines · Dave Chinner
Quoted 2 lines · Darrick J. Wong
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?

Quoted 7 lines

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.

Christian Brauner

maintainer

2024-03-11 13:42 UTC #12 lore raw

In reply to Darrick J. Wong

Quoted 114 lines · Darrick J. Wong

Quoted 113 lines · Kent Overstreet
Quoted 111 lines · Neal Gompa
Quoted 97 lines · Kent Overstreet
Quoted 95 lines · Neal Gompa
Quoted 83 lines · 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/ open here
Signed-off-by Kent Overstreet <[email protected]>
Cc Josef Bacik <[email protected]>
Cc Miklos Szeredi <[email protected]>
Cc Christian Brauner <[email protected]>
Cc David Howells <[email protected]>
Signed-off-by Kent Overstreet <[email protected]>
---
 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(-)
fs/bcachefs/fs.c +3 -0
index 3f073845bbd7..6a542ed43e2c 100644
@@ -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);
fs/stat.c +1 -0
index 77cdc69eb422..70bd3e888cfa 100644
@@ -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;
 }
include/linux/stat.h +1 -0
index 52150570d37a..bf92441dbad2 100644
@@ -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. */
include/uapi/linux/stat.h +3 -2
index 2f2ee82d5517..67626d535316 100644
@@ -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 */
 
--
Quoted 3 lines
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.
Quoted 6 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 4 lines

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.

Johannes Thumshirn

2024-03-11 08:12 UTC #13 lore raw

In reply to Kent Overstreet

Quoted 5 lines · 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.

For btrfs you can add the following:

From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
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 <[email protected]>

---
  fs/btrfs/inode.c | 3 +++
  1 file changed, 3 insertions(+)

fs/btrfs/inode.c +3 -0

index 37701531eeb1..8cf692c708d7 100644
@@ -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);

Signature · 1 line

2.35.3

Christian Brauner

maintainer

2024-03-11 13:43 UTC #14 lore raw

Quoted 15 lines · Johannes Thumshirn

Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
Date: Mon, 11 Mar 2024 09:09:36 +0100
Subject: [PATCH] btrfs: provide subvolume id for statx

Quoted 4 lines

Add the inode's subvolume id to the newly proposed statx subvol field.
Signed-off-by Johannes Thumshirn <[email protected]>
---

Thanks, will fold, once I hear from Josef.

Kent Overstreet

author

2024-03-11 20:15 UTC #15 lore raw

Quoted 16 lines · Christian Brauner

Quoted 15 lines · Johannes Thumshirn
Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
Date: Mon, 11 Mar 2024 09:09:36 +0100
Subject: [PATCH] btrfs: provide subvolume id for statx

Quoted 6 lines

Quoted 4 lines
Add the inode's subvolume id to the newly proposed statx subvol field.
Signed-off-by Johannes Thumshirn <[email protected]>
---
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.

Christian Brauner

maintainer

2024-03-12 14:27 UTC #16 lore raw

Quoted 19 lines · Kent Overstreet

Quoted 18 lines · Christian Brauner
Quoted 17 lines · Johannes Thumshirn
Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Signed-off-by Johannes Thumshirn <[email protected]>
---
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?

David Sterba

2024-03-11 22:50 UTC #17 lore raw

In reply to Christian Brauner

Quoted 16 lines · Christian Brauner

Quoted 15 lines · Johannes Thumshirn
Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
Date: Mon, 11 Mar 2024 09:09:36 +0100
Subject: [PATCH] btrfs: provide subvolume id for statx

Quoted 6 lines

Quoted 4 lines
Add the inode's subvolume id to the newly proposed statx subvol field.
Signed-off-by Johannes Thumshirn <[email protected]>
---
Thanks, will fold, once I hear from Josef.

We're OK with it.

Christian Brauner

maintainer

2024-03-12 14:27 UTC #18 lore raw

Quoted 17 lines · David Sterba

Quoted 16 lines · Christian Brauner
Quoted 15 lines · Johannes Thumshirn
Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
Date: Mon, 11 Mar 2024 09:09:36 +0100
Subject: [PATCH] btrfs: provide subvolume id for statx

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Add the inode's subvolume id to the newly proposed statx subvol field.
Signed-off-by Johannes Thumshirn <[email protected]>
---
Thanks, will fold, once I hear from Josef.
We're OK with it.

Thanks!

Neal Gompa

Reviewed-by

2024-03-12 17:18 UTC #19 lore raw

Quoted 21 lines · Christian Brauner

Quoted 19 lines · David Sterba
Quoted 18 lines · Christian Brauner
Quoted 17 lines · Johannes Thumshirn
Quoted 5 lines · 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.
For btrfs you can add the following:
From 82343b7cb2a947bca43234c443b9c22339367f68 Mon Sep 17 00:00:00 2001
From: Johannes Thumshirn <[email protected]>
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Signed-off-by Johannes Thumshirn <[email protected]>
---
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 <[email protected]>

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

Eric Biggers

2024-03-12 02:13 UTC #20 lore raw

In reply to Kent Overstreet

include/uapi/linux/stat.h struct statx { · line 132

Quoted 7 lines · Kent Overstreet

include/uapi/linux/stat.h +2 -1

 	__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

John Garry

2024-05-28 12:46 UTC #21 lore raw

Quoted 9 lines · Eric Biggers

include/uapi/linux/stat.h struct statx { · line 132
Quoted 7 lines · Kent Overstreet
__u32 stx_dio_mem_align; /* Memory buffer alignment for direct I/O */
__u32 stx_dio_offset_align; /* File offset alignment for direct I/O */
include/uapi/linux/stat.h +2 -1
+	__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).

I would tend to agree. Was this intentionally not updated (or deleted)?

Andreas Dilger

2024-05-28 22:11 UTC #22 lore raw

Quoted 14 lines · John Garry

Quoted 9 lines · Eric Biggers
include/uapi/linux/stat.h struct statx { · line 132
Quoted 7 lines · Kent Overstreet
include/uapi/linux/stat.h +2 -1
 	__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).

Quoted 1 line

I would tend to agree. Was this intentionally not updated (or deleted)?

More correct would be to add the new stx_subvol field after the "0xa0" comment so that it is clear at what offset in the struct this field is.

Cheers, Andreas