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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > Cc: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: David Howells <dhowells@xxxxxxxxxx>
> > Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> > ---
> >  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.