From: Amir Goldstein <amir73il@gmail.com>
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>,
Christian Brauner <brauner@kernel.org>,
Kent Overstreet <kent.overstreet@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux Next Mailing List <linux-next@vger.kernel.org>,
Jeff Layton <jlayton@kernel.org>,
Chuck Lever <chuck.lever@oracle.com>
Subject: Re: linux-next: manual merge of the vfs-brauner tree with the bcachefs tree
Date: Thu, 26 Oct 2023 22:34:18 +0300 [thread overview]
Message-ID: <CAOQ4uxhNDADk9CgSMxKc93qunDUD17AFaA+tuSni9AOsjkfPYw@mail.gmail.com> (raw)
In-Reply-To: <20231026183539.cffe6uljmnjgacxq@moria.home.lan>
On Thu, Oct 26, 2023 at 9:35 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> On Thu, Oct 26, 2023 at 08:16:14AM +0300, Amir Goldstein wrote:
> > On Thu, Oct 26, 2023 at 2:02 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > >
> > > Hi all,
> > >
> > > Today's linux-next merge of the vfs-brauner tree got a conflict in:
> > >
> > > include/linux/exportfs.h
> > >
> > > between commit:
> > >
> > > 85e95ca7cc48 ("bcachefs: Update export_operations for snapshots")
> > >
> > > from the bcachefs tree and commit:
> > >
> > > 2560fa66d2ac ("exportfs: define FILEID_INO64_GEN* file handle types")
> > >
> > > from the vfs-brauner tree.
> > >
> > > I fixed it up (see below) and can carry the fix as necessary. This
> > > is now fixed as far as linux-next is concerned, but any non trivial
> > > conflicts should be mentioned to your upstream maintainer when your tree
> > > is submitted for merging. You may also want to consider cooperating
> > > with the maintainer of the conflicting tree to minimise any particularly
> > > complex conflicts.
> > >
> > > --
> > > Cheers,
> > > Stephen Rothwell
> >
> > [adding exportfs maintainers]
> >
> > >
> > > diff --cc include/linux/exportfs.h
> > > index be9900cc8786,21bae8bfeef1..000000000000
> > > --- a/include/linux/exportfs.h
> > > +++ b/include/linux/exportfs.h
> > > @@@ -98,12 -98,17 +98,23 @@@ enum fid_type
> > > */
> > > FILEID_FAT_WITH_PARENT = 0x72,
> > >
> > > + /*
> > > + * 64 bit inode number, 32 bit subvolume, 32 bit generation number:
> > > + */
> > > + FILEID_BCACHEFS_WITHOUT_PARENT = 0x80,
> > > + FILEID_BCACHEFS_WITH_PARENT = 0x81,
> > > +
> > > + /*
> > > + * 64 bit inode number, 32 bit generation number.
> > > + */
> > > - FILEID_INO64_GEN = 0x81,
> > > ++ FILEID_INO64_GEN = 0x82,
> > > +
> > > + /*
> > > + * 64 bit inode number, 32 bit generation number,
> > > + * 64 bit parent inode number, 32 bit parent generation.
> > > + */
> > > - FILEID_INO64_GEN_PARENT = 0x82,
> > > ++ FILEID_INO64_GEN_PARENT = 0x83,
> > > +
> >
> > This is wrong.
> > Those are filesystem defined constants.
> > Please don't change them.
> >
> > 0x81/0x82 have been used by xfs and fuse for years,
> > even though neither defined a constant in this enum so far.
>
> Perhaps we could get that fixed...?
commit 2560fa66d2ac ("exportfs: define FILEID_INO64_GEN*
file handle types") fixes that for fuse.
I may fix up xfs to use these constants later.
>
> > Conflicting with FILEID_BCACHEFS_WITH_PARENT is not
> > a serious issue, but I encourage Kent to pick different constants
> > for bcachefs or keep the bcachefs constants out of this enum.
>
> Happy to do so. Since it seems this enum doesn't have all the constants
> I'd need to avoid conflicting with, I might need some help here :)
>
Technically, you don't *need* to avoid conflicting with fileid types
of other filesystems and you do not *need* to define your constant
in this enum. It serves no real purpose unless your constant
declares a fileid format that other filesystems also use.
See the comment at the top of the enum.
> > It is a slight inconvenience for users that have bcachefs exported
> > to NFS clients and upgrade their server, but maybe that is acceptable.
> > In overlayfs, we encoded type OVL_FILEID_V0 and switched to encoding
> > type OVL_FILEID_V1, but we still accept decoding of both types, neither
> > of which are listed in this enum BTW.
> >
> > Adding fid types to this enum is not required.
> > This enum is a place to standardize and for different fs to share the same
> > fid type/encoding as is the case with FILEID_INO{32,64}_GEN*.
> > IMO, the bcachefs constant do not follow the convention in this
> > enum and their format is unlikely to be used by other fs, so
> > they should not be added to this enum at all.
>
> Eh?
>
> Most of the constants here appear to be completely filesystem specific -
> I see UDF, nilfs, btrfs, fat...
>
There is no good reason for those to be in the enum either
other than documentation.
> And since you also don't want conflicts with fid_types that aren't
> defined here, it seems like they really should all be here.
If you define your constants internally in bcachefs, I don't care
about conflicts, but if I were you, I would avoid conflicts with
the known types.
If you want to define your constants in this enum please choose
any vacant 0x?{1,2} values. 0xb{1,2}?
Thanks,
Amir.
next prev parent reply other threads:[~2023-10-26 19:34 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-25 23:01 linux-next: manual merge of the vfs-brauner tree with the bcachefs tree Stephen Rothwell
2023-10-26 5:16 ` Amir Goldstein
2023-10-26 18:35 ` Kent Overstreet
2023-10-26 19:34 ` Amir Goldstein [this message]
2023-10-26 20:37 ` Kent Overstreet
2024-02-12 23:04 Stephen Rothwell
2024-02-14 23:05 Stephen Rothwell
2024-02-26 0:12 Stephen Rothwell
2024-03-12 3:56 ` Stephen Rothwell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxhNDADk9CgSMxKc93qunDUD17AFaA+tuSni9AOsjkfPYw@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=jlayton@kernel.org \
--cc=kent.overstreet@gmail.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=sfr@canb.auug.org.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).