Re: [PATCH v2] statx: stx_subvol

[Neal Gompa]

On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
<kent.overstreet@xxxxxxxxx> 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@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.
>

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!