Re: [PATCH 0/3] fanotify support for btrfs sub-volumes

Christian Brauner
2023-11-08T16:20:16Z

On Wed, Nov 08, 2023 at 05:16:38PM +0100, Christian Brauner wrote:
> On Wed, Nov 08, 2023 at 06:08:09AM -0800, Christoph Hellwig wrote:
> > On Wed, Nov 08, 2023 at 09:27:44AM +0100, Christian Brauner wrote:
> > > > What is that flag going to buy us?
> > >
> > > The initial list that Josef provided in
> > > https://lore.kernel.org/linux-btrfs/20231025210654.GA2892534@perftesting
> > > asks to give users a way to figure out whether a file is located on a
> > > subvolume. Which I think is reasonable and there's a good chunk of
> > > software out there that could really benefit from this. Now all of the

I explained myself badly here. What I mean and what is immediately
useful is to add STATX_ATTR_SUBVOLUME_ROOT which works for both btrfs
and bcachefs and makes it easy for userspace to figure out whether an
inode is the root of a subvolume:

(This won't compile obviously.)

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index 166d8d8abe68..fce8603d37b0 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -776,6 +776,10 @@ static int bch2_getattr(struct mnt_idmap *idmap,
stat->attributes |= STATX_ATTR_NODUMP;
stat->attributes_mask |= STATX_ATTR_NODUMP;

+ if (BTRFS_is_subvol_root(inode))
+ stat->attributes_mask |= STATX_ATTR_SUBVOLUME_ROOT;
+ stat->attributes_mask |= STATX_ATTR_SUBVOLUME_ROOT;
+
return 0;
}

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5e3fccddde0c..c339a9a08d7e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8657,10 +8657,14 @@ static int btrfs_getattr(struct mnt_idmap *idmap,
if (bi_ro_flags & BTRFS_INODE_RO_VERITY)
stat->attributes |= STATX_ATTR_VERITY;

+ if (BCH2_is_subvol_root(inode))
+ stat->attributes |= STATX_ATTR_SUBVOLUME_ROOT;
+
stat->attributes_mask |= (STATX_ATTR_APPEND |
STATX_ATTR_COMPRESSED |
STATX_ATTR_IMMUTABLE |
- STATX_ATTR_NODUMP);
+ STATX_ATTR_NODUMP |
+ STATX_ATTR_SUBVOLUME_ROOT);

generic_fillattr(idmap, request_mask, inode, stat);
stat->dev = BTRFS_I(inode)->root->anon_dev;
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 7cab2c65d3d7..24d493babe63 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -187,6 +187,7 @@ struct statx {
#define STATX_ATTR_ENCRYPTED 0x00000800 /* [I] File requires key to decrypt in fs */
#define STATX_ATTR_AUTOMOUNT 0x00001000 /* Dir: Automount trigger */
#define STATX_ATTR_MOUNT_ROOT 0x00002000 /* Root of a mount */
+#define STATX_ATTR_SUBVOLUME_ROOT 0x00004000 /* Root of a subvolume */
#define STATX_ATTR_VERITY 0x00100000 /* [I] Verity protected file */
#define STATX_ATTR_DAX 0x00200000 /* File is currently in DAX state */

This would be a pretty big help for userspace already.

Right now all code needs to do a stat() and a statfs() and then check
the inode number. And that likely only works for btrfs.

This would also allow tools that want to to detect when they're crossing
into a new subvolume - be it on btrfs or bcachefs - and take appropriate
measures deciding what they want to do just relying on statx() without
any additional system calls.

And I think that's something we should do.