linux-next: manual merge of the vfs-brauner tree with the bcachefs tree

  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.
  

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.

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.

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.

Thanks,
Amir.
  

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...?

> 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 :)

> 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...

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.
  

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.
  

On Thu, Oct 26, 2023 at 10:34:18PM +0300, Amir Goldstein wrote:
> On Thu, Oct 26, 2023 at 9:35 PM Kent Overstreet
> > > 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.

Wonderful

> > > 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.

Well, clearly not: since the cause of this whole thread was conflicts
with constants that were /not/ previously in this enum.

> 
> > 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}?

That'll do, I'll patch accordingly.