Received-SPF: pass (google.com: domain of linux-kernel+bounces-96946-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240308022914.196982-1-kent.overstreet@linux.dev>
In-Reply-To: <20240308022914.196982-1-kent.overstreet@linux.dev>
From: Neal Gompa <neal@gompa.dev>
Date: Fri, 8 Mar 2024 06:42:27 -0500
Message-ID: <CAEg-Je96OKs_LOXorNVj1a1=e+1f=-gw34v4VWNOmfKXc6PLSQ@mail.gmail.com>
Subject: Re: [PATCH v2] statx: stx_subvol
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: linux-fsdevel@vger.kernel.org, linux-bcachefs@vger.kernel.org, 
	linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Josef Bacik <josef@toxicpanda.com>, Miklos Szeredi <mszeredi@redhat.com>, 
	Christian Brauner <brauner@kernel.org>, David Howells <dhowells@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 7, 2024 at 9:29=E2=80=AFPM 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/2uvhm6gweyl7iyyp2xpfryvcu2g3p=
adagaeqcbiavjyiis6prl@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   =3D block_bytes(c);
>         stat->blocks    =3D inode->v.i_blocks;
>
> +       stat->subvol    =3D inode->ei_subvol;
> +       stat->result_mask |=3D STATX_SUBVOL;
> +
>         if (request_mask & STATX_BTIME) {
>                 stat->result_mask |=3D STATX_BTIME;
>                 stat->btime =3D 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 __use=
r *buffer)
>         tmp.stx_mnt_id =3D stat->mnt_id;
>         tmp.stx_dio_mem_align =3D stat->dio_mem_align;
>         tmp.stx_dio_offset_align =3D stat->dio_offset_align;
> +       tmp.stx_subvol =3D 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 di=
rect I/O */
>         __u32   stx_dio_offset_align;   /* File offset alignment for dire=
ct 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 al=
ignment 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 f=
uture 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.


--
=E7=9C=9F=E5=AE=9F=E3=81=AF=E3=81=84=E3=81=A4=E3=82=82=E4=B8=80=E3=81=A4=EF=
=BC=81/ Always, there's only one truth!