| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240311-anbringen-pelle-1d1eaa78961a@brauner>
Date: Mon, 11 Mar 2024 14:42:40 +0100
From: Christian Brauner <brauner@...nel.org>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Kent Overstreet <kent.overstreet@...ux.dev>,
Neal Gompa <neal@...pa.dev>, linux-fsdevel@...r.kernel.org, linux-bcachefs@...r.kernel.org,
linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org, Josef Bacik <josef@...icpanda.com>,
Miklos Szeredi <mszeredi@...hat.com>, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH v2] statx: stx_subvol
On Fri, Mar 08, 2024 at 08:56:33AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 08, 2024 at 11:48:31AM -0500, Kent Overstreet wrote:
> > On Fri, Mar 08, 2024 at 11:44:48AM -0500, Neal Gompa wrote:
> > > On Fri, Mar 8, 2024 at 11:34 AM Kent Overstreet
> > > <kent.overstreet@...ux.dev> 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@...ux.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/2uvhm6gweyl7iyyp2xpfryvcu2g3padagaeqcbiavjyiis6prl@yjm725bizncq/
> > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>
> > > > > > Cc: Josef Bacik <josef@...icpanda.com>
> > > > > > Cc: Miklos Szeredi <mszeredi@...hat.com>
> > > > > > Cc: Christian Brauner <brauner@...nel.org>
> > > > > > Cc: David Howells <dhowells@...hat.com>
> > > > > > Signed-off-by: Kent Overstreet <kent.overstreet@...ux.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 = 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.
> >
> > 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.
>
> 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.
Powered by blists - more mailing lists